Updates Zoom bubble layout and adds +/- buttons

chrome/browser/ui/views/location_bar/zoom_bubble_view.cc

Index: chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
diff --git a/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc b/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
index ec20b37ecb77020ba24d1dbc6f9a167756ebe507..f61dfb5687d1d577be95e45adf9e77dd23220860 100644
--- a/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
+++ b/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
@@ -4,9 +4,11 @@
 
 #include "chrome/browser/ui/views/location_bar/zoom_bubble_view.h"
 
+#include "base/auto_reset.h"
 #include "base/i18n/number_formatting.h"
 #include "base/i18n/rtl.h"
 #include "base/strings/stringprintf.h"
+#include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/ui/browser.h"
@@ -29,13 +31,24 @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/ui_features.h"
 #include "ui/gfx/favicon_size.h"
+#include "ui/native_theme/native_theme.h"
 #include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
+namespace {
+
+constexpr int kVerticalInset = 2;

[Evan Stade, 2017/04/27 17:29:39]

I would prefer these be defined where they are used. That gives them context.
Also I don't really see an advantage to listing them on different lines, as
opposed to 

const gfx::Insets kInsetsForSomething(2, 0, 2, 10);

[varkha, 2017/04/28 02:06:14]

Done.

+constexpr int kLeftInset = 0;
+constexpr int kRightInset = 10;
+constexpr int kResetButtonPadding = 10;

[Evan Stade, 2017/04/27 17:29:39]

ditto

[varkha, 2017/04/28 02:06:13]

Done.

+
+}  // namespace
+
 // static
 ZoomBubbleView* ZoomBubbleView::zoom_bubble_ = nullptr;
 
@@ -133,8 +146,12 @@ void ZoomBubbleView::Refresh() {
   zoom::ZoomController* zoom_controller =
       zoom::ZoomController::FromWebContents(web_contents_);
   int zoom_percent = zoom_controller->GetZoomPercent();
-  label_->SetText(l10n_util::GetStringFUTF16(
-      IDS_TOOLTIP_ZOOM, base::FormatPercent(zoom_percent)));
+  const base::string16 label_text =
+      ui::MaterialDesignController::IsSecondaryUiMaterial()
+          ? base::FormatPercent(zoom_percent)
+          : l10n_util::GetStringFUTF16(IDS_TOOLTIP_ZOOM,
+                                       base::FormatPercent(zoom_percent));
+  label_->SetText(label_text);

[varkha, 2017/04/27 11:42:39]

Self review: carve out utility UpdateZoomPercent() method and reuse here and in
Init().

[varkha, 2017/04/28 02:06:13]

Done.

   StartTimerIfNecessary();
 }
 
@@ -147,8 +164,12 @@ ZoomBubbleView::ZoomBubbleView(
     : LocationBarBubbleDelegateView(anchor_view, anchor_point, web_contents),
       image_button_(nullptr),
       label_(nullptr),
+      zoom_out_button_(nullptr),
+      zoom_in_button_(nullptr),
+      reset_button_(nullptr),
       web_contents_(web_contents),
       auto_close_(reason == AUTOMATIC),
+      ignore_close_bubble_(false),
       immersive_mode_controller_(immersive_mode_controller) {
   set_notify_enter_exit_on_child(true);
   if (immersive_mode_controller_)
@@ -193,6 +214,18 @@ void ZoomBubbleView::Init() {
   }
   columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
+  const bool material = ui::MaterialDesignController::IsSecondaryUiMaterial();
+  if (material) {
+    columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
+                       views::GridLayout::USE_PREF, 0, 0);
+    columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
+                       views::GridLayout::USE_PREF, 0, 0);
+    columns->AddPaddingColumn(0, kResetButtonPadding);
+    columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::FILL, 0,
+                       views::GridLayout::USE_PREF, 0, 0);
+    grid_layout->SetInsets(kVerticalInset, kLeftInset, kVerticalInset,
+                           kRightInset);
+  }
   grid_layout->StartRow(0, 0);
 
   // If this zoom change was initiated by an extension, that extension will be
