Make hiding an extension action cause it to go to the overflow menu

Make hiding an extension action cause it to go to the overflow menu

With the --extension-action-redesign switch, hiding an extension action should
cause it to go to the overflow menu (rather than disappearing completely).
Enable this functionality.

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

[Devlin, 2014-08-19 00:35:29]

When you have a chance. :)

[Finnur, 2014-08-19 10:18:04]

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_context_menu_model.cc (right):

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_context_menu_model.cc:74: int
GetVisibilityStringId(Profile* profile, const Extension* extension) {
nit: const Profile&

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:501:
registrar_.RemoveAll();  // Cheat since we only have one notification.
Seems like you aren't saving much with this. Why not explicitly remove it and
avoid potential bugs in the future?

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model_unittest.cc (right):

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model_unittest.cc:726: // Reseting
the visibility to 'true' should result in the extension being
nit: Two t's in Resetting. Same below.

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model_unittest.cc:758: EXPECT_EQ(-1,
toolbar_model()->GetVisibleIconCount());  // -1 = 'all'
nit: End comments with period.

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model_unittest.cc:784: // Removing a
second extension should result in it being sent to the overflow
'Removing' is misleading (you're not removing the extension).
'Hiding'?

[Devlin, 2014-08-19 16:54:40]

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_context_menu_model.cc (right):

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_context_menu_model.cc:74: int
GetVisibilityStringId(Profile* profile, const Extension* extension) {
On 2014/08/19 10:18:04, Finnur wrote:
> nit: const Profile&

Sadly... can't.

ExtensionActionAPI takes a non-const BrowserContext* for Get(), which hooks into
the whole BrowserContextKeyedService implementation (as does ExtensionPrefs). 
Could pass in a non-const Profile&, but we really don't do that much without
compelling reason.

But added in a DCHECK at least, for good measure.

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:501:
registrar_.RemoveAll();  // Cheat since we only have one notification.
On 2014/08/19 10:18:04, Finnur wrote:
> Seems like you aren't saving much with this. Why not explicitly remove it and
> avoid potential bugs in the future?

sigh... probably for the best.  But typing
NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY three times in ten lines just
hurts my fingers. :P

Just more incentive to make it an observer method instead... but that's a CL for
another time.

Done.

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model_unittest.cc (right):

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model_unittest.cc:726: // Reseting
the visibility to 'true' should result in the extension being
On 2014/08/19 10:18:04, Finnur wrote:
> nit: Two t's in Resetting. Same below.

Whoops!  Done.

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model_unittest.cc:758: EXPECT_EQ(-1,
toolbar_model()->GetVisibleIconCount());  // -1 = 'all'
On 2014/08/19 10:18:04, Finnur wrote:
> nit: End comments with period.

Done.

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model_unittest.cc:784: // Removing a
second extension should result in it being sent to the overflow
On 2014/08/19 10:18:04, Finnur wrote:
> 'Removing' is misleading (you're not removing the extension).
> 'Hiding'?

Good point.  Done.

[Finnur, 2014-08-20 12:06:16]

LGTM, with one question (and a besserwisser comment). :)

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:501:
registrar_.RemoveAll();  // Cheat since we only have one notification.
> just hurts my fingers. :P

That's why they invented the clipboard! ;)

https://codereview.chromium.org/476873002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/476873002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:505: registered = true;
This is a little confusing, given that you remove the registration next. How
about |was_registered|?

[Devlin, 2014-08-20 15:31:23]

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/476873002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:501:
registrar_.RemoveAll();  // Cheat since we only have one notification.
On 2014/08/20 12:06:16, Finnur wrote:
> > just hurts my fingers. :P
> 
> That's why they invented the clipboard! ;)

:P

https://codereview.chromium.org/476873002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/476873002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:505: registered = true;
On 2014/08/20 12:06:16, Finnur wrote:
> This is a little confusing, given that you remove the registration next. How
> about |was_registered|?

Done.

[Devlin, 2014-08-20 15:31:55]

+sky for views (just a couple of lines)

[sky, 2014-08-20 15:58:11]

LGTM

[Devlin, 2014-08-21 00:06:30]

Finnur, mind taking a quick look at the delta?

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (left):

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:1037:
model_->SetVisibleIconCount(num_visible_icons);
Had to move this to OnResize because otherwise the overflow container was trying
to set the model size.  But I don't think we should do this any other time
(besides OnResize), right?  Animate() is also called from ExtensionAdded/Removed
and highlight mode changed, but in all those cases, the model triggers the
change, so it's already up-to-date.

Sound right?

[Finnur, 2014-08-21 10:47:51]

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:864:
Animate(gfx::Tween::EASE_OUT, browser_action_views_.size());
I can't say for sure if this is correct. I suspect it is not (and same goes for
ToolbarExtensionAdded). Remember that extensions can be added and removed in the
background and we need to think about what happens if, e.g. an extension is
killed by blacklisting just moments before the user shuts down the browser. Will
the container have time to save what the right value should be before shutting
down? If not, won't the container on next startup have (in this case) room for
one more extension than intended and one fewer in the ExtensionAdded function?

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:918:
Animate(gfx::Tween::EASE_OUT, GetIconCount());
I am unclear on this one too...

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:929:
Animate(gfx::Tween::LINEAR, browser_action_views_.size());
This is probably a good change, since if the browser crashes while the
highlighting is up then we won't have saved the visible count while
highlighting.

[Devlin, 2014-08-21 13:56:31]

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:864:
Animate(gfx::Tween::EASE_OUT, browser_action_views_.size());
On 2014/08/21 10:47:51, Finnur wrote:
> I can't say for sure if this is correct. I suspect it is not (and same goes
for
> ToolbarExtensionAdded). Remember that extensions can be added and removed in
the
> background and we need to think about what happens if, e.g. an extension is
> killed by blacklisting just moments before the user shuts down the browser.
Will
> the container have time to save what the right value should be before shutting
> down? If not, won't the container on next startup have (in this case) room for
> one more extension than intended and one fewer in the ExtensionAdded function?

Sorry; I didn't explain very well.

I'm fairly certain that the only time we actually *want* to save the desired
size from the view itself is when the view has been resized.  All these other
times, we actually just pass in the same size as the model already has. 
Specifically:
ToolbarExtensionAdded():
  We add an extension to |browser_action_views_|, increasing the size of the
vector by 1.  If we're not showing all, we're done.  If we are showing all, pass
in the new size (by definition, all) to SaveDesiredSizeAndAnimate().  This means
that we are setting the ToolbarSize to be all visible (which it already is).
ToolbarExtensionRemoved():
  Works very similarly - remove the view (reducing vector size by 1).  If
showing all views, update the size (but this will still show all views).
ToolbarHighlightModeChanged():
  Set the new size to reflect the new current number of visible icons.

In all of these cases, we actually just pass in the "current number of shown
icons" to SaveDesiredSizeAndAnimate(), which would then update the Model.  Of
course, updating from visible size -1 to size -1 doesn't do anything (or from
e.g., 3 to 3 in highlight mode).

