[Chromoting] Add ability to enable/disable GDrive support per-app in gyp.

[Chromoting] Add ability to enable/disable GDrive support per-app in gyp.

Since not all AppRemoting apps will want or need GDrive support to be
enabled, we need to be able to control which apps enable this feature.

This cl adds 'app_features' to the AppRemoting gyp targets and passes
the list of features into the build_webapp script (so the manifest can
have the correct oauth scopes) and the webapp.

Also, because the 'cast' capability was "in the way" for these changes,
it has been converted into a feature as well (default: disabled).

BUG=

Committed: https://crrev.com/12564e898915dd71a082b1082d72a5f9521e778c
Cr-Commit-Position: refs/heads/master@{#314408}

[garykac, 2015-02-02 22:20:22]

garykac@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2015-02-02 22:57:47]

https://codereview.chromium.org/895523004/diff/1/remoting/app_remoting_webapp...
File remoting/app_remoting_webapp.gyp (right):

https://codereview.chromium.org/895523004/diff/1/remoting/app_remoting_webapp...
remoting/app_remoting_webapp.gyp:72: # Sample app with no features enabled.
Is there any value in having this sample app? I think it's pretty clear from the
'app_features' field how you would modify it to disable GDrive, so it doesn't
have much value as a tutorial, and it presumably takes up space on buildbots and
archives. Unless we need it for something, I suggest just having one sample app,
which supports GDrive.

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/base/js/app_...
File remoting/webapp/base/js/app_features.js (right):

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/base/js/app_...
remoting/webapp/base/js/app_features.js:11: remoting.app_features =
['APPLICATION_FEATURES'];
It feels to me like features should be an enumerated type, rather than free-form
strings. This file would declare the enum as well as this array, or (better
still) just use remoting.ClientSession.Capability.

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/base/js/appl...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/base/js/appl...
remoting/webapp/base/js/application.js:16: * @param {Array.<string>}
app_features Array of application features.
There's confusion in this file as to "features" vs. "capabilities"; particularly
in getRequiredCapabilities, where they are used interchangeably. We should be
consistent wrt naming.

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/build-webapp.py
File remoting/webapp/build-webapp.py (right):

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/build-webapp...
remoting/webapp/build-webapp.py:223: 
No need for an extra blank line here (IIRC, that convention is used to separate
top-level definitions in Python).

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/build-webapp...
remoting/webapp/build-webapp.py:370: valid_app_features = ['cast', 'gdrive']
If you use an enumerated type, you can get rid of this check--jscompile will do
it for you.

[garykac, 2015-02-03 01:50:21]

ptal

https://codereview.chromium.org/895523004/diff/1/remoting/app_remoting_webapp...
File remoting/app_remoting_webapp.gyp (right):

https://codereview.chromium.org/895523004/diff/1/remoting/app_remoting_webapp...
remoting/app_remoting_webapp.gyp:72: # Sample app with no features enabled.
On 2015/02/02 22:57:47, Jamie wrote:
> Is there any value in having this sample app? I think it's pretty clear from
the
> 'app_features' field how you would modify it to disable GDrive, so it doesn't
> have much value as a tutorial, and it presumably takes up space on buildbots
and
> archives. Unless we need it for something, I suggest just having one sample
app,
> which supports GDrive.

Done.

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/base/js/app_...
File remoting/webapp/base/js/app_features.js (right):

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/base/js/app_...
remoting/webapp/base/js/app_features.js:11: remoting.app_features =
['APPLICATION_FEATURES'];
On 2015/02/02 22:57:47, Jamie wrote:
> It feels to me like features should be an enumerated type, rather than
free-form
> strings. This file would declare the enum as well as this array, or (better
> still) just use remoting.ClientSession.Capability.

Done.

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/base/js/appl...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/base/js/appl...
remoting/webapp/base/js/application.js:16: * @param {Array.<string>}
app_features Array of application features.
On 2015/02/02 22:57:47, Jamie wrote:
> There's confusion in this file as to "features" vs. "capabilities";
particularly
> in getRequiredCapabilities, where they are used interchangeably. We should be
> consistent wrt naming.

Done.

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/build-webapp.py
File remoting/webapp/build-webapp.py (right):

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/build-webapp...
remoting/webapp/build-webapp.py:223: 
On 2015/02/02 22:57:47, Jamie wrote:
> No need for an extra blank line here (IIRC, that convention is used to
separate
> top-level definitions in Python).

I fixed this locally but forgot to upload. grr.

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/build-webapp...
remoting/webapp/build-webapp.py:370: valid_app_features = ['cast', 'gdrive']
On 2015/02/02 22:57:47, Jamie wrote:
> If you use an enumerated type, you can get rid of this check--jscompile will
do
> it for you.

Done.

[Jamie, 2015-02-03 18:06:24]

lgtm

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/build-webapp.py
File remoting/webapp/build-webapp.py (right):

https://codereview.chromium.org/895523004/diff/1/remoting/webapp/build-webapp...
remoting/webapp/build-webapp.py:78: 
I think this blank line should stay, because it separates top-level definitions.

https://codereview.chromium.org/895523004/diff/40001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/895523004/diff/40001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:58: capabilities.push.apply(capabilities,
this.app_capabilities_);
I think this would be better done in the ctor, to avoid having a member variable
and its accessor having different values.

[garykac, 2015-02-03 21:40:52]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2015-02-03 21:42:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/895523004/40001

[commit-bot: I haz the power, 2015-02-03 21:45:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-03 21:46:59]

Patchset 3 (id:??) landed as
https://crrev.com/12564e898915dd71a082b1082d72a5f9521e778c
Cr-Commit-Position: refs/heads/master@{#314408}