Extension Toolbar redesign, part 1 (overflow)

Extension Toolbar redesign, part 1 (overflow)

Add a flag that allows the browser action icons to overflow into the wrench menu and make sure all extensions without browser action/page action icon show at least the default icon.

The overflow is implemented by introducing a master/slave mode for the BrowserActionsContainer (BAC) class. The BAC next to the Omnibox is considered the master and works as before. The BAC in inside the wrench menu is considered the slave and shows only the icons that the master does not. 

Known issues:

- Dragging between BAC objects works, but in one direction only (from slave to master). The other way doesn't work because the wrench menu isn't a drop target for browser action icons (and doesn't open).
- Context menu handling for browser action icons needs to be changed to prevent a nested messageloop when you right-click two icons in a row.
- Badges on icons in the overflow container need adjusting (appear a bit too low).

BUG=391280
TBR=cpu

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

[Finnur, 2014-06-20 17:54:27]

Devlin, can you review files with |extension| or |toolbar| in the path and the
gypi files.

Scott, can you do OWNERS for |views| files and file with |chrome/app| in path.
And also review the wrench menu & toolbar files?

[Devlin, 2014-06-23 17:55:30]

First run-through.

I tried to patch this in on Linux, and sadly, it didn't work.  Adding extensions
with BrowserActions worked fine, and I could shrink the BAC as normal, but
whenever I had a BrowserAction that should have overflowed in the wrench menu
and then clicked on the wrench menu, it crashed.  Clicking the wrench menu with
no overflowing actions worked fine.  Very unfortunately, there was no stack
trace associated with the crash (even with --single-process and
--in-process-gpu), so I'm not sure what's happening.  If you need me to look
into it more (I'm not sure if similar behavior is seen on Windows, or if you
have access to a linux box), lemme know and I'll dig deeper.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (right):

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:188: views::Widget*
parent;
nit: initialize to NULL.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:191: if (nested) {
nit: since |nested| is only used here, and is short to define, just inline it in
the if clause.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:194: // already visible
menu, in this case the wrench menu (hamburger).
I don't know if I've ever heard the wrench menu be called the hamburger menu
before (I know I've heard "wrench", "options", "stack", "application", "hotdog",
etc). Is hamburger a common name, too?  (I'm not really too worried, I think
everyone knows that the food-related menu is the options menu.)

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

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:130: 
For a sanity check, can we do something like:
DCHECK(!is_slave() || FeatureSwitch::extension_action_redesign()->IsEnabled())?

(If so, maybe just make a bool for the switch since it'll be used on line 144.)

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:270: (is_slave() ?
master_->VisibleBrowserActions() : 0);
I'm a little nervous about how master and slave each have a full set of browser
action icons.  At best, it seems like extra work, and at worst, it seems very
prone to error.  Unfortunately, unless we link the master and slave, it'd be
difficult to use any other approach.  What do you think?

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:280:
(std::max(((our_icon_count - 1) / kIconsPerMenuRow), 0U) + 1) *
I think you'll get a compile error here on pickier systems - need 0lu.
That said, do you actually need the std::max?  If our_icon_count == 0, we return
on line 275, so our_icon_count - 1 will be >= 0, and >= 0 / > 0 is >= 0 always,
right?

Also, it may be worth putting a small comment with the y-calculation.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:282: } else {
nit: no need for the else after a return.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:324: if (chevron_)
nit: else if (chevron_) {

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:831: return;
Obviously not this patch's code, but this return looks kind of... useless.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:873: if (tp &&
chevron_) {
maybe instead:
if (!tp || !chevron)
  return;
...

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:47: // two flavors,
"master" (used for the toolbar) and "slave" (used in the main
"master" and "slave" is kinda weird here, because there's not a master/slave
relationship, per se.  Master is completely unaware of slave, and slave makes
all its own decisions (admittedly based on masters behavior).  I might opt for
something more direct, like "main" and "overflow", but I don't feel strongly
about it.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:57: // whereas the
slave container is placed at the top of the hot dog menu. The
nit: I would say "the slave container is placed within the hot dog menu, at the
top" or something similar to indicate it is stored |inside| the menu, rather
than, e.g., above it.

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

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h:13: //
ExtensionToolbarMenuView is the view containing the ToolbarAction icons.
This comment doesn't seem quite right.  "ToolbarAction" is vague, and this
should mention that this is owned by the wrench menu.  "ExtensionToolbarMenuView
is the view containing the extension actions that overflowed from the
BrowserActionsContainer, and is contained in and owned by the wrench menu." or
something to that effect?

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1152: // items enabled state
updates appropriately.
does this comment need updating?

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1253:
extension_toolbar_menu_view_.reset(
If this variable is only ever created and then released in PopulateMenu, does it
need to be a persistent member variable?

https://codereview.chromium.org/324393002/diff/150001/chrome/common/extension...
File
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.cc
(right):

https://codereview.chromium.org/324393002/diff/150001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.cc:19:
bool SynthesizeBrowserActionHandler::Parse(Extension* extension,
This seems kinda weird.  I think a better solution would be something along the
lines of:
- Make an ExtensionAction handler which parses the data for both page and
browser actions.
- Have that handler always parse for extension types.
- Have it create the default browser action (if necessary) in
ManifestHandler::Validate().

Obviously, that's a much bigger change, so I'd settle for a TODO(finnur or
devlin) to combine some of the logic here.

https://codereview.chromium.org/324393002/diff/150001/extensions/common/featu...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/324393002/diff/150001/extensions/common/featu...
extensions/common/feature_switch.cc:44: switches::kExtensionActionRedesign,
nit: Can we have the order in the .h, the static class, and the definition below
all be similar?

https://codereview.chromium.org/324393002/diff/150001/extensions/common/featu...
extensions/common/feature_switch.cc:45: FeatureSwitch::DEFAULT_ENABLED),
This is scary.  Should this really be enabled by default?

https://codereview.chromium.org/324393002/diff/150001/extensions/common/switc...
File extensions/common/switches.h (right):

https://codereview.chromium.org/324393002/diff/150001/extensions/common/switc...
extensions/common/switches.h:25: extern const char kExtensionActionRedesign[];
nit: ExtensionAction < ExtensionContent

[Finnur, 2014-06-24 15:23:43]

> I'm not sure if similar behavior is seen on Windows

Nope, worked fine when I uploaded.

> I tried to patch this in on Linux, and sadly, it didn't work.

I'll take a look.

[Finnur, 2014-06-24 17:00:50]

Half way through replying to the comments when I found the cause of the crash.
So, the code hasn't changed, but look at my third comment.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (right):

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:194: // already visible
menu, in this case the wrench menu (hamburger).
I've heard it called both and it looks more to me like hamburger than a hotdog
(although I see the resemblance either way). :)

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

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:831: return;
No, we want only one action to get deleted (it is inside the for loop).

But I'll add a comment.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1262: parent, menu_index, model,
i, model->GetTypeAt(i), height);
It crashes because parent->GetSubmenu() returns NULL on Linux and AddMenuItem
uses that pointer without NULL checking if height > 0 (on line 1347 below). Not
sure why it is different on Windows.

Maybe Scott knows.

[Finnur, 2014-06-25 16:18:11]

I think I've replied/changed code to address all comments. PTAL.

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

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:270: (is_slave() ?
master_->VisibleBrowserActions() : 0);
Yeah, I know. But I think there are complexities either way we do it.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:280:
(std::max(((our_icon_count - 1) / kIconsPerMenuRow), 0U) + 1) *
Good point.

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:47: // two flavors,
"master" (used for the toolbar) and "slave" (used in the main
Converted to main and overflow.

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

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h:13: //
ExtensionToolbarMenuView is the view containing the ToolbarAction icons.
SGTM

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1253:
extension_toolbar_menu_view_.reset(
Nope.

https://codereview.chromium.org/324393002/diff/150001/extensions/common/featu...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/324393002/diff/150001/extensions/common/featu...
extensions/common/feature_switch.cc:44: switches::kExtensionActionRedesign,
Sure. My initial plan was to reorder this list to be alphabetical (and that was
even part of the initial CL), but when I got merge conflicts I decided it would
be better done as a follow-up. I did make the order of this consistent, though.

https://codereview.chromium.org/324393002/diff/150001/extensions/common/featu...
extensions/common/feature_switch.cc:45: FeatureSwitch::DEFAULT_ENABLED),
Gaaaa! No. Good catch.

On 2014/06/23 17:55:29, Devlin wrote:
> This is scary.  Should this really be enabled by default?

[Devlin, 2014-06-26 00:04:08]

halfway through round 2.  I'll finish up tomorrow morning.

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

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:280:
(std::max(((our_icon_count - 1) / kIconsPerMenuRow), 0U) + 1) *
On 2014/06/25 16:18:11, Finnur wrote:
> Good point.

I don't see a change for this...?

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

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:278: if
(browser_action_views_.empty() || our_icon_count == 0)
I don't think it should be possible for browser_action_views_ to be empty and
*not* have |our_icon_count| be zero... prefer just
if (our_icon_count == 0) {
...

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:328:
chevron_->SetVisible(false);
nit: indentation

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:335:
browser_action_views_.size() - main_container_->VisibleBrowserActions();
We rely on VisibleBrowserActions() here, even though it will be "set" for the
main container in this same method (since it just returns which browser actions
are visible, and that's modified here on line 356ish.  Is there a risk of a race
condition, in which we render incorrectly because we use an out-dated number of
visible actions?  (If so, would it only be for one frame?  I'm not familiar
enough with our graphics stack...)

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:336: for (size_t i
= 0; i < our_icon_count; ++i) {
I personally think it's a bit clear that this is handling overflow as:
for (size_t i = main_container_->VisibleBrowserActions();
     i < browser_action_views_.size();
     ++i) {
  BrowserActionView* view = browser_action_views_[i];
  ...
}

But I don't feel strongly about it.

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:344:
(std::max((static_cast<int>(i) / kIconsPerMenuRow), 0)),
I think this std::max() isn't needed, either?

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:348:
view->SetVisible(true);
We never SetVisible(false) for the overflow container - do we need to?

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:401:
(chevron_->GetPreferredSize().width() + kChevronSpacing) : 0);
Is this not a seg fault risk because the wrench menu isn't a drag target yet?

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:70: //   C: An
optional chevron, visible for overflow.  As wide as the
This comment is kind of confusing, since things like the chevron aren't visible
with the switch turned on.  Could we make that a bit more clear?

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:368: // mode). See
class comments for details on the difference).
nit: Parentheses mismatch.

[Finnur, 2014-06-26 12:00:44]

All addressed, and all uploaded this time, I hope. Sorry for the confusion. :)

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

https://codereview.chromium.org/324393002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:280:
(std::max(((our_icon_count - 1) / kIconsPerMenuRow), 0U) + 1) *
Somehow forgot to upload it. Should be done now.

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

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:335:
browser_action_views_.size() - main_container_->VisibleBrowserActions();
This code path only hits in the overflow container and it only checks the main
container (never the other way around). The overflow container is created when
the menu opens, at which point the main container has already been created. I
guess it is possible to be out-dated due to animations that are in progress
while the menu is firing up... It doesn't hurt to use my new function that takes
animations into account. I'll do that.

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:336: for (size_t i
= 0; i < our_icon_count; ++i) {
On 2014/06/26 00:04:07, Devlin wrote:
> I personally think it's a bit clear that this is handling overflow as:
> for (size_t i = main_container_->VisibleBrowserActions();
>      i < browser_action_views_.size();
>      ++i) {
>   BrowserActionView* view = browser_action_views_[i];
>   ...
> }
> 
> But I don't feel strongly about it.

Done.

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:344:
(std::max((static_cast<int>(i) / kIconsPerMenuRow), 0)),
What is this, a crusade now? ;)

But yeah, I think you are right. Removed it.

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:348:
view->SetVisible(true);
The |view| here is the browser action. For the main container, we hide the
|view|s that overflow, but the overflow container is unbounded (it just grows
more menu lines as needed). We might need a scroll solution in the menu
eventually, but it is out of scope for now.

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:401:
(chevron_->GetPreferredSize().width() + kChevronSpacing) : 0);
Correct. I'll fix this.

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:70: //   C: An
optional chevron, visible for overflow.  As wide as the
Took a stab at it.

[Finnur, 2014-06-26 14:55:48]

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1348: parent->CreateSubmenu();
This is what fixes your crash. The AddMenuItem functions implicitly add the
submenu if it hasn't been created before, but if the *first* menu in the list
has a non-zero height, then this doesn't happen (because we don't take the else
clause here in that case).

The reason it happened on Linux and not Windows is because Windows Chrome has a
hidden top menu item (Show Incompatibilities), which implicitly adds a submenu
for the parent, so it doesn't crash in this location. When I commented that menu
out, it started crashing on Windows as well.

But that's no longer a concern. If the submenu is missing for the parent, we add
it.

[Devlin, 2014-06-26 16:29:13]

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

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:348:
view->SetVisible(true);
On 2014/06/26 12:00:44, Finnur wrote:
> The |view| here is the browser action. For the main container, we hide the
> |view|s that overflow, but the overflow container is unbounded (it just grows
> more menu lines as needed). We might need a scroll solution in the menu
> eventually, but it is out of scope for now.

Yeah, I was wondering if, since both BACs have a full set of browser actions,
the overflow container would have to ensure that any items that no longer
displayed (because they moved to the main bar) are, in fact, hidden (rather than
simply overlayed by another).  Basically, just something like:
for (int i = 0; i < main_container_->VisibleBrowserActions(); ++i) {
  // Ensure that any browser actions shown in the main view are hidden in
  // the overflow view.
  (*browser_action_views_[i])->SetVisible(false);
}

But I'll defer to your expertise for whether or not this is needed.

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

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:283: // times the
number of rows need.
nit: s/need/needed.

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:338:
browser_action_views_[i];
nit: I think this can go on the previous line now.

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:71: //      it has
icons that don't fit in the space allocated for it. As wide as
nit: "don't fit in the space allocated for it and there is not another container
in 'overflow' mode."

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

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:41: // If we go
above 6 icons, make sure the seventh is in line with the right
If I'm reading this comment right, it doesn't quite look like this worked (at
least, on linux).  The right edge of the 7th icon looks significantly more left
than the right edge of the paste button.

http://imgur.com/1Z2HEF8

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

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.h:21: class
ExtensionToolbarMenuView;
nit: no need for this anymore.

https://codereview.chromium.org/324393002/diff/220001/chrome/common/extension...
File
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.cc
(right):

https://codereview.chromium.org/324393002/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.cc:32:
!extension->manifest()->HasKey(manifest_keys::kPageAction))
Page actions are designed to only show some of the time, so if an extension has
a page action, it won't show up in the new menu (even though an extension with
no browser action and no page action will).  I think it'd be better to have a
browser action per extension, and just risk showing both a page action and a
browser action.  I don't feel too strongly about it though - either way, we have
to address it later.

https://codereview.chromium.org/324393002/diff/220001/chrome/common/extension...
File
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.h
(right):

https://codereview.chromium.org/324393002/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.h:14:
// Parses the "commands" manifest key.
nit: comment needs updating.

https://codereview.chromium.org/324393002/diff/220001/extensions/common/featu...
File extensions/common/feature_switch.h (right):

https://codereview.chromium.org/324393002/diff/220001/extensions/common/featu...
extensions/common/feature_switch.h:27: static FeatureSwitch*
extension_action_redesign();
nit: looks like everywhere in the .cc file this is below
enable_override_bookmarks_ui

[Finnur, 2014-06-27 14:23:38]

Scott: Mind taking a look?

As in:
An OWNERS for |views| files and file with |chrome/app| in path. And also review
the wrench menu & toolbar files?

Devlin: All done, PTAL.

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

https://codereview.chromium.org/324393002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:348:
view->SetVisible(true);
On 2014/06/26 16:29:13, Devlin wrote:
> On 2014/06/26 12:00:44, Finnur wrote:
> > The |view| here is the browser action. For the main container, we hide the
> > |view|s that overflow, but the overflow container is unbounded (it just
grows
> > more menu lines as needed). We might need a scroll solution in the menu
> > eventually, but it is out of scope for now.
> 
> Yeah, I was wondering if, since both BACs have a full set of browser actions,
> the overflow container would have to ensure that any items that no longer
> displayed (because they moved to the main bar) are, in fact, hidden (rather
than
> simply overlayed by another).  Basically, just something like:
> for (int i = 0; i < main_container_->VisibleBrowserActions(); ++i) {
>   // Ensure that any browser actions shown in the main view are hidden in
>   // the overflow view.
>   (*browser_action_views_[i])->SetVisible(false);
> }
> 
> But I'll defer to your expertise for whether or not this is needed.

Done.

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

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:283: // times the
number of rows need.
On 2014/06/26 16:29:13, Devlin wrote:
> nit: s/need/needed.

Done.

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:338:
browser_action_views_[i];
On 2014/06/26 16:29:13, Devlin wrote:
> nit: I think this can go on the previous line now.

Done.

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:71: //      it has
icons that don't fit in the space allocated for it. As wide as
The wording was becoming a bit convoluted so I attempted to rewrite it. See if
it fits the bill.

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

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:41: // If we go
above 6 icons, make sure the seventh is in line with the right
You are correct on both accounts. I've fixed this for Linux now. Try again.

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

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.h:21: class
ExtensionToolbarMenuView;
On 2014/06/26 16:29:13, Devlin wrote:
> nit: no need for this anymore.

Done.

https://codereview.chromium.org/324393002/diff/220001/chrome/common/extension...
File
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.cc
(right):

https://codereview.chromium.org/324393002/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.cc:32:
!extension->manifest()->HasKey(manifest_keys::kPageAction))
I thought about that, and wasn't sure what to do here. I was actually more
worried about having two icons for the same extensions in the browser action
container (when we merge page and browser actions), but I guess I'll just add a
reminder here.

