Make the chevron menu button responsible for legacy overflow menu logic

Make the chevron menu button responsible for legacy overflow menu logic

Move the ChevronMenuButton class into its own file, and move the logic to show
the legacy (drop-down) extension action overflow menu from the
BrowserActionsContainer into there.

This is mainly because:
1. We'll eventually be getting rid of this legacy behavior, and this way, the
   removal is all concentrated in one area (ChevronMenuButton).
2. BrowserActionsContainer is already doing a ton (since it has both new
   overflow and normal logic). It doesn't need this, too.

As a bonus, refactored the lifetime of the overflow menu to be more
deterministic, and got rid of the back-and-forth between it and the
BrowserActionsContainer.

And all with a negative line count! Woohoo!

BUG=417085
Committed: https://crrev.com/4efc003eb10aba8c8189d831ec8825eb022c5b2c
Cr-Commit-Position: refs/heads/master@{#297240}

[Devlin, 2014-09-23 23:59:22]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-09-24 18:36:44]

Patchset #2 (id:40001) has been deleted

[Devlin, 2014-09-24 18:36:48]

Patchset #1 (id:20001) has been deleted

[Devlin, 2014-09-24 18:39:48]

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

[Devlin, 2014-09-24 18:39:49]

Finnur and Peter, mind taking a look?

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

https://codereview.chromium.org/597783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/chevron_menu_button.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Much of this file is similar to browser_action_overflow_menu_controller.cc.  Out
of consideration for the reviewers, patch set 1 has the file with the original
name, so the changes are easier to see.  The only difference between patch set 1
and patch set 2 is the renaming (but don't expect Rietveld to believe that).

[Peter Kasting, 2014-09-25 01:06:43]

I didn't do a super-detailed review.

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:83:
// Overridden from views::MenuDelegate:
Nit: "Overridden from" not necessary

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:92:
// menu.
So, MenuButton and MenuDelegate both have these drag-and-drop methods?  Is there
a way to just use one set?  It looks below as if we basically implement all
these in both classes by forwarding to BrowserActionDragData, it would be nice
to save on some boilerplate.

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

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:128:
chevron_->set_owned_by_client();
Why do we want to own this ourselves instead of letting the view hierarchy own
it like is typical, and just keep a raw pointer to it?

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:261: return
chevron_.get() ?
Nit: Don't use get() when using a scoped_ptr<> as a bool (it's unnecessary)
(multiple places)

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:558: DCHECK(false);
Why are we even overriding this method, then?

[Finnur, 2014-09-25 09:58:49]

Peter beat me to all of my comments, except one (see below).

LGTM, with Peter's concerns addressed.

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

https://codereview.chromium.org/597783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/chevron_menu_button.cc:68: // drag and drop,
etc). This class manages its own lifetime.
Oh yeah? Why is it a scoped_ptr then? :)
This looks like a copy-paste error...

[Finnur, 2014-09-25 10:00:52]

And by the way... re-factoring code before deletion? Seriously, I need to know
what you are having for breakfast, because I clearly need the same. :)

And don't say you eat breakfast at the Google cafeteria because that would just
be rubbing it in! ;)

[Devlin, 2014-09-25 15:56:50]

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:83:
// Overridden from views::MenuDelegate:
On 2014/09/25 01:06:43, Peter Kasting wrote:
> Nit: "Overridden from" not necessary

Dang, caught it on line 49 but not here.  Removed.

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:92:
// menu.
On 2014/09/25 01:06:43, Peter Kasting wrote:
> So, MenuButton and MenuDelegate both have these drag-and-drop methods?  Is
there
> a way to just use one set?  It looks below as if we basically implement all
> these in both classes by forwarding to BrowserActionDragData, it would be nice
> to save on some boilerplate.

Unfortunately, there really isn't a good way.  The methods on MenuButton are
inherited from View, whereas those on MenuDelegate aren't inherited, but are
called from MenuController when the menu is run.  And we really do need to
inherit both separately, since both the chevron button and the drop down menu
need to respond to drag/drop events.  We could potentially have a third class
(e.g. "BrowserActionDropController") that each of these classes own and call
into, but that only saves us on the implementation of these methods, not the
declaration.  Since the methods are already basically just forwarded to
BrowserActionDragData, that doesn't really help.

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

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:128:
chevron_->set_owned_by_client();
On 2014/09/25 01:06:43, Peter Kasting wrote:
> Why do we want to own this ourselves instead of letting the view hierarchy own
> it like is typical, and just keep a raw pointer to it?

