Allow the user to drag an extension to the overflow menu, even when its empty

Allow the user to drag an extension to the overflow menu, even when its empty

We need to show an empty bar for the user to drag extension actions into for
overflow when there are none currently overflowed. Obviously, this empty bar
should only be displayed when the user is dragging an extension action.

BUG=414386
Committed: https://crrev.com/9bdf45315e4aeae69b66db1ed4bf57fbf129e124
Cr-Commit-Position: refs/heads/master@{#295179}

[Devlin, 2014-09-16 19:25:06]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-09-16 19:28:35]

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:459: // Figure out
where to display the indicator. This is a complex calculation:
The body of this if is just the indented version of 453 - 515.

[Devlin, 2014-09-16 19:28:51]

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

[Devlin, 2014-09-16 19:28:52]

Peter, mind taking a look when you get a chance?

[Peter Kasting, 2014-09-16 21:40:55]

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:275:
model_->SetVisibleIconCount(icon_count - 1);
What if |icon_count| is 0?  Do we really want to call with -1?

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:333: // display,
the container won't be shown.
Nit: Isn't this comment incorrect, in that sometimes we show the container when
there's nothing to display (the point of this change)?

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:340:
DCHECK(!in_overflow_mode());
Nit: I'd remove this DCHECK as it's obviously true given the line just above.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:343: if
(browser_action_views_.size() == 0)
Nit: empty()?

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:552: i +=
main_container_->VisibleBrowserActionsAfterAnimation();
It looks like now |i| is going to be based on the total number of browser
actions in both containers rather than just the number in the overflow
container.  But none of the other ode here changed.  That seems worrisome?

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:735: int
drop_icon_x = ToolbarView::kStandardSpacing +
This seems very different than the old code.  Why is it correct?

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:736:
(drop_position_->icon_in_row * IconWidth(true));
Nit: Previous indenting was correct

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:48: // that will
be visible.
Nit: Say why this matters, since the remainder of this function doesn't care.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:64:
container_->VisibleBrowserActionsAfterAnimation();
Nit: Indent 4, not even

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h (right):

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h:28: // Returns
true the wrench menu should show this view.
Nit: true -> whether

Consider explaining when this returns true, i.e. why this is useful.

[Devlin, 2014-09-16 21:58:30]

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:275:
model_->SetVisibleIconCount(icon_count - 1);
On 2014/09/16 21:40:54, Peter Kasting wrote:
> What if |icon_count| is 0?  Do we really want to call with -1?

Interestingly, VisibleIconCount() *cannot* be 0 here, as this is saying that an
icon was moved from the main bar to overflow.  As such, there must have been at
least one visible icon.  Added a DCHECK and a comment.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:333: // display,
the container won't be shown.
On 2014/09/16 21:40:54, Peter Kasting wrote:
> Nit: Isn't this comment incorrect, in that sometimes we show the container
when
> there's nothing to display (the point of this change)?

Fair - I was counting a "drop area" as "something to be shown".  But
disambiguated it.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:340:
DCHECK(!in_overflow_mode());
On 2014/09/16 21:40:54, Peter Kasting wrote:
> Nit: I'd remove this DCHECK as it's obviously true given the line just above.

Done.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:343: if
(browser_action_views_.size() == 0)
On 2014/09/16 21:40:54, Peter Kasting wrote:
> Nit: empty()?

Done.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:552: i +=
main_container_->VisibleBrowserActionsAfterAnimation();
On 2014/09/16 21:40:54, Peter Kasting wrote:
> It looks like now |i| is going to be based on the total number of browser
> actions in both containers rather than just the number in the overflow
> container.  But none of the other ode here changed.  That seems worrisome?

Line 551 ensures that we're in the overflow container here.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:735: int
drop_icon_x = ToolbarView::kStandardSpacing +
On 2014/09/16 21:40:54, Peter Kasting wrote:
> This seems very different than the old code.  Why is it correct?

Before, we would say "The start of the first visible icon" because, when it was
written, we started the overflow and main containers at different points (IIRC,
overflow container had slightly different padding?).  This has changed, and now
each has only padding (ToolbarView::kStandardSpacing), followed by the icons.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:736:
(drop_position_->icon_in_row * IconWidth(true));
On 2014/09/16 21:40:54, Peter Kasting wrote:
> Nit: Previous indenting was correct

changed back.

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:48: // that will
be visible.
On 2014/09/16 21:40:55, Peter Kasting wrote:
> Nit: Say why this matters, since the remainder of this function doesn't care.

Done.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:64:
container_->VisibleBrowserActionsAfterAnimation();
On 2014/09/16 21:40:54, Peter Kasting wrote:
> Nit: Indent 4, not even

Done.

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h (right):

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h:28: // Returns
true the wrench menu should show this view.
On 2014/09/16 21:40:55, Peter Kasting wrote:
> Nit: true -> whether
> 
> Consider explaining when this returns true, i.e. why this is useful.

Done.

[Peter Kasting, 2014-09-16 22:08:49]

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:552: i +=
main_container_->VisibleBrowserActionsAfterAnimation();
On 2014/09/16 21:58:29, Devlin wrote:
> On 2014/09/16 21:40:54, Peter Kasting wrote:
> > It looks like now |i| is going to be based on the total number of browser
> > actions in both containers rather than just the number in the overflow
> > container.  But none of the other ode here changed.  That seems worrisome?
> 
> Line 551 ensures that we're in the overflow container here.