https://codereview.chromium.org/324393002/diff/220001/chrome/common/extension...
File
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.h
(right):

https://codereview.chromium.org/324393002/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/synthesize_browser_action_handler.h:14:
// Parses the "commands" manifest key.
On 2014/06/26 16:29:13, Devlin wrote:
> nit: comment needs updating.

Done.

https://codereview.chromium.org/324393002/diff/220001/extensions/common/featu...
File extensions/common/feature_switch.h (right):

https://codereview.chromium.org/324393002/diff/220001/extensions/common/featu...
extensions/common/feature_switch.h:27: static FeatureSwitch*
extension_action_redesign();
Indeed.

[Devlin, 2014-06-27 15:19:29]

LGTM.

Another thing we noticed was that badges on browser actions are wrong in the
overflow menu.  They get cut off and are placed lower
(http://imgur.com/zKasgXc,GGhUdBq#0 - look at the height difference between the
bottom of the 8 and the top of the 7).  With multiple rows, it shows the whole
thing, but it's too large for the space (http://imgur.com/zKasgXc,GGhUdBq#1). 
Not sure if this is linux only or all platforms.

If you want to fix it in this patch, great, otherwise we can add it to the list
of known issues. :)

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:71: //      it has
icons that don't fit in the space allocated for it. As wide as
On 2014/06/27 14:23:38, Finnur wrote:
> The wording was becoming a bit convoluted so I attempted to rewrite it. See if
> it fits the bill.