More abstractly, ToolbarExtensionAdded/Removed and HighlightModeChanged are all
directly called by the toolbar model, and the model itself has already updated
the visible size.  Calling back into Model to set the visible size is odd at
best, and potentially buggy at worst (if somehow overflow mode highlighted, we
would have a bug with the overflow mode setting the size in the model).

I think the reason we previously did this is because we used to have the
different pref for height, which was actually set by the view itself (and the
model didn't store the pref), meaning that this would be necessary.  Since the
model now controls it, we don't need to in these cases.

Okay, lengthy explanation over with.  Does it make sense?  Are there points I'm
missing?

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:918:
Animate(gfx::Tween::EASE_OUT, GetIconCount());
On 2014/08/21 10:47:51, Finnur wrote:
> I am unclear on this one too...

Here, we just say "Animate to the number of icons that should be visible
according to the model."  Thinking about this carefully, it will actually be
wrong in incognito mode - which means that GetIconCount should probably discount
any non-incognito extensions while in incognito mode.

[Finnur, 2014-08-21 14:08:30]

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:864:
Animate(gfx::Tween::EASE_OUT, browser_action_views_.size());
No, I would agree that there's no need to save the same value over and over
again. 

> we used to have the different pref for height

Do you mean |width|?

[Devlin, 2014-08-21 15:24:59]

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/476873002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:864:
Animate(gfx::Tween::EASE_OUT, browser_action_views_.size());
On 2014/08/21 14:08:30, Finnur wrote:
> No, I would agree that there's no need to save the same value over and over
> again. 
> 
> > we used to have the different pref for height
> 
> Do you mean |width|?

Ah yes, sorry, width. :) (that should teach me to try and make sense early in
the morning :P)

[Finnur, 2014-08-21 15:41:18]

OK, LGTM. :)

[Devlin, 2014-08-21 16:25:39]

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

[commit-bot: I haz the power, 2014-08-21 16:27:31]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/476873002/...

[Devlin, 2014-08-21 17:40:19]

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

[Devlin, 2014-08-21 17:43:41]

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

[commit-bot: I haz the power, 2014-08-21 17:46:03]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/476873002/...

[commit-bot: I haz the power, 2014-08-21 20:44:14]

Committed patchset #8 (240001) as 291174