Enabling Commands API for Apps

Enabling Commands API for Apps (on dev)

BUG=302437
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233310

[Yoyo Zhou, 2013-10-25 21:28:07]

I talked to kalman about this. It looks like there are certain assumptions in
ComplexFeature that would be incorrect for this use case, but luckily they won't
matter.

[Finnur, 2013-10-26 18:05:22]

Are you saying the CHECK is wrong and a CL for augmenting it is on the way?

[Finnur, 2013-10-28 13:43:24]

Update, I think I got it to work (see _manifest_features.json).

[Finnur, 2013-10-28 13:44:46]

No, wait. Upon further review I think this isn't quite what I want.

[Finnur, 2013-10-28 14:03:47]

OK, now I think I've got it correctly (patch set 3) as this seems to work ---
don't get the CHECK anymore.

To test, I tried hardcoding GetChannel to return CHANNEL_STABLE and saw that it
returned an error for Apps, which was expected. 

And keyboard shortcuts now work for Apps on Dev, both local and global.

Yoz, mind reviewing?

[Finnur, 2013-10-28 14:04:12]

On 2013/10/28 14:03:47, Finnur wrote:
> OK, now I think I've got it correctly (patch set 3) as this seems to work ---
> don't get the CHECK anymore.
> 
> To test, I tried hardcoding GetChannel to return CHANNEL_STABLE and saw that
it
> returned an error for Apps, which was expected. 
> 
> And keyboard shortcuts now work for Apps on Dev, both local and global.
> 
> Yoz, mind reviewing?

That was supposed to read: "Yoyo, mind reviewing"? :)

[not at google - send to devlin, 2013-10-28 16:17:21]

I'd feel better about this if there were a test for having different extension
types on different channels.

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
chrome/common/extensions/api/_manifest_features.json:85: "extension_types":
["extension", "platform_app"],
You shouldn't need the "extension" here. These lists of features are an OR op,
whereas each rule within a feature is an AND.

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
chrome/common/extensions/api/_manifest_features.json:101:
"min_manifest_version": 2
So you need this rule to balance the one above for some reason?

[Finnur, 2013-10-28 16:34:23]

> I'd feel better about this if there were a test for 
> having different extension types on different channels.

We kind of have a belts and suspenders approach at the moment, because the
feature is guarded by a command line arg and also by a channel check. :)

But I don't mind adding a test for this. I'm trying to remember if we have tests
that affect which channel gets reported, do you know?

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
chrome/common/extensions/api/_manifest_features.json:85: "extension_types":
["extension", "platform_app"],
Changed.

To be clear -- because "extension" features on "stable", I don't need to specify
it also for "dev"?

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
chrome/common/extensions/api/_manifest_features.json:101:
"min_manifest_version": 2
Yes. This makes the CHECK go away.

[not at google - send to devlin, 2013-10-28 16:41:32]

You can fake the channel with:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
chrome/common/extensions/api/_manifest_features.json:85: "extension_types":
["extension", "platform_app"],
On 2013/10/28 16:34:24, Finnur wrote:
> Changed.
> 
> To be clear -- because "extension" features on "stable", I don't need to
specify
> it also for "dev"? 

Yes exactly.

https://codereview.chromium.org/44553002/diff/130001/chrome/common/extensions...
chrome/common/extensions/api/_manifest_features.json:101:
"min_manifest_version": 2
On 2013/10/28 16:34:24, Finnur wrote:
> Yes. This makes the CHECK go away.

Huh. Which CHECK?

[Finnur, 2013-10-28 16:52:40]

On 2013/10/28 16:41:32, kalman wrote:
> You can fake the channel with:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

Neat!

> > Yes. This makes the CHECK go away.
> 
> Huh. Which CHECK?

The one I've been talking to Yoyo about, specifically:
  CHECK(root.GetDictionaryWithoutPathExpansion(parent_name, &parent));
... in the ctor of BaseFeatureProvider.

[not at google - send to devlin, 2013-10-28 16:54:40]