Ah, much better. Thanks :)

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

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:25: const int
kFixedMaxWidth = 312;
unfortunate that these aren't defined anywhere else... oh well.

[sky, 2014-06-27 18:08:28]

I'm out for ~2 weeks, so I'm hesitant to start on this as no doubt you want to
get it in. Not sure if Peter is around, but I'm redirecting to him.
sky->pkasting

[Finnur, 2014-06-27 21:53:56]

Thanks Scott. Enjoy your vacation. :)


On Fri, Jun 27, 2014 at 6:08 PM, <sky@chromium.org> wrote:

> I'm out for ~2 weeks, so I'm hesitant to start on this as no doubt you
> want to
> get it in. Not sure if Peter is around, but I'm redirecting to him.
> sky->pkasting
>
> https://codereview.chromium.org/324393002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-06-30 23:36:34]

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:737:
AddSeparator(ui::UPPER_SEPARATOR);
Shouldn't we only add these if the overflow menu is going to be non-empty?  Or
are there always menu items in this menu even when there are no overflowed
icons?

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (right):

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:193: // already visible
menu, in this case the wrench menu (hamburger).
Nit: wrench menu (hamburger) -> Chrome menu

(Chrome menu is the UI leads' name, AFAIK.  You can also just call it the wrench
menu since there's still code that uses that phrase, though I think that's a
little misleading.  Don't say hamburger though.)

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

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:62: // in overflow
mode (inside the app menu, aka. hamburger menu).
Nit: hamburger again

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:63: const int
kPaddingForBadge = 9;
Can we compute this value automatically from the image sizes, container size,
etc.?

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:68: const int
kIconsPerMenuRow = 8;  // The menu on Linux is wider.
Again, can we compute this from the container or parent width?

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:111: Browser*
browser, View* owner_view, BrowserActionsContainer* main_container)
Nit: One arg per line

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:277: size_t
our_icon_count = browser_action_views_.size() -
Nit: Remove "our_", that's just odd

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:278:
(handling_overflow() ? main_container_->VisibleBrowserActions() : 0);
Nit: For example, this is a place where "main_container_ ? ..." would actually
be more clearly safe than the way it's written right now.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:351: ((index /
kIconsPerMenuRow) * (IconWidth(true) * kIconsPerMenuRow));
Nit: Parens around "IconWidth(true) * kIconsPerMenuRow" not necessary

