Optimizing the time taken to build and show the WrenchMenu.

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

Index: chrome/browser/ui/views/toolbar/wrench_menu.cc
diff --git a/chrome/browser/ui/views/toolbar/wrench_menu.cc b/chrome/browser/ui/views/toolbar/wrench_menu.cc
index c2e2985776833bd48f35b0379ec367bf1e4ef9cc..9789d100b2430018bef3858695a1264ddf039570 100644
--- a/chrome/browser/ui/views/toolbar/wrench_menu.cc
+++ b/chrome/browser/ui/views/toolbar/wrench_menu.cc
@@ -371,6 +371,7 @@ class WrenchMenuView : public views::View,
 
  protected:
   WrenchMenu* menu() { return menu_; }
+  WrenchMenu* menu() const { return menu_; }

[Peter Kasting, 2015/07/23 18:46:32]

Const methods cannot return non-const pointers.  You can return a const
WrenchMenu* if need be.  Otherwise we need to come up with a different solution
to whatever problem you're having.

[bruthig, 2015/07/23 21:18:04]

Done.

   ButtonMenuItemModel* menu_model() { return menu_model_; }
 
  private:
@@ -492,7 +493,8 @@ class WrenchMenu::ZoomView : public WrenchMenuView {
         zoom_label_(NULL),
         decrement_button_(NULL),
         fullscreen_button_(NULL),
-        zoom_label_width_(0) {
+        zoom_label_width_(0),
+        zoom_label_width_value_is_valid_(false) {
     browser_zoom_subscription_ =
         ui_zoom::ZoomEventManager::GetForBrowserContext(
             menu->browser_->profile())
@@ -514,7 +516,7 @@ class WrenchMenu::ZoomView : public WrenchMenuView {
     zoom_label_->set_background(center_bg);
 
     AddChildView(zoom_label_);
-    zoom_label_width_ = MaxWidthForZoomLabel();
+    zoom_label_width_value_is_valid_ = false;
 
     increment_button_ = CreateButtonWithAccName(
         IDS_ZOOM_PLUS2, InMenuButtonBackground::RIGHT_BUTTON,
@@ -560,8 +562,9 @@ class WrenchMenu::ZoomView : public WrenchMenuView {
     // Returned height doesn't matter as MenuItemView forces everything to the
     // height of the menuitemview. Note that we have overridden the height when
     // constructing the menu.
-    return gfx::Size(button_width + zoom_label_width_ + button_width +
-                     fullscreen_width, 0);
+    return gfx::Size(
+        button_width + MaxWidthForZoomLabel() + button_width + fullscreen_width,
+        0);
   }
 
   void Layout() override {
@@ -574,7 +577,7 @@ class WrenchMenu::ZoomView : public WrenchMenuView {
 
     x += bounds.width();
     bounds.set_x(x);
-    bounds.set_width(zoom_label_width_);
+    bounds.set_width(MaxWidthForZoomLabel());
     zoom_label_->SetBoundsRect(bounds);
 
     x += bounds.width();
@@ -596,7 +599,7 @@ class WrenchMenu::ZoomView : public WrenchMenuView {
     zoom_label_->SetBorder(views::Border::CreateEmptyBorder(
         0, kZoomLabelHorizontalPadding, 0, kZoomLabelHorizontalPadding));
     zoom_label_->SetFontList(menu_config.font_list);
-    zoom_label_width_ = MaxWidthForZoomLabel();
+    zoom_label_width_value_is_valid_ = false;
 
     if (theme) {
       zoom_label_->SetEnabledColor(theme->GetSystemColor(
@@ -651,11 +654,20 @@ class WrenchMenu::ZoomView : public WrenchMenuView {
     zoom_label_->SetText(
         l10n_util::GetStringFUTF16Int(IDS_ZOOM_PERCENT, zoom));
 
-    zoom_label_width_ = MaxWidthForZoomLabel();
+    zoom_label_width_value_is_valid_ = false;
   }
 
-  // Calculates the max width the zoom string can be.
-  int MaxWidthForZoomLabel() {
+  // Returns the max width the zoom string can be.
+  int MaxWidthForZoomLabel() const {

[Peter Kasting, 2015/07/23 18:46:32]

Nit: ZoomLabelMaxWidth()?  MaxZoomLabelWidth()?  And then change both the member
variable names to match

[bruthig, 2015/07/23 21:18:03]

Done.

+    if (!zoom_label_width_value_is_valid_) {
+      zoom_label_width_ = CalculateMaxWidthForZoomLabel();

[Peter Kasting, 2015/07/23 18:46:32]

Nit: Since this is called once, just inline its code into here.

[jonross, 2015/07/23 19:55:51]

It is called in both GetPreferredSize and Layout.

[jonross, 2015/07/23 20:01:16]

Nevermind, misread. Please ignore.

[bruthig, 2015/07/23 21:18:04]

Done.  But I am curious why it is you prefer to inline this long and somewhat
complex method?  From a readability standpoint I find having the
CalculateZoomLabelMaxWidth() much easier to read and understand.  It offers a
nice level of abstraction from the perspective of the ZoomLabelMaxWidth()
method. i.e.  I don't have to sift through the details of the calculation code
when trying to understand how ZoomLabelMaxWidth() to fully understand how
ZoomLabelMaxWidth() is performing the caching I can simply trust that
CalculateZoomLabelMaxWidth() is doing something.

+      zoom_label_width_value_is_valid_ = true;
+    }
+    return zoom_label_width_;
+  }
+
+  // Calculates and returns the max width the zoom string can be.
+  int CalculateMaxWidthForZoomLabel() const {
     const gfx::FontList& font_list = zoom_label_->font_list();
     int border_width =
         zoom_label_->border() ? zoom_label_->border()->GetInsets().width() : 0;
@@ -699,8 +711,14 @@ class WrenchMenu::ZoomView : public WrenchMenuView {
 
   ImageButton* fullscreen_button_;
 
-  // Width given to |zoom_label_|. This is the width at 100%.
-  int zoom_label_width_;
+  // Cached width of how wide the zoom label string can be. This should not be
+  // accessed directly, use MaxWidthForZoomLabel() instead. This is the width at
+  // 100%.

[Peter Kasting, 2015/07/23 18:46:32]

Nit: Consider adding a short note about why we bother to cache this.

[bruthig, 2015/07/23 21:18:04]

Done.

+  mutable int zoom_label_width_;
+
+  // Flag that is true if |zoom_label_width_| is a valid value or false if it

[Peter Kasting, 2015/07/23 18:46:32]

Nit: How about "Flag tracking whether calls to MaxWidthForZoomLabel() need to
recalculate the label width, because the cached value may no longer be correct."

[bruthig, 2015/07/23 21:18:04]

Done. I like :)

+  // needs to be computed.
+  mutable bool zoom_label_width_value_is_valid_;

[Peter Kasting, 2015/07/23 18:46:32]

Nit: |zoom_label_width_valid_| is just as clear and less verbose.

[bruthig, 2015/07/23 21:18:04]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(ZoomView);
 };