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..80364820939c74359df909f4b79d764eaeec6892 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,7 +31,10 @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/ui_features.h"
 #include "ui/gfx/favicon_size.h"
+#include "ui/gfx/vector_icon_types.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"
@@ -130,11 +135,7 @@ ZoomBubbleView* ZoomBubbleView::GetZoomBubble() {
 }
 
 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)));
+  UpdateZoomPercent();
   StartTimerIfNecessary();
 }
 
@@ -147,8 +148,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 +198,20 @@ void ZoomBubbleView::Init() {
   }
   columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
+  const bool material = ui::MaterialDesignController::IsSecondaryUiMaterial();

[Peter Kasting, 2017/04/28 05:58:48]

Do we have to key this off secondary-ui-material?  I thought this was a UI we'd
wanted for several years now.  I'd like to avoid the conditionals and just
change this.

This would hopefully also let us switch to a box layout.

[varkha, 2017/04/28 06:21:19]

Yes, I'll do that if this is not blocked on Harmony launch.

[varkha, 2017/05/01 10:12:00]

Done.

+  if (material) {
+    const int kResetButtonPadding = 10;

[Peter Kasting, 2017/04/28 05:58:48]

This should be using a constant from the layout provider, presumably the
unrelated control horizontal spacing.

[varkha, 2017/04/28 06:21:19]

Acknowledged.

[varkha, 2017/05/01 10:12:00]

Done.

+    const gfx::Insets kBubbleContentsInsets(2, 0, 2, 10);

[Peter Kasting, 2017/04/28 05:58:48]

These numbers are very magic (and I don't recognize them).

It seems like these should be using the toast insets from the harmony spec (
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...
).  I would think the best way to do this would be to provide a new InsetsMetric
in the layout provider for this.

[varkha, 2017/04/28 06:21:19]

Acknowledged.

[varkha, 2017/05/01 10:11:59]

Done.

+
+    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(kBubbleContentsInsets);
+  }
   grid_layout->StartRow(0, 0);
 
   // If this zoom change was initiated by an extension, that extension will be
@@ -208,25 +227,44 @@ void ZoomBubbleView::Init() {
   }
 
   // Add zoom label with the new zoom percent.
-  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());
+  label_ = new views::Label();
+  UpdateZoomPercent();
+  if (material) {
+    zoom_out_button_ = CreateZoomButton(kRemoveIcon, IDS_ACCNAME_ZOOM_MINUS2);
+    grid_layout->AddView(zoom_out_button_);
+
+    label_->SetEnabledColor(GetNativeTheme()->GetSystemColor(
+        ui::NativeTheme::kColorId_ButtonEnabledColor));

[Peter Kasting, 2017/04/28 05:58:48]

The latest Harmony mock I see shows this in normal text color, which is what I'd
expect.  Using a button color for a label is surprising.

[varkha, 2017/04/28 06:21:19]

Acknowledged. On Mac the labels are all muted gray so I was trying to preserve
that
I guess.

[Peter Kasting, 2017/04/28 06:47:26]

What do you mean by "on Mac the labels are all muted gray"?  Are you referring
to the screenshot in the bug?  That is from an outdated mock and should
basically be ignored at this point.

[varkha, 2017/05/01 10:12:00]

Done.

+  } 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_ = CreateZoomButton(kAddIcon, 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) {
+    // TODO(varkha): When cleaning up pre-material code path and
+    // IDS_ZOOM_SET_DEFAULT is not used as the button text anymore, rename the
+    // IDS and change the string to a more explanatory version, e.g. "Reset to
+    // default zoom level".
+    reset_button_->SetTooltipText(
+        l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT));
+  }
+  grid_layout->AddView(reset_button_);
 
   StartTimerIfNecessary();
 }
@@ -239,6 +277,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 +289,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);

[Peter Kasting, 2017/04/28 05:58:48]

Is this so if the timer is already running, it won't do anything?  If so, why
not just cancel the timer here instead of adding this variable?

[varkha, 2017/04/28 06:21:19]

No, this is to skip over 100% without closing the dialog. I think any button
interaction here should never result in the bubble getting closed so I've
added this here rather than plumbing do_not_close_me variable up and
down.

[Peter Kasting, 2017/04/28 06:47:26]

My brain hurts trying to understand how closing works because we now seem to
have multiple levels of callbacks, guarding each other's functionality using
auto-resetting boolean members.  Can you find a more direct way of plumbing
things?  I'm sorry, it's too late at night for me to have concrete suggestions
on precisely how to do this.

[varkha, 2017/05/01 10:11:59]

I've added a comment explaining the reasoning. I will look some more if it is
possible to make something that doesn't complicate things too much but this
actually seemed to me more understandable than passing an additional UI-related
parameter via multiple levels of stack.
e.g. Browser::OnZoomChanged would need an additional field (on top of existing
data.can_show_bubble) to mark when zoom is changing via the bubble buttons.
The chain of invocations currently leading to this is quite wide so this could
mushroom in something much larger. Certainly seems like at least a separate
task.

+  auto_close_ = false;

[Peter Kasting, 2017/04/28 05:58:48]

Won't this mean once you click a button the bubble never auto-hides?  Seems like
clicking a button should just reset the timer (to a longer value like 5
seconds).

[varkha, 2017/04/28 06:21:19]

Yes. This is similar to ET_GESTURE_TAP handling that also blocks auto_close. I
wasn't expecting a dialog to close on me after interacting with it, especially
with the
buttons that invite repetition like +/- where repetition may be rare for some
people
and any auto-close timer might come when a user is about to press or tap +/-
again.

[Peter Kasting, 2017/04/28 06:47:26]

Stopping auto-close permanently is potentially annoying.  If you think the timer
reset idea I have (using a longer timeout) is not the right way to go, I would
bring this up with hwi.

[varkha, 2017/05/01 10:12:00]

I've played a bit more with it and I find that 5 seconds is long enough to still
be
comfortable for repeated presses. Done.

+  StopTimer();
   if (sender == image_button_) {
     DCHECK(extension_info_.icon_image) << "Invalid button press.";
     Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
@@ -255,8 +299,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();
   }
 }
 
@@ -319,6 +369,27 @@ void ZoomBubbleView::SetExtensionInfo(const extensions::Extension* extension) {
                                 this));
 }
 
+views::Button* ZoomBubbleView::CreateZoomButton(const gfx::VectorIcon& icon,

[Peter Kasting, 2017/04/28 05:58:48]

Nit: It would be nice if this returned a unique_ptr to make it clear it was
transferring ownership back.

[varkha, 2017/04/28 06:21:19]

Acknowledged.

[varkha, 2017/05/01 10:12:00]

Done.

+                                                int tooltip_id) {
+  views::ImageButton* button = views::CreateVectorImageButton(this);
+  views::SetImageFromVectorIcon(button, icon);
+  button->SetFocusForPlatform();
+  button->SetTooltipText(l10n_util::GetStringUTF16(tooltip_id));
+  return button;
+}
+
+void ZoomBubbleView::UpdateZoomPercent() {
+  zoom::ZoomController* zoom_controller =
+      zoom::ZoomController::FromWebContents(web_contents_);
+  const int zoom_percent = zoom_controller->GetZoomPercent();
+  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);
+}
+
 void ZoomBubbleView::StartTimerIfNecessary() {
   if (!auto_close_)
     return;