@@ -211,22 +244,52 @@ void ZoomBubbleView::Init() {
   zoom::ZoomController* zoom_controller =
       zoom::ZoomController::FromWebContents(web_contents_);
   int zoom_percent = zoom_controller->GetZoomPercent();
-  label_ = new views::Label(l10n_util::GetStringFUTF16(
-      IDS_TOOLTIP_ZOOM, base::FormatPercent(zoom_percent)));
-  label_->SetFontList(GetTitleFontList());
+  const base::string16 label_text =
+      material ? base::FormatPercent(zoom_percent)
+               : l10n_util::GetStringFUTF16(IDS_TOOLTIP_ZOOM,
+                                            base::FormatPercent(zoom_percent));
+  label_ = new views::Label(label_text);
+  if (material) {
+    zoom_out_button_ = views::CreateVectorImageButton(this);
+    views::SetImageFromVectorIcon(zoom_out_button_, kMinusIcon);
+    zoom_out_button_->SetFocusForPlatform();
+    zoom_out_button_->SetTooltipText(
+        l10n_util::GetStringUTF16(IDS_ACCNAME_ZOOM_MINUS2));

[varkha, 2017/04/27 11:42:39]

Self review: carve out utility ImageButton*
CreateZoomButton(icon, tooltip_id).

[varkha, 2017/04/28 02:06:13]

Done.

+    grid_layout->AddView(zoom_out_button_);
+
+    label_->SetEnabledColor(GetNativeTheme()->GetSystemColor(
+        ui::NativeTheme::kColorId_ButtonEnabledColor));
+  } else {
+    label_->SetFontList(GetTitleFontList());
+  }
   grid_layout->AddView(label_);
 
-  // Second row.
-  grid_layout->AddPaddingRow(0, 8);
-  columns = grid_layout->AddColumnSet(1);
-  columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                     views::GridLayout::USE_PREF, 0, 0);
-  grid_layout->StartRow(0, 1);
+  if (material) {
+    zoom_in_button_ = views::CreateVectorImageButton(this);
+    views::SetImageFromVectorIcon(zoom_in_button_, kPlusIcon);
+    zoom_in_button_->SetFocusForPlatform();
+    zoom_in_button_->SetTooltipText(
+        l10n_util::GetStringUTF16(IDS_ACCNAME_ZOOM_PLUS2));
+    grid_layout->AddView(zoom_in_button_);
+  } else {
+    // Second row.
+    grid_layout->AddPaddingRow(0, 8);
+    columns = grid_layout->AddColumnSet(1);
+    columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
+                       views::GridLayout::USE_PREF, 0, 0);
+    grid_layout->StartRow(0, 1);
+  }
 
   // Add "Reset to Default" button.
-  grid_layout->AddView(
-      views::MdTextButton::CreateSecondaryUiButton(
-          this, l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT)));
+  reset_button_ = views::MdTextButton::CreateSecondaryUiButton(
+      this, l10n_util::GetStringUTF16(material ? IDS_ZOOM_SET_DEFAULT_SHORT
+                                               : IDS_ZOOM_SET_DEFAULT));
+  if (material) {
+    reset_button_->SetTooltipText(
+        l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT));

[Evan Stade, 2017/04/27 17:29:39]

so it's normally "Reset" with a tooltip of "Reset to Default". That doesn't seem
that useful, I'd either skip the tooltip or, if "reset" is really ambiguous, use
"Reset to default zoom level", or even better, "Reset to [whatever the default
zoom level is as per settings]"

this is a product decision so I wouldn't block this CL on it.

[varkha, 2017/04/28 02:06:13]

Acknowledged. I've added a todo to change that when
IDS_ZOOM_SET_DEFAULTS is no longer used. I do find "Reset" to be a bit
ambiguous here and to include a numeric value in "Reset to []" version I think,
might be confusing as well since where that value comes from may not be
obvious to a user in this context.

+  }

[varkha, 2017/04/27 11:42:39]

nit: no newline.

[varkha, 2017/04/28 02:06:13]

Done.

+
+  grid_layout->AddView(reset_button_);
 
   StartTimerIfNecessary();
 }
@@ -239,6 +302,9 @@ void ZoomBubbleView::WindowClosing() {
 }
 
 void ZoomBubbleView::CloseBubble() {
+  if (ignore_close_bubble_)
+    return;
+
   // Widget's Close() is async, but we don't want to use zoom_bubble_ after
   // this. Additionally web_contents_ may have been destroyed.
   zoom_bubble_ = nullptr;
@@ -248,6 +314,9 @@ void ZoomBubbleView::CloseBubble() {
 
 void ZoomBubbleView::ButtonPressed(views::Button* sender,
                                    const ui::Event& event) {
+  base::AutoReset<bool> auto_ignore_close_bubble(&ignore_close_bubble_, true);
+  auto_close_ = false;
+  StopTimer();
   if (sender == image_button_) {
     DCHECK(extension_info_.icon_image) << "Invalid button press.";
     Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
@@ -255,8 +324,14 @@ void ZoomBubbleView::ButtonPressed(views::Button* sender,
         browser, GURL(base::StringPrintf("chrome://extensions?id=%s",
                                          extension_info_.id.c_str())),
         ui::PAGE_TRANSITION_FROM_API);
-  } else {
+  } else if (sender == zoom_out_button_) {
+    zoom::PageZoom::Zoom(web_contents_, content::PAGE_ZOOM_OUT);
+  } else if (sender == zoom_in_button_) {
+    zoom::PageZoom::Zoom(web_contents_, content::PAGE_ZOOM_IN);
+  } else if (sender == reset_button_) {
     zoom::PageZoom::Zoom(web_contents_, content::PAGE_ZOOM_RESET);
+  } else {
+    NOTREACHED();
   }
 }