Open the WrenchMenu on mouseover when dragging a browser action

Open the WrenchMenu on mouseover when dragging a browser action

Enable dragging from the main container to the overflow container by opening the
wrench menu when hovering over the button while dragging a browser action. This
also requires keeping the menu open while the drag is happening.

Note: this doesn't fix drag calculations for the overflow menu.
In an effort to keep these patches small, that will come later.

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

[Devlin, 2014-07-17 19:45:15]

Finnur, please take a look when you have a chance.

[dcheng, 2014-07-17 19:58:03]

Some drive by comments.

https://codereview.chromium.org/399143004/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/browser_action_drag_data.cc (right):

https://codereview.chromium.org/399143004/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_drag_data.cc:16: const char*
kClipboardFormatString = "chromium/x-browser-actions";
Minor nit: const char kClipboardFormatString[]

https://codereview.chromium.org/399143004/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_drag_data.cc:30: int* formats,
std::set<ui::OSExchangeData::CustomFormat>* custom_formats) {
Given that |formats| isn't used here at all, probably no need to pass it atm.

[Devlin, 2014-07-17 20:43:35]

https://codereview.chromium.org/399143004/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/browser_action_drag_data.cc (right):

https://codereview.chromium.org/399143004/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_drag_data.cc:16: const char*
kClipboardFormatString = "chromium/x-browser-actions";
On 2014/07/17 19:58:02, dcheng (OOO) wrote:
> Minor nit: const char kClipboardFormatString[]

Sure, done.

https://codereview.chromium.org/399143004/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_drag_data.cc:30: int* formats,
std::set<ui::OSExchangeData::CustomFormat>* custom_formats) {
On 2014/07/17 19:58:03, dcheng (OOO) wrote:
> Given that |formats| isn't used here at all, probably no need to pass it atm.

Mmkay.  Originally I had it just so it shadowed the View method better, but I
don't feel strongly either way.

[Finnur, 2014-07-18 10:40:40]

> Note: this doesn't fix drag calculations for the overflow menu. 

Does that mean you can drag but the icon will be dropped in the wrong location?
(don't have a problem with that (short term, obviously), just trying to
understand what it means).

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/wrench_menu_model.cc:749: if
(toolbar_model->GetVisibleIconCount() != -1)
Hah... somehow I thought this would be more complicated to figure out! :)

One question: If the count is 0 should we skip adding
IDC_EXTENSIONS_OVERFLOW_MENU?

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_action_view.h (right):

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_action_view.h:61: // wasn't removed
during the drag-and-drop process.
... as in the extension being uninstalled?

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

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:271: //
TODO(devlin) we should see about fixing that.
nit: Missing colon after end-parenthesis.

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/toolbar_view.cc:418: ShowAppMenu(false);  // not
for drop
Nit: Capitalize 'n' and add period.

https://codereview.chromium.org/399143004/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/399143004/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.h:405: // Closes the menu because a drag
left the view.
Drag (noun), pronounced /drag/.
"a boring or tiresome person or thing."

:)

I guess what I'm trying to say is that the wording of this comment is a bit
weird. :) Is the phrase you are looking for 'hover away from'/'hover out' event?

[Finnur, 2014-07-18 10:42:08]

> Does that mean you can drag but the icon will be dropped in the wrong
location?

Oh, I just noticed your other patch. Never mind. :)

[Devlin, 2014-07-18 15:44:32]

On 2014/07/18 10:40:40, Finnur wrote:
> > Note: this doesn't fix drag calculations for the overflow menu. 
> 
> Does that mean you can drag but the icon will be dropped in the wrong
location?
> (don't have a problem with that (short term, obviously), just trying to
> understand what it means).

Yep, after this patch the calculations are still blatantly wrong, so any icon
dragging doesn't really work (even though it almost looks like it does). :)

Other patch (for future/others' reference):
https://codereview.chromium.org/399173004/

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/wrench_menu_model.cc:749: if
(toolbar_model->GetVisibleIconCount() != -1)
On 2014/07/18 10:40:39, Finnur wrote:
> Hah... somehow I thought this would be more complicated to figure out! :)
> 
> One question: If the count is 0 should we skip adding
> IDC_EXTENSIONS_OVERFLOW_MENU?