(They'd be better around "index * IconWidth(true)" in the line above)

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:354: IconHeight() *
(static_cast<int>(index) / kIconsPerMenuRow),
Nit: You compute this quotient twice, consider using a temp instead

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:361: for (size_t i
= 0; i < browser_action_views_.size(); ++i) {
Nit: Use an iterator

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:364: if (x +
icon_width <= max_x) {
Nit: Simpler:

      view->SetVisible(x + icon_width <= max_x);
      if (view->visible())
        view->SetBounds(x, 0, icon_width, IconHeight());

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:904: int
visible_actions = handling_overflow() ? model_->toolbar_items().size() -
Nit: Break after '?' instead of before ':' and indent 4

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:914:
IconCountToWidth(visible_actions, chevron_ ? chevron_->visible() : false);
Nit: Simpler: chevron_ && chevron_->visible()

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:982: (chevron_ ?
(kChevronSpacing + chevron_->GetPreferredSize().width() +
Nit: Unusual indenting/wrapping; how about this:

  if (!chevron_)
    return ToolbarView::kStandardSpacing;
  return (ToolbarView::kStandardSpacing * 2) + kChevronSpacing +
      chevron_->GetPreferredSize().width();

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:49: // hot
dog/hamburger menu).
Nit: Again, do not use "hot dog" or "hamburger" to refer to the Chrome menu in
code/comments.  (multiple places)

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:52: //
BrowserActionContainer but in 'overflow' mode the container is effectively
Nit: BrowserActionContainer but -> BrowserActionContainer, but

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:72: //      - This
is the main container.
Nit: This point is already clear from the fact that this whole description is
about the main container.  Remove.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:78: //           
inside the wrench menu.
Nit: This NOTE seems like a rehash of comments above, I'd remove it.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:149: //
|main_container_|, see class comments.
Nit: No trailing underscore

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:374: // Whether this
container is overflow container (as opposed to in 'main'
Nit: overflow container -> in overflow mode (or say "the overflow container" and
change "in 'main' mode" to "the main container").

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:376: bool
handling_overflow() const { return main_container_ != NULL; }
Nit: handling_overflow() is a rather confusing name IMO.  I'd think
is_overflow_container() or in_overflow_mode() (pick what matches the comment)
would be better.

However, given that this is private, I suggest calling code either just check
whether |main_container_| is non-NULL directly, or call ShownInsideMenu().

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:413: // is
permanently suppressing the chevron menu.
Nit: "NULL when in overflow mode or when the toolbar..." or similar.

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

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:15: const int
kLeftEdgePadding = 33;
Can we get this constant from elsewhere?  Seems like other parts of the menu
must have either static or dynamic constants/calculations that produce this
value, avoiding the need to hardcode here.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:19: const int
kVerticalPadding = 8;
Can we compute this from the icon sizes and menu height?

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:25: const int
kFixedMaxWidth = 312;
On 2014/06/27 15:19:28, Devlin wrote:
> unfortunate that these aren't defined anywhere else... oh well.

It seems like we ought to be able to compute these from constants or the dynamic
widths of parent objects or something defined elsewhere.

If these were determined empirically, they're probably wrong.  The menu has far
different widths depending on different locales, so e.g. looking at English
Chrome will give you a value that doesn't make sense for Malayalam Chrome.  This
is why we should be calculating the number of icons per row dynamically, based
on the current width of this object.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:60: // If we even
more room, maybe consider adding another icon?
Nit: This sentence no grammar

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

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h:21: // Overridden
from View.
Nit: We're wildly inconsistent about what to put in this comment, but personally
I prefer just "views::View:" which is as short as possible.  (Use the same name
the class parent declaration above does.)

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h:27: 
Nit: Unnecessary blank line

[Finnur, 2014-07-02 16:59:57]

Peter, PTAL

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:737:
AddSeparator(ui::UPPER_SEPARATOR);
This experiment is behind a flag and more is needed before it can be considered
fully implemented. I've added a TODO for this.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (right):

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:193: // already visible
menu, in this case the wrench menu (hamburger).
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: wrench menu (hamburger) -> Chrome menu
> 
> (Chrome menu is the UI leads' name, AFAIK.  You can also just call it the
wrench
> menu since there's still code that uses that phrase, though I think that's a
> little misleading.  Don't say hamburger though.)

Done.

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

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:62: // in overflow
mode (inside the app menu, aka. hamburger menu).
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: hamburger again

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:63: const int
kPaddingForBadge = 9;
Updated comment (see my next reply also, applies to this also).

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:68: const int
kIconsPerMenuRow = 8;  // The menu on Linux is wider.
Yes, definitely. I don't have time for it in this batch of changes (am handing
this off to Devlin before I go on vacation) so I added a TODO.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:111: Browser*
browser, View* owner_view, BrowserActionsContainer* main_container)
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: One arg per line

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:277: size_t
our_icon_count = browser_action_views_.size() -
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: Remove "our_", that's just odd

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:278:
(handling_overflow() ? main_container_->VisibleBrowserActions() : 0);
I wrote this CL a few months back and had to put it on hold to work on other
stuff. When I resurrected the CL and started getting familiar with the code
again I found that I got terribly confused by reading things like:

if (main_container_)
  foo();

... because it is so easy to read this as 'if this is the main container', when
in fact the opposite is what is intended. Using...

if (handling_overflow())
  foo();

... removes that chance of confusion.

So yes, one does take a little bit longer verifying that this is safe, but the
upside is that the code is generally more readable.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:351: ((index /
kIconsPerMenuRow) * (IconWidth(true) * kIconsPerMenuRow));
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: Parens around "IconWidth(true) * kIconsPerMenuRow" not necessary
> 
> (They'd be better around "index * IconWidth(true)" in the line above)

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:354: IconHeight() *
(static_cast<int>(index) / kIconsPerMenuRow),
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: You compute this quotient twice, consider using a temp instead

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:361: for (size_t i
= 0; i < browser_action_views_.size(); ++i) {
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: Use an iterator

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:364: if (x +
icon_width <= max_x) {
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: Simpler:
> 
>       view->SetVisible(x + icon_width <= max_x);
>       if (view->visible())
>         view->SetBounds(x, 0, icon_width, IconHeight());

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:904: int
visible_actions = handling_overflow() ? model_->toolbar_items().size() -
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: Break after '?' instead of before ':' and indent 4

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:914:
IconCountToWidth(visible_actions, chevron_ ? chevron_->visible() : false);
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: Simpler: chevron_ && chevron_->visible()

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:982: (chevron_ ?
(kChevronSpacing + chevron_->GetPreferredSize().width() +
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: Unusual indenting/wrapping; how about this:
> 
>   if (!chevron_)
>     return ToolbarView::kStandardSpacing;
>   return (ToolbarView::kStandardSpacing * 2) + kChevronSpacing +
>       chevron_->GetPreferredSize().width();

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:49: // hot
dog/hamburger menu).
On 2014/06/30 23:36:34, Peter Kasting wrote:
> Nit: Again, do not use "hot dog" or "hamburger" to refer to the Chrome menu in
> code/comments.  (multiple places)

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:52: //
BrowserActionContainer but in 'overflow' mode the container is effectively
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: BrowserActionContainer but -> BrowserActionContainer, but

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:72: //      - This
is the main container.
On 2014/06/30 23:36:34, Peter Kasting wrote:
> Nit: This point is already clear from the fact that this whole description is
> about the main container.  Remove.

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:78: //           
inside the wrench menu.
On 2014/06/30 23:36:33, Peter Kasting wrote:
> Nit: This NOTE seems like a rehash of comments above, I'd remove it.

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:149: //
|main_container_|, see class comments.
On 2014/06/30 23:36:34, Peter Kasting wrote:
> Nit: No trailing underscore

Done.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:376: bool
handling_overflow() const { return main_container_ != NULL; }
Changed it to in_overflow_mode. I think that is the most readable of the
alternatives, as it describes the purpose of the container and at the same time
helps explains the logic a bit (why we are dealing with the icons that the main
container isn't showing).

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:413: // is
permanently suppressing the chevron menu.
On 2014/06/30 23:36:34, Peter Kasting wrote:
> Nit: "NULL when in overflow mode or when the toolbar..." or similar.

Done.

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

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:15: const int
kLeftEdgePadding = 33;
Yes, I've found the right function to call.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:19: const int
kVerticalPadding = 8;
This comment of mine is incorrect. I've revised it, and added a TODO to fix the
underlying cause (which will enable us to remove this const).

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:25: const int
kFixedMaxWidth = 312;
I took a stab at doing this properly, but ran out of time and just removed it
(so now it is not altering the menu width to align the last button with the
paste button).

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:60: // If we even
more room, maybe consider adding another icon?
YAH U CAN HAZ GRAMMER.

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

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h:21: // Overridden
from View.
Weird. I switched my preference from this to views::View: myself at some point
in the past, but this must be that saying about the old dog, new tricks, and all
that. Anyway -- changed.

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h:27: 
On 2014/06/30 23:36:34, Peter Kasting wrote:
> Nit: Unnecessary blank line

Done.

[Peter Kasting, 2014-07-02 20:54:01]

LGTM

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:374: // Whether this
container is overflow container (as opposed to in 'main'
On 2014/06/30 23:36:34, Peter Kasting wrote:
> Nit: overflow container -> in overflow mode (or say "the overflow container"
and
> change "in 'main' mode" to "the main container").

You should still do this.

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

https://codereview.chromium.org/324393002/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:372: ((it -
browser_action_views_.begin()) * IconWidth(true));
Oh, somehow I missed that you were needing the index value in this statement. 
Maybe an index would have been better after all.

Up to you, pick whichever you think is best.

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

https://codereview.chromium.org/324393002/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:15: // overflow
menu are cut off (so we can remove this).
Nit: Move TODO to be just after this next sentence instead of before it.

[Finnur, 2014-07-03 13:59:32]

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.h (right):

https://codereview.chromium.org/324393002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.h:374: // Whether this
container is overflow container (as opposed to in 'main'
Indeed.

[Finnur, 2014-07-03 14:06:40]

TBR-ing Carlos for OWNERS on chrome_command_ids.h.

[Finnur, 2014-07-03 14:12:03]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2014-07-03 14:12:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/finnur@chromium.org/324393002/340001

[commit-bot: I haz the power, 2014-07-03 16:11:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-03 17:37:03]

Change committed as 281291