I know.  I'm saying, the old code here called GetFirstVisibleIconIndex() which
would always return 0.  Now this code calls something which is presumably
guaranteed to be nonzero.  So the meaning of |i| has changed, but none of the
other code here appears to have changed in parallel.

[Devlin, 2014-09-16 22:10:39]

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:552: i +=
main_container_->VisibleBrowserActionsAfterAnimation();
On 2014/09/16 22:08:49, Peter Kasting wrote:
> On 2014/09/16 21:58:29, Devlin wrote:
> > On 2014/09/16 21:40:54, Peter Kasting wrote:
> > > It looks like now |i| is going to be based on the total number of browser
> > > actions in both containers rather than just the number in the overflow
> > > container.  But none of the other ode here changed.  That seems worrisome?
> > 
> > Line 551 ensures that we're in the overflow container here.
> 
> I know.  I'm saying, the old code here called GetFirstVisibleIconIndex() which
> would always return 0.  Now this code calls something which is presumably
> guaranteed to be nonzero.  So the meaning of |i| has changed, but none of the
> other code here appears to have changed in parallel.

GetFirstVisibleIconIndex() would always return
main_container_->VisibleBrowserActionsAfterAnimation() for the overflow
container, and always return 0 for the main container.  Where we call it here is
for the overflow container, so VisibleBrowserActionsAfterAnimation() is correct.

Right?

[Peter Kasting, 2014-09-16 22:15:57]

LGTM

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:552: i +=
main_container_->VisibleBrowserActionsAfterAnimation();
On 2014/09/16 22:10:39, Devlin wrote:
> On 2014/09/16 22:08:49, Peter Kasting wrote:
> > On 2014/09/16 21:58:29, Devlin wrote:
> > > On 2014/09/16 21:40:54, Peter Kasting wrote:
> > > > It looks like now |i| is going to be based on the total number of
browser
> > > > actions in both containers rather than just the number in the overflow
> > > > container.  But none of the other ode here changed.  That seems
worrisome?
> > > 
> > > Line 551 ensures that we're in the overflow container here.
> > 
> > I know.  I'm saying, the old code here called GetFirstVisibleIconIndex()
which
> > would always return 0.  Now this code calls something which is presumably
> > guaranteed to be nonzero.  So the meaning of |i| has changed, but none of
the
> > other code here appears to have changed in parallel.
> 
> GetFirstVisibleIconIndex() would always return
> main_container_->VisibleBrowserActionsAfterAnimation() for the overflow
> container, and always return 0 for the main container.  Where we call it here
is
> for the overflow container, so VisibleBrowserActionsAfterAnimation() is
correct.
> 
> Right?

Oh, I'm dumb, I mentally reversed the contents of GetFirstVisibleIconIndex(). 
Sorry...

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

https://codereview.chromium.org/556293003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:336: // if the
container shouldn't be shown.
Nit: How about something like:

...and always of at least one row, even when we're empty.  The parent may decide
to show us even when empty, e.g. as a drag target for dragging in icons from the
main container.

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

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

https://codereview.chromium.org/556293003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:552: i +=
main_container_->VisibleBrowserActionsAfterAnimation();
On 2014/09/16 22:15:57, Peter Kasting wrote:
> On 2014/09/16 22:10:39, Devlin wrote:
> > On 2014/09/16 22:08:49, Peter Kasting wrote:
> > > On 2014/09/16 21:58:29, Devlin wrote:
> > > > On 2014/09/16 21:40:54, Peter Kasting wrote:
> > > > > It looks like now |i| is going to be based on the total number of
> browser
> > > > > actions in both containers rather than just the number in the overflow
> > > > > container.  But none of the other ode here changed.  That seems
> worrisome?
> > > > 
> > > > Line 551 ensures that we're in the overflow container here.
> > > 
> > > I know.  I'm saying, the old code here called GetFirstVisibleIconIndex()
> which
> > > would always return 0.  Now this code calls something which is presumably
> > > guaranteed to be nonzero.  So the meaning of |i| has changed, but none of
> the
> > > other code here appears to have changed in parallel.
> > 
> > GetFirstVisibleIconIndex() would always return
> > main_container_->VisibleBrowserActionsAfterAnimation() for the overflow
> > container, and always return 0 for the main container.  Where we call it
here
> is
> > for the overflow container, so VisibleBrowserActionsAfterAnimation() is
> correct.
> > 
> > Right?
> 
> Oh, I'm dumb, I mentally reversed the contents of GetFirstVisibleIconIndex(). 
> Sorry...

Happens to all of us. :)

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

https://codereview.chromium.org/556293003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:336: // if the
container shouldn't be shown.
On 2014/09/16 22:15:57, Peter Kasting wrote:
> Nit: How about something like:
> 
> ...and always of at least one row, even when we're empty.  The parent may
decide
> to show us even when empty, e.g. as a drag target for dragging in icons from
the
> main container.

Done.

[Devlin, 2014-09-16 22:25:45]

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

[commit-bot: I haz the power, 2014-09-16 22:27:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/556293003/60001

[commit-bot: I haz the power, 2014-09-16 23:57:34]

Committed patchset #3 (id:60001) as 699ffb6e5d0cf0b813047d8fe8a761f021c91f8c

[commit-bot: I haz the power, 2014-09-16 23:58:02]

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