I'm pretty sure that we *should* add this if the count is 0.  It looks like
GetVisibleIconCount() will return -1 if all extension actions are visible -
including if 0 out of 0 extension actions are visible [1].  Therefore, a count
of zero should mean that 0 are displayed on the main bar, and 1 or more are
overflowed.

Does that sound right?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_action_view.h (right):

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_action_view.h:61: // wasn't removed
during the drag-and-drop process.
On 2014/07/18 10:40:40, Finnur wrote:
> ... as in the extension being uninstalled?

Not quite.  We delete and recreate all the views every time there's a change in
the browser actions order.  Which is kinda excessive (and results in an ugly
"flicker" on repaint), but that's a separate issue to fix. :)  Problem is, the
view is only notified that the drag is done if it wasn't deleted as part of the
drag - which these are if there's actually a movement.

Comment updated to make it more clear.

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

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:271: //
TODO(devlin) we should see about fixing that.
On 2014/07/18 10:40:40, Finnur wrote:
> nit: Missing colon after end-parenthesis. 

Done.

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/toolbar_view.cc:418: ShowAppMenu(false);  // not
for drop
On 2014/07/18 10:40:40, Finnur wrote:
> Nit: Capitalize 'n' and add period.

Done.

https://codereview.chromium.org/399143004/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/399143004/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.h:405: // Closes the menu because a drag
left the view.
On 2014/07/18 10:40:40, Finnur wrote:
> Drag (noun), pronounced /drag/.
> "a boring or tiresome person or thing."
> 
> :)
> 
> I guess what I'm trying to say is that the wording of this comment is a bit
> weird. :) Is the phrase you are looking for 'hover away from'/'hover out'
event?

Haha fair enough. :)  I'd like to stick to drag-and-drop nomenclature for
consistency (I don't see 'hover' anywhere else, and am worried people might
think it was something different), but I tried to clean it up a bit to make it
more clear. :)

[Finnur, 2014-07-18 15:53:51]

LGTM, with nit.

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/399143004/diff/40001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/wrench_menu_model.cc:749: if
(toolbar_model->GetVisibleIconCount() != -1)
Ah, yes.

https://codereview.chromium.org/399143004/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/399143004/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.h:405: // Closes the menu because a drag
left the view.
Yup, what you have now is good.

https://codereview.chromium.org/399143004/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_action_view.h (right):

https://codereview.chromium.org/399143004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_action_view.h:61: // wasn't removed
during the drag-and-drop process (which happens if there
s/which happens if/not when/

Right?

[Devlin, 2014-07-18 18:58:52]

https://codereview.chromium.org/399143004/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_action_view.h (right):

https://codereview.chromium.org/399143004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_action_view.h:61: // wasn't removed
during the drag-and-drop process (which happens if there
On 2014/07/18 15:53:51, Finnur wrote:
> s/which happens if/not when/
> 
> Right?

Yeah, that looks better.  Done.

[Devlin, 2014-07-18 18:59:07]

Scott, mind taking a look?

[sky, 2014-07-18 19:36:54]

This is complex enough that you should add test coverage.

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right):

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:71: // could
break the BrowserActionsContainer's observer list if done here.
observer list handles removal while notifying.

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.h (right):

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.h:44: bool use_new_menu,
I think it's time to convert to a bitmask here.

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.h:55: void CloseMenu();
Instead of the close can you just delete this object?

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:1594:
state_.item->GetDelegate()->InNestedDrag())
This isn't state that's going to change. So it should be passed in rather than
another delegate call.

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner.cc (left):

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner.cc:178: bool has_mnemonics = (types &
MenuRunner::HAS_MNEMONICS) != 0 && !for_drop_;
Why are you removing this?

[Devlin, 2014-07-18 21:38:31]

> This is complex enough that you should add test coverage.

Sure - primarily for the views changes (nested drag staying open), opening the
wrench on drag over, or both?

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right):

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:71: // could
break the BrowserActionsContainer's observer list if done here.
On 2014/07/18 19:36:53, sky wrote:
> observer list handles removal while notifying.

