Move the featureFlag processor into window scope.

Move the featureFlag processor into window scope.

- In order to check for feature flags the processing must be done before the
app starts
- Adds some tests to verify we parse the url correctly with our regex/etc.
- Update the app.js to not deal with feature flags. It's done in index.html.
- Update the simulateEvents to be triggered by the feature flag existing vs
the custom route.

https://code.launchpad.net/~rharding/juju-gui/global-ff/+merge/165438

(do not edit description out of merge proposal)

[rharding, 2013-05-23 16:37:38]

Please take a look.

[rharding, 2013-05-23 16:46:24]

Please take a look.

[bcsaller, 2013-05-23 17:18:49]

Thanks for this, I think this is moving in the right direction but I did have a
few comments/concerns.

https://codereview.appspot.com/9682046/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode238
app/index.html:238: /**
What do you think about moving this into its own script and including that
directly? Sure its another request but it will help curb the tendency to keep
growing index.html.

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode343
app/index.html:343: GlobalConfig.flags || {}
Does passing flags in this way somewhat conflate flags with config? If
GlobalConfig.flags has any values hasn't that moved past the 'experimental
feature flag' view of the world and promoted them to config?

https://codereview.appspot.com/9682046/diff/3001/test/test_startup.js.bottom
File test/test_startup.js.bottom (right):

https://codereview.appspot.com/9682046/diff/3001/test/test_startup.js.bottom#...
test/test_startup.js.bottom:83: assert.deepEqual(window.flags, {});
If a test prior to this sets a flag I think we'll have an issue. This seems
possible to me.

[gary.poster, 2013-05-23 17:23:36]

https://codereview.appspot.com/9682046/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode238
app/index.html:238: /**
On 2013/05/23 17:18:50, bcsaller wrote:
> What do you think about moving this into its own script and including that
> directly? Sure its another request but it will help curb the tendency to keep
> growing index.html.

FWIW, this was the approach we discussed in a pre-imp.  I see your point and
don't disagree, but I'd rather not have this possibility block Rick from landing
the branch.  OTOH, if he'd like to make that change, great.

[jeff.pihach, 2013-05-23 17:28:22]

LGTM Thanks there is a trivial request if you so choose.

https://codereview.appspot.com/9682046/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9682046/diff/1/app/index.html#newcode284
app/index.html:284: // Sometimes it matches a trailing / as a second item.
This should be able to be handled in the regex - maybe someone better at them
than me can help with the solution :)

https://codereview.appspot.com/9682046/diff/1/app/index.html#newcode342
app/index.html:342: window.location.href,
you should be able to use window.location.pathname here

[rharding, 2013-05-23 17:29:43]

Responses below.

https://codereview.appspot.com/9682046/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode238
app/index.html:238: /**
On 2013/05/23 17:18:50, bcsaller wrote:
> What do you think about moving this into its own script and including that
> directly? Sure its another request but it will help curb the tendency to keep
> growing index.html.

This branch goes with the guidance of Jeff and pre-imp with Gary. 

Ideally I'd prefer if this could be brought in as a YUI module and just required
in the main YUI block in index.html. It could provide an actual api for
getting/setting flags and be more flexible. This is a basic 'move' the current
code.

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode343
app/index.html:343: GlobalConfig.flags || {}
On 2013/05/23 17:18:50, bcsaller wrote:
> Does passing flags in this way somewhat conflate flags with config? If
> GlobalConfig.flags has any values hasn't that moved past the 'experimental
> feature flag' view of the world and promoted them to config?

One of the goals I'll have in a follow up is to work with the Gui charm to allow
for setting flags in the config. So yes, but it's intended. For instance, our
staging server for testing things with the browser would want to deploy the
charm and set the feature flag globally from the charm config into the
application for all users so that the url doesn't need to be tweaked for each
call.

Feature flags are meant to be deprecated and removed. I think it's a bit
different from config. In looking at the current config I think if we get to the
next step of allowing feature flags to be defined by the config then things like
simulateEvents could move to being standardized as feature flags vs a
combination of feature flag or config.

https://codereview.appspot.com/9682046/diff/3001/test/test_startup.js.bottom
File test/test_startup.js.bottom (right):

https://codereview.appspot.com/9682046/diff/3001/test/test_startup.js.bottom#...
test/test_startup.js.bottom:83: assert.deepEqual(window.flags, {});
On 2013/05/23 17:18:50, bcsaller wrote:
> If a test prior to this sets a flag I think we'll have an issue. This seems
> possible to me.

Well, if another test fails to clean up after itself it can't be the fault of
this code. This test is a good hint and should fail if someone doesn't clean up
after themselves before this test runs.

[bcsaller, 2013-05-23 17:34:21]

LGTM

Thanks for the follow up.

[rharding, 2013-05-23 18:50:58]

*** Submitted:

Move the featureFlag processor into window scope.

- In order to check for feature flags the processing must be done before the
app starts
- Adds some tests to verify we parse the url correctly with our regex/etc.
- Update the app.js to not deal with feature flags. It's done in index.html.
- Update the simulateEvents to be triggered by the feature flag existing vs
the custom route.

R=bcsaller, gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/9682046