Extension Toolbar redesign, part 1 (overflow)

chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h"
+
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
+
+namespace {
+
+// Padding to the left of the menu to make sure the icons are in line with the
+// rest of the menu.
+const int kLeftEdgePadding = 33;

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

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.

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

Yes, I've found the right function to call.

+
+// Vertical padding to make sure the icons are centered vertically within the
+// menu.
+const int kVerticalPadding = 8;

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

Can we compute this from the icon sizes and menu height?

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

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).

+
+// The max width we report back to the menu, when there is a full set of icons
+// in the first row. Linux has a little wider menu than Windows, so we allocate
+// more space.
+#if defined(OS_LINUX)
+const int kFixedMaxWidth = 312;

[Devlin, 2014/06/27 15:19:28]

unfortunate that these aren't defined anywhere else... oh well.

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

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.

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

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).

+#else
+const int kFixedMaxWidth = 280;
+#endif
+
+// The amount of pixels that fits all but the last icon of a row in the menu.
+#if defined(OS_LINUX)
+const int kResizeWidth = 258;
+#else
+const int kResizeWidth = 200;
+#endif
+
+}  // namespace
+
+ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser)
+    : browser_(browser) {
+  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
+  container_ = new BrowserActionsContainer(
+      browser_,
+      NULL,  // No owner view, means no extra keybindings are registered.
+      browser_view->GetToolbarView()->browser_actions());
+  container_->Init();
+  AddChildView(container_);
+}
+
+ExtensionToolbarMenuView::~ExtensionToolbarMenuView() {
+}
+
+gfx::Size ExtensionToolbarMenuView::GetPreferredSize() const {
+  gfx::Size sz = container_->GetPreferredSize();
+  if (sz.height() == 0)
+    return sz;
+  // If we fill up max icons per row, make sure the last icon is in line with
+  // the right edge of the Paste button further down the menu.
+  if (sz.width() > kResizeWidth) {
+    // If we even more room, maybe consider adding another icon?

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

Nit: This sentence no grammar

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

YAH U CAN HAZ GRAMMER.

+    DCHECK_LT(sz.width(), kFixedMaxWidth);
+
+    sz.set_width(std::max(sz.width(), kFixedMaxWidth));
+  }
+
+  sz.Enlarge(0, kVerticalPadding);
+  return sz;
+}
+
+void ExtensionToolbarMenuView::Layout() {
+  // All buttons are given the same width.
+  gfx::Size sz = container_->GetPreferredSize();
+  container_->SetBounds(
+      kLeftEdgePadding, 0, sz.width(), sz.height() + (kVerticalPadding / 2));
+}