Removes the browser feature flag.

Removes the browser feature flag.

Does what it says on the tin.

https://code.launchpad.net/~jcsackett/juju-gui/unflag-browser/+merge/168507

(do not edit description out of merge proposal)

[j.c.sackett, 2013-06-10 18:16:58]

Please take a look.

[rharding, 2013-06-10 18:26:22]

Couple of notes, will QA and then get back with a final looks good.

https://codereview.appspot.com/10156043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10156043/diff/1/app/app.js#newcode355
app/app.js:355: this.subApplications.push({
This can just be placed into the definition of subApplications at the top of the
class def.

https://codereview.appspot.com/10156043/diff/1/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/10156043/diff/1/app/subapps/browser/browser.js...
app/subapps/browser/browser.js:244:
/^\/?(sidebar|fullscreen|minimized|search|test|test\/index\.html)\/?(search)?\/?/;
I'd like to QA this and check why it was necessary. It wasn't an issue before,
but code has changed since we had the browser by default working.

https://codereview.appspot.com/10156043/diff/1/app/subapps/browser/browser.js...
app/subapps/browser/browser.js:719: 
no double space please.

[matthew.scott, 2013-06-10 18:31:02]

LGTM, though I'll defer to Rick.  One thing for a future branch: clicking on
"Charm Browser" while in fullscreen generates a URL like
http://localhost:8888/fullscreen/search/?categories=databases&series=precise&...

[rharding, 2013-06-10 18:52:35]

I've pushed up a branch that fixes the test issue you hit.

lp:~rharding/juju-gui/sanity-check-default

Adding /test to the regex check for ids might have unintended consequences as
it's limiting the name space for valid packages down the road. I'd prefer if we
could fix the tests themselves and the route code (the code that's running when
it's not intended to) than to hack in a /test check instead. It's just not
getting the original issue.

[j.c.sackett, 2013-06-10 21:06:20]

On 2013/06/10 18:52:35, rharding wrote:
> I've pushed up a branch that fixes the test issue you hit.
> 
> lp:~rharding/juju-gui/sanity-check-default
> 
> Adding /test to the regex check for ids might have unintended consequences as
> it's limiting the name space for valid packages down the road. I'd prefer if
we
> could fix the tests themselves and the route code (the code that's running
when
> it's not intended to) than to hack in a /test check instead. It's just not
> getting the original issue.

I'm not sure your changes get to the original issue either (as we haven't IDed
the issue). However, I like that yours is *way* less likely to have unintended
consequences, as we should always have a store in production. I've merged in
your changes, and will push up.

If you're good with those changes, I can land sometime this evening; otherwise
catch me in the morning and lets chat.

[j.c.sackett, 2013-06-10 21:10:38]

Please take a look.

[rharding, 2013-06-10 21:16:34]

LGTM after removing the push() call that's not needed.

[j.c.sackett, 2013-06-11 13:28:07]

*** Submitted:

Removes the browser feature flag.

Does what it says on the tin.

R=rharding, matthew.scott
CC=
https://codereview.appspot.com/10156043