Code review comment for lp://staging/~gary/juju-gui/bug1167967

LGTM.

Some suggestions around the url handling but take it or leave it.

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode171
app/assets/javascripts/ns-routing-app-extension.js:171: url += ':' + ns
+ ':' + base[ns];
Maybe a flag to the method to control this behavior so we can use this
method to generate all such links?

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js
File app/views/charm.js (right):

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js#newcode100
app/views/charm.js:100: this.fire('navigateTo', {url: '/:gui:/'});
In cases like this I previously recommended that we use nsRouter.url to
create the URL. That no longer works in this case as '/' becomes
implicit in the namespace.

I like the abstraction of something that can build and assemble proper
URLs for the developer. Maybe a flag to nsRouter.url that forces
inclusion. I'll make a note in the router code as well but its up to you
if you agree or not.

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js
File app/views/landscape.js (left):

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js#oldcode132
app/views/landscape.js:132: if (!modelAnnotation) {
oops. While improv doesn't add these annotations to new nodes it was my
understanding that landscape will? Still, I'm sure there is going to be
some latency in that process and these checks are valid.

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js
File test/test_routing.js (right):

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js#newcode53
test/test_routing.js:53: assert.strictEqual(u, '/:foo:/shazam/');
The reason I originally went the other way was I though presence in the
URL could indicate state, such that :inspector:/ might indicate an open
panel. I think you're right though that there are other ways to model it
and making '/' implicit is fine.

https://codereview.appspot.com/8680043/