ChevronMenuButton takes (and uses) a raw pointer to |this|
BrowserActionsContainer, so we need to make sure that |this| outlives the
button.  Admittedly, since this object would own the view as a child, that would
be the case with the current view hierarchy, but I think this way it's a more
explicit (and less fragile if for some reason we move the chevron somewhere). 
For clarity, I think I'd prefer to keep it like this (it certainly doesn't harm
it), but if you'd prefer me to remove it, I'm happy to do so. :)

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:261: return
chevron_.get() ?
On 2014/09/25 01:06:43, Peter Kasting wrote:
> Nit: Don't use get() when using a scoped_ptr<> as a bool (it's unnecessary)
> (multiple places)

For some reason, I thought we were getting rid of this, but apparently not. 
(Un)done.

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:558: DCHECK(false);
On 2014/09/25 01:06:43, Peter Kasting wrote:
> Why are we even overriding this method, then?

Whoops!  Missed that one in the cleanup (had it as DCHECK false during testing).
 Thanks :)

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

https://codereview.chromium.org/597783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/chevron_menu_button.cc:68: // drag and drop,
etc). This class manages its own lifetime.
On 2014/09/25 09:58:49, Finnur wrote:
> Oh yeah? Why is it a scoped_ptr then? :)
> This looks like a copy-paste error...

Whoops, done.

[Devlin, 2014-09-25 16:00:37]

On 2014/09/25 10:00:52, Finnur wrote:
> And by the way... re-factoring code before deletion?
This was more about cleaning up BrowserActionsContainer than the chevron.  BAC
had (and still has) a bunch of stuff going on in it, and inherited from a
whopping 8 different classes (now down to 6).  I felt like we should trim the
fat a bit. :)  That, and I disliked the fact that
BrowserActionOverflowMenuController managed its own lifetime while referencing
BrowserActionsContainer (which had caused bugs in the past).

> Seriously, I need to know
> what you are having for breakfast, because I clearly need the same. :)
> 
> And don't say you eat breakfast at the Google cafeteria because that would
just
> be rubbing it in! ;)

Haha nope, nothing like a nice leisurely breakfast at home. :)

[Peter Kasting, 2014-09-25 19:06:18]

Finnur's sign off is sufficient to land this.

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

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:128:
chevron_->set_owned_by_client();
On 2014/09/25 15:56:49, Devlin wrote:
> On 2014/09/25 01:06:43, Peter Kasting wrote:
> > Why do we want to own this ourselves instead of letting the view hierarchy
own
> > it like is typical, and just keep a raw pointer to it?
> 
> ChevronMenuButton takes (and uses) a raw pointer to |this|
> BrowserActionsContainer, so we need to make sure that |this| outlives the
> button.  Admittedly, since this object would own the view as a child, that
would
> be the case with the current view hierarchy, but I think this way it's a more
> explicit (and less fragile if for some reason we move the chevron somewhere). 
> For clarity, I think I'd prefer to keep it like this (it certainly doesn't
harm
> it), but if you'd prefer me to remove it, I'm happy to do so. :)

Hmm.  I see what you mean.

I think I probably prefer the view-hierarchy-owns-the-objects route in the end
just because it's more consistent with how we'd do this elsewhere, but I think
it would be fine to put comments in either or both of these classes at some
appropriate place noting this lifetime requirement.  For example, a comment here
could be:

      // Since the ChevronMenuButton holds a raw pointer to us, we need to
      // ensure it doesn't outlive us.  Having it owned by the view hierarchy as
      // a child will suffice.
      chevron_.reset(new ChevronMenuButton(this));

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:261: return
chevron_.get() ?
On 2014/09/25 15:56:49, Devlin wrote:
> On 2014/09/25 01:06:43, Peter Kasting wrote:
> > Nit: Don't use get() when using a scoped_ptr<> as a bool (it's unnecessary)
> > (multiple places)
> 
> For some reason, I thought we were getting rid of this, but apparently not. 
> (Un)done.