On 2013/10/28 16:52:40, Finnur wrote:
> On 2013/10/28 16:41:32, kalman wrote:
> > You can fake the channel with:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> 
> Neat!
> 
> > > Yes. This makes the CHECK go away.
> > 
> > Huh. Which CHECK?
> 
> The one I've been talking to Yoyo about, specifically:
>   CHECK(root.GetDictionaryWithoutPathExpansion(parent_name, &parent));
> ... in the ctor of BaseFeatureProvider.

Ah. Ok. Well I think a neater hack for that would be to just put a list around
the single object. I.e.

"commands.global": {
  ...
}

--->

"commands.global": [{
  ...
}]

but that sounds like a real bug with the way feature parsing works.

[not at google - send to devlin, 2013-10-28 16:54:55]

+justinlin FYI :)

[justinlin, 2013-10-28 19:30:55]

Filed for the CHECK problem: https://crbug.com/312385

[Finnur, 2013-10-29 16:29:14]

I've been banging my head against the rock today, trying to debug why the tests
I'm adding continue to fail.

I would appreciate a look, if you could (justinlin), to make sure I'm specifying
the permissions correctly in _manifest_features.json (my code in this CL) and
that the _manifest_features.json parser works as expected (not my code). The
intent of what I'm trying to do with _manifest_features.json is documented in
the unit test (see comments).

Basically, it fails on the three LoadAndExpectError, so the APIs are not
restricted to dev. :/

[Finnur, 2013-10-29 16:31:46]

@justinlin:

> The intent of what I'm trying to do with _manifest_features.json is documented
in
> the unit test (see comments).

And to clarify: 
Ext+Command+Global
... means "Extension that specifies a command and includes a command that is
global".
... which corresponds to the two clauses in the json file:

"commands"
and 
"commands.global"

[justinlin, 2013-10-29 17:39:55]

Sure, happy to try help. The manifest.json looks fine to me (although nit: I
would indent it with the curly braces on the previous line).

I took a quick look, and I think there's 1 test problem and 1 existing problem:
- Using a test extension I tried the commands.global thing, and that seems to
work anyway in Chrome stable. Is that expected? I'm not that familiar with how
this "." stuff is supposed to work in the manifest (I've only used it for API
features).
- You probably want LoadAndExpectWarning. You get a warning if the feature is
not available to the current Chrome channel, not an error.

[Finnur, 2013-10-30 13:58:21]

Thanks a lot for your input, Justin. 

Yup, LoadAndExpectWarning is exactly what I was looking for. Can't believe I was
that close to the solution. :)

> Using a test extension I tried the commands.global thing, and that seems to
> work anyway in Chrome stable. Is that expected?

I had not gotten as far as testing that due to the warning vs error problem.
But, my expectations (when I changed that file) were that "commands.global"
should not work on stable because only "dev" is specified in the json clause for
"commands.global". But I guess it works on stable because we've already given
stable access to "commands" (thinking out loud).

I have therefore made sure global is not parsed from the extension manifest and
tested this is the case on stable (commands.global doesn't work).

> The manifest.json looks fine to me (although nit: I
> would indent it with the curly braces on the previous line).

I'm confused... There's no manifest.json file in the CL...

[Yoyo Zhou, 2013-10-30 14:58:10]

On 2013/10/30 13:58:21, Finnur wrote:
> Thanks a lot for your input, Justin. 
> 
> Yup, LoadAndExpectWarning is exactly what I was looking for. Can't believe I
was
> that close to the solution. :)
> 
> > Using a test extension I tried the commands.global thing, and that seems to
> > work anyway in Chrome stable. Is that expected?
> 
> I had not gotten as far as testing that due to the warning vs error problem.
> But, my expectations (when I changed that file) were that "commands.global"
> should not work on stable because only "dev" is specified in the json clause
for
> "commands.global". But I guess it works on stable because we've already given
> stable access to "commands" (thinking out loud).

