[Extensions Toolbar] Have all extensions in the toolbar show a context menu

[Extensions Toolbar] Have all extensions in the toolbar show a context menu

Previously, we never showed context menus for component extensions, but this
just looks weird when it's in the browser actions toolbar. Give component
extensions a context menu if the redesign is on.

BUG=428996
Committed: https://crrev.com/fcc1e54ddde55b158cc7addd46d1cd59b2bce577
Cr-Commit-Position: refs/heads/master@{#316617}

[Devlin, 2015-02-12 19:11:08]

rdevlin.cronin@chromium.org changed reviewers:
+ finnur@chromium.org

[Finnur, 2015-02-13 14:49:59]

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension.cc
File extensions/common/extension.cc (right):

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension....
extensions/common/extension.cc:283:
!FeatureSwitch::extension_action_redesign()->IsEnabled();
But what does the menu show for component extensions?

Does it suggest the user can uninstall a component extension?
Do the basic component extensions have meaningful names?
Can it at any point in time lead the user to a 404 on the webstore (by clicking
on the name)?
Does the Inspect Popup entry work?
Does the Options entry work?

[Devlin, 2015-02-13 18:06:54]

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension.cc
File extensions/common/extension.cc (right):

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension....
extensions/common/extension.cc:283:
!FeatureSwitch::extension_action_redesign()->IsEnabled();
On 2015/02/13 14:49:59, Finnur wrote:
> But what does the menu show for component extensions?
> 
> Does it suggest the user can uninstall a component extension?
> Do the basic component extensions have meaningful names?
> Can it at any point in time lead the user to a 404 on the webstore (by
clicking
> on the name)?
> Does the Inspect Popup entry work?
> Does the Options entry work?

Good questions, all!

So, first off, the only component extension that has an action is Stars (we
won't, by default, show component extensions in the toolbar, because unlike
other extensions with no actions, they're basically an implementation detail of
chrome).

> Does it suggest the user can uninstall a component extension?
No.  Just like policy-enabled extensions, the "Remove from Chrome" option will
be disabled, because the user can't modify the settings. That said, this one
probably warrants a unit test.  Added.

> Do the basic component extensions have meaningful names?
Right now, this is just stars - which I think has the official name of "Bookmark
Manager" (which is meaningful, I think).

> Can it at any point in time lead the user to a 404 on the webstore (by
clicking
> on the name)?
We link to the extension's "homepage" by clicking on the name. If the extension
doesn't specify a (valid) homepage in the manifest, we default to the store, if
it updates via the gallery.  If it doesn't, then we don't link to anything (this
happens for, e.g., unpacked extensions).  This is actually an interesting
question for Stars, which *does* have a webstore extension, but I'm not sure if
the one uploaded in the webstore and the default-component one are the same...
in any case, we should never 404.

> Does the Inspect Popup entry work?
Yep (as long as it has a popup).

> Does the Options entry work?
Yep (as long as it has an options page).

[Finnur, 2015-02-16 11:39:51]

LGTM, just nits below.

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension.cc
File extensions/common/extension.cc (right):

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension....
extensions/common/extension.cc:283:
!FeatureSwitch::extension_action_redesign()->IsEnabled();
Hah! I made the same mistake you did when I reviewed this and thought it was
correct. 

However, isn't this a bit more readable?

// Normally we don't show a context menu for component actions, but when
// re-design is enabled we show them in the toolbar at times, and it is
// weird to have a random button that has no context menu when the rest do.
if (location() == Manifest::COMPONENT ||
    location() == Manifest::EXTERNAL_COMPONENT)
  return FeatureSwitch::extension_action_redesign()->IsEnabled();

return true;

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension....
extensions/common/extension.cc:283:
!FeatureSwitch::extension_action_redesign()->IsEnabled();
> but I'm not sure if the one uploaded in the webstore and the default-component
one are the same...

I'd be surprised if they are different, but in any case -- this link is just to
learn more about it, and as long as the description (and all that) still
applies, it wouldn't be a huge deal if there is a mismatch in what's on the
store vs. locally.

https://codereview.chromium.org/919733002/diff/20001/extensions/common/extens...
File extensions/common/extension.cc (right):

https://codereview.chromium.org/919733002/diff/20001/extensions/common/extens...
extensions/common/extension.cc:277: // extension with options. All other menu
items like uninstall have
This comment is now out of date (no component extension with options). But see
below (to the left).

[Devlin, 2015-02-17 18:03:04]

New patchsets have been uploaded after l-g-t-m from finnur@chromium.org

[Devlin, 2015-02-17 18:40:41]

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension.cc
File extensions/common/extension.cc (right):

https://codereview.chromium.org/919733002/diff/1/extensions/common/extension....
extensions/common/extension.cc:283:
!FeatureSwitch::extension_action_redesign()->IsEnabled();
On 2015/02/16 11:39:51, Finnur wrote:
> Hah! I made the same mistake you did when I reviewed this and thought it was
> correct. 
> 
> However, isn't this a bit more readable?
> 
> // Normally we don't show a context menu for component actions, but when
> // re-design is enabled we show them in the toolbar at times, and it is
> // weird to have a random button that has no context menu when the rest do.
> if (location() == Manifest::COMPONENT ||
>     location() == Manifest::EXTERNAL_COMPONENT)
>   return FeatureSwitch::extension_action_redesign()->IsEnabled();
> 
> return true;

Yes, that is more readable. :)  Done.

https://codereview.chromium.org/919733002/diff/20001/extensions/common/extens...
File extensions/common/extension.cc (right):

https://codereview.chromium.org/919733002/diff/20001/extensions/common/extens...
extensions/common/extension.cc:277: // extension with options. All other menu
items like uninstall have
On 2015/02/16 11:39:51, Finnur wrote:
> This comment is now out of date (no component extension with options). But see
> below (to the left).

Acknowledged.

[Devlin, 2015-02-17 18:40:45]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-02-17 18:42:02]

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

[commit-bot: I haz the power, 2015-02-17 18:59:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-17 19:00:33]

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