Hi Ben.  Thank you for this branch.  Land with changes (specifically
reinstating some of Nicola's changes that you lost, as mentioned below).

Tests pass for me.

The functionality works well, except for variants of the pre-existing
bug 1099921: moving services while the delta stream is being processed
leads to unexpected and undesired behavior.  Either the drag is stopped,
or the drag completes but the processed location then moves the service
to where it was before.  Hopefully we can get that addressed soon.

If it helps move things along in terms of Monday/Tuesday delivery, I
would be OK with you landing this and then responding to the
second/subsequent reviews in a separate branch.  OTOH, if it doesn't
help you, don't do it. :-)

Thanks

Gary


https://codereview.appspot.com/7132061/diff/6001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7132061/diff/6001/app/app.js#newcode351
app/app.js:351: active.update();
cool

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode28
app/views/topology/service.js:28: mouseout: 'serviceStatusMouseOut'
Moving the functions out of the event registration does make it easier
to read.  However, it makes the diff much harder to read.  For instance,
AFAICT, there are no changes to these functions, but I'm coming to that
conclusion by eyeball rather than by mechanical diff.  This would be the
kind of change I'd like to see in a separate branch to keep the branch
size down, to reduce the development time, and to ease reviews of the
pertinent code changes to actually implement this feature.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode33
app/views/topology/service.js:33: * relation.
We now have this comment both in the event registration and in the
function definition.  I'm not sure that inline event handler functions
don't have their place, to be honest.  If we do separate them, I think
we can only expect to maintain comments in one place.  I believe that
the place should be the function, so I would recommend deleting this
copy of the comment.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode65
app/views/topology/service.js:65: toggleControlPanel: {callback:
function() {
I prefer the change that Nicola made, that you have reverted.  In that
change, he renamed "ControlPanel" to "ServiceMenu," which seemed like a
naming improvement to Nicola and to his two reviewers.  He also was able
to be explicit that we only needed to "hide" here, which would be nice
if we can manage it.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode178
app/views/topology/service.js:178:
self.service_click_actions.toggleControlPanel(null, self);
You've reverted Nicola's changes.  I think they were improvements, but
even if you disagree, socially a discussion would be better.  Please
don't do this.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode189
app/views/topology/service.js:189: .toggleControlPanel(box, context);
You've reverted Nicola's changes.  Please don't do this.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode201
app/views/topology/service.js:201: .toggleControlPanel(box, context);
You've reverted Nicola's changes.  Please don't do this.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode285
app/views/topology/service.js:285:
self.service_click_actions.toggleControlPanel(null, self);
If you use Nicola's changes, this will need to change as well.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode316
app/views/topology/service.js:316: drag: function(d, self, pos,
includeTransition) {
Cool.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js#newcode869
app/views/topology/service.js:869: * @param {object} module The service
module..
Thank you for correcting this.

https://codereview.appspot.com/7132061/diff/6001/undocumented
File undocumented (left):

https://codereview.appspot.com/7132061/diff/6001/undocumented#oldcode1
undocumented:1: app/app.js:599 "callback"
You increased the undocumented count from 241 to 246.  It would be nice
if we at least kept the number stable across these refactorings.  Given
my desire to be able to declare this complete by Monday, or Tuesday at
the latest, I won't stop you from landing for this, but maybe you could
come back to write up five methods later.

https://codereview.appspot.com/7132061/