We're getting rid of implicit conversion of scoped_refptr<> to something (raw
pointer?), maybe that's what you're thinking of.

[Devlin, 2014-09-25 20:04:10]

I think I actually do need your stamp on this Peter (for
c/b/ui/views/toolbar/*).

Thanks!

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

https://codereview.chromium.org/597783002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:128:
chevron_->set_owned_by_client();
On 2014/09/25 19:06:18, Peter Kasting wrote:
> On 2014/09/25 15:56:49, Devlin wrote:
> > On 2014/09/25 01:06:43, Peter Kasting wrote:
> > > Why do we want to own this ourselves instead of letting the view hierarchy
> own
> > > it like is typical, and just keep a raw pointer to it?
> > 
> > ChevronMenuButton takes (and uses) a raw pointer to |this|
> > BrowserActionsContainer, so we need to make sure that |this| outlives the
> > button.  Admittedly, since this object would own the view as a child, that
> would
> > be the case with the current view hierarchy, but I think this way it's a
more
> > explicit (and less fragile if for some reason we move the chevron
somewhere). 
> > For clarity, I think I'd prefer to keep it like this (it certainly doesn't
> harm
> > it), but if you'd prefer me to remove it, I'm happy to do so. :)
> 
> Hmm.  I see what you mean.
> 
> I think I probably prefer the view-hierarchy-owns-the-objects route in the end
> just because it's more consistent with how we'd do this elsewhere, but I think
> it would be fine to put comments in either or both of these classes at some
> appropriate place noting this lifetime requirement.  For example, a comment
here
> could be:
> 
>       // Since the ChevronMenuButton holds a raw pointer to us, we need to
>       // ensure it doesn't outlive us.  Having it owned by the view hierarchy
as
>       // a child will suffice.
>       chevron_.reset(new ChevronMenuButton(this));

Done.

[Peter Kasting, 2014-09-25 20:42:41]

LGTM once you fix the below.

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

https://codereview.chromium.org/597783002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:130:
chevron_.reset(new ChevronMenuButton(this));
This needs to not be a scoped_ptr now, since otherwise you have
double-ownership.

[Devlin, 2014-09-25 20:47:13]

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

https://codereview.chromium.org/597783002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:130:
chevron_.reset(new ChevronMenuButton(this));
On 2014/09/25 20:42:41, Peter Kasting wrote:
> This needs to not be a scoped_ptr now, since otherwise you have
> double-ownership.

Done.

[Devlin, 2014-09-25 22:19:14]

On 2014/09/25 20:47:13, Devlin wrote:
>
https://codereview.chromium.org/597783002/diff/120001/chrome/browser/ui/views...
> File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):
> 
>
https://codereview.chromium.org/597783002/diff/120001/chrome/browser/ui/views...
> chrome/browser/ui/views/toolbar/browser_actions_container.cc:130:
> chevron_.reset(new ChevronMenuButton(this));
> On 2014/09/25 20:42:41, Peter Kasting wrote:
> > This needs to not be a scoped_ptr now, since otherwise you have
> > double-ownership.
> 
> Done.

PSA: Due to the fact that branch is tomorrow, and other people are panicking
with stuff that absolutely-must-go-in-at-all-costs, this patch will be committed
after Friday EOD.

[Devlin, 2014-09-29 16:19:55]

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

[commit-bot: I haz the power, 2014-09-29 16:20:56]

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

[commit-bot: I haz the power, 2014-09-29 17:33:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-29 17:33:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2014-09-29 18:47:57]

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

[commit-bot: I haz the power, 2014-09-29 18:50:13]

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

[commit-bot: I haz the power, 2014-09-29 19:47:05]

Committed patchset #7 (id:180001) as ae1f3dd7bbccbb96403a525a4740f9151da99819

[commit-bot: I haz the power, 2014-09-29 19:48:38]

Patchset 7 (id:??) landed as
https://crrev.com/4efc003eb10aba8c8189d831ec8825eb022c5b2c
Cr-Commit-Position: refs/heads/master@{#297240}