Oh, nifty.  Coulda sworn it didn't...  Done.

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.h (right):

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.h:44: bool use_new_menu,
On 2014/07/18 19:36:54, sky wrote:
> I think it's time to convert to a bitmask here.

Done.

https://codereview.chromium.org/399143004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.h:55: void CloseMenu();
On 2014/07/18 19:36:54, sky wrote:
> Instead of the close can you just delete this object?

Nope - doing so causes a segfault.

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:1594:
state_.item->GetDelegate()->InNestedDrag())
On 2014/07/18 19:36:54, sky wrote:
> This isn't state that's going to change. So it should be passed in rather than
> another delegate call.

Sorry, not sure I follow.  Passed in to where?

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner.cc (left):

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner.cc:178: bool has_mnemonics = (types &
MenuRunner::HAS_MNEMONICS) != 0 && !for_drop_;
On 2014/07/18 19:36:54, sky wrote:
> Why are you removing this?

(Please double check me on this)

If the menu has mnemonics, we hide the prefix unless we specifically want to
show it. [1]  (Showing the prefix results in the ampersand being visible, e.g.
"New &Tab".)  Since the menu opening for a drop doesn't (so far as I can tell)
affect whether or not the menu actually has mnemonics, this check results in the
mnemonics being shown during a drag-and-drop session, despite show_mnemonics
always being false.

Is there a better way to fix this?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

[sky, 2014-07-18 22:45:40]

Ideally tests to cover it all.

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:1594:
state_.item->GetDelegate()->InNestedDrag())
On 2014/07/18 21:38:31, Devlin wrote:
> On 2014/07/18 19:36:54, sky wrote:
> > This isn't state that's going to change. So it should be passed in rather
than
> > another delegate call.
> 
> Sorry, not sure I follow.  Passed in to where?

Into MenuRunner when you Run the menu. Basically I'm saying add another param to
MenuRunner::Run().

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner.cc (left):

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner.cc:178: bool has_mnemonics = (types &
MenuRunner::HAS_MNEMONICS) != 0 && !for_drop_;
On 2014/07/18 21:38:31, Devlin wrote:
> On 2014/07/18 19:36:54, sky wrote:
> > Why are you removing this?
> 
> (Please double check me on this)
> 
> If the menu has mnemonics, we hide the prefix unless we specifically want to
> show it. [1]  (Showing the prefix results in the ampersand being visible, e.g.
> "New &Tab".)  Since the menu opening for a drop doesn't (so far as I can tell)
> affect whether or not the menu actually has mnemonics, this check results in
the
> mnemonics being shown during a drag-and-drop session, despite show_mnemonics
> always being false.
> 
> Is there a better way to fix this?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

Ok, that makes sense.

[Devlin, 2014-07-21 20:00:56]

I think this is everything.  toolbar_view_interactive_uitest.cc may change
slightly (need to test on Windows), but should be fairly stable.  Everything
else should be done.

Cheers!

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/399143004/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:1594:
state_.item->GetDelegate()->InNestedDrag())
On 2014/07/18 22:45:39, sky wrote:
> On 2014/07/18 21:38:31, Devlin wrote:
> > On 2014/07/18 19:36:54, sky wrote:
> > > This isn't state that's going to change. So it should be passed in rather
> than
> > > another delegate call.
> > 
> > Sorry, not sure I follow.  Passed in to where?
> 
> Into MenuRunner when you Run the menu. Basically I'm saying add another param
to
> MenuRunner::Run().

Done.

[sky, 2014-07-21 22:31:42]

LGTM

[Devlin, 2014-07-23 18:34:57]

On 2014/07/21 22:31:42, sky wrote:
> LGTM

FYI, fully done with toolbar_view_interactive_uitest.cc with passing win bots,
if you wanted to take another look (no need to, just if you want, as it still
does the same process - drags a browser action to the wrench and makes sure it
opens).

[sky, 2014-07-23 19:13:48]

SLGTM

[Devlin, 2014-07-23 19:55:24]

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

[commit-bot: I haz the power, 2014-07-23 19:57:57]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-07-24 03:31:35]

Change committed as 285142