I'm not clear on the meaning of "noparent", but is it something that would help
here (though I'm not sure if it's possible to use it with complex features)?

> I have therefore made sure global is not parsed from the extension manifest
and
> tested this is the case on stable (commands.global doesn't work).
> 
> > The manifest.json looks fine to me (although nit: I
> > would indent it with the curly braces on the previous line).
> 
> I'm confused... There's no manifest.json file in the CL...

I think he's referring to _manifest_features.json, but the style you used is the
same as the existing style, so I would leave it as is.

[not at google - send to devlin, 2013-10-30 16:51:35]

On 2013/10/30 14:58:10, Yoyo Zhou wrote:
> On 2013/10/30 13:58:21, Finnur wrote:
> > Thanks a lot for your input, Justin. 
> > 
> > Yup, LoadAndExpectWarning is exactly what I was looking for. Can't believe I
> was
> > that close to the solution. :)
> > 
> > > Using a test extension I tried the commands.global thing, and that seems
to
> > > work anyway in Chrome stable. Is that expected?
> > 
> > I had not gotten as far as testing that due to the warning vs error problem.
> > But, my expectations (when I changed that file) were that "commands.global"
> > should not work on stable because only "dev" is specified in the json clause
> for
> > "commands.global". But I guess it works on stable because we've already
given
> > stable access to "commands" (thinking out loud).
> 
> I'm not clear on the meaning of "noparent", but is it something that would
help
> here (though I'm not sure if it's possible to use it with complex features)?
> 
> > I have therefore made sure global is not parsed from the extension manifest
> and
> > tested this is the case on stable (commands.global doesn't work).
> > 
> > > The manifest.json looks fine to me (although nit: I
> > > would indent it with the curly braces on the previous line).
> > 
> > I'm confused... There's no manifest.json file in the CL...
> 
> I think he's referring to _manifest_features.json, but the style you used is
the
> same as the existing style, so I would leave it as is.

noparent means that the properties of 'commands' aren't inherited by
'commands.global'. You probably don't want to set it.

[Yoyo Zhou, 2013-10-30 17:02:48]

https://codereview.chromium.org/44553002/diff/410001/chrome/common/extensions...
File chrome/common/extensions/command.cc (right):

https://codereview.chromium.org/44553002/diff/410001/chrome/common/extensions...
chrome/common/extensions/command.cc:442: chrome::VersionInfo::GetChannel() <=
chrome::VersionInfo::CHANNEL_DEV)
This channel check should not be necessary. It looks like the reason "global" is
still appearing in the dictionary in stable is:

Manifest::GetDictionary("commands") checks that we have access to the "commands"
feature and returns the whole "commands" dictionary, including the "global" key.

Manifest::GetBoolean("commands.global") would check that we have access to the
"commands.global" feature, and return nothing.

You'd have to pass Manifest into this Parse function instead of just the
DictionaryValue for "commands".

[Finnur, 2013-10-31 13:46:33]

I don't think that will work. Bear in mind that a Command can have no
commands.global value in the manifest, but the user can still set that command
to be global in the UI. In that case, the manifest check for commands.global
will be false (which is incorrect).

[Finnur, 2013-11-05 11:00:54]

Yoyo?

What do you think? Good as is?

[Yoyo Zhou, 2013-11-05 20:09:55]

On 2013/11/05 11:00:54, Finnur wrote:
> Yoyo?
> 
> What do you think? Good as is?

Sorry for the latency. LGTM, we can figure out the "correct" way to do this
later.

[commit-bot: I haz the power, 2013-11-06 10:12:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/finnur@chromium.org/44553002/410001

[commit-bot: I haz the power, 2013-11-06 12:47:36]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-06 13:08:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/finnur@chromium.org/44553002/410001

[commit-bot: I haz the power, 2013-11-06 16:05:51]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-06 16:10:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/finnur@chromium.org/44553002/410001

[commit-bot: I haz the power, 2013-11-06 18:50:18]

Change committed as 233310