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..1a518ed83cc7058437cff56f99c952e304fd1639 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"
@@ -14,6 +16,7 @@
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/location_bar/zoom_view.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
@@ -29,13 +32,29 @@
 #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"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
+namespace {
+
+// The default time in milliseconds that the bubble should stay on the screen if
+// it will close automatically.
+const int kBubbleCloseDelayDefaultMs = 1500;
+
+// A longer timeout used for how long the bubble should stay on the screen
+// before it will close automatically after + or - buttons have been used.
+const int kBubbleCloseDelayLongMs = 5000;
+
+}  // namespace
+
 // static
 ZoomBubbleView* ZoomBubbleView::zoom_bubble_ = nullptr;
 
@@ -130,11 +149,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();
 }
 
@@ -145,10 +160,15 @@ ZoomBubbleView::ZoomBubbleView(
     DisplayReason reason,
     ImmersiveModeController* immersive_mode_controller)
     : LocationBarBubbleDelegateView(anchor_view, anchor_point, web_contents),
+      timer_duration_(kBubbleCloseDelayDefaultMs),
       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_)
@@ -181,19 +201,19 @@ void ZoomBubbleView::OnMouseExited(const ui::MouseEvent& event) {
 }
 
 void ZoomBubbleView::Init() {
-  // Set up the layout of the zoom bubble. A grid layout is used because
-  // sometimes an extension icon is shown next to the zoom label.
-  views::GridLayout* grid_layout = new views::GridLayout(this);
-  SetLayoutManager(grid_layout);
-  views::ColumnSet* columns = grid_layout->AddColumnSet(0);
-  // First row.
-  if (extension_info_.icon_image) {
-    columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::CENTER, 2,
-                       views::GridLayout::USE_PREF, 0, 0);
-  }
-  columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                     views::GridLayout::USE_PREF, 0, 0);
-  grid_layout->StartRow(0, 0);
+  // Set up the layout of the zoom bubble.
+  ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
+  const int margin =
+      provider->GetDistanceMetric(views::DISTANCE_RELATED_BUTTON_HORIZONTAL);
+  views::BoxLayout* box_layout =
+      new views::BoxLayout(views::BoxLayout::kHorizontal, margin, 0, 0);
+
+  box_layout->set_main_axis_alignment(
+      views::BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER);
+  box_layout->set_cross_axis_alignment(
+      views::BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER);
+  box_layout->SetDefaultFlex(1);
+  SetLayoutManager(box_layout);
 
   // If this zoom change was initiated by an extension, that extension will be
   // attributed by showing its icon in the zoom bubble.
@@ -204,29 +224,38 @@ void ZoomBubbleView::Init() {
                                    base::UTF8ToUTF16(extension_info_.name)));
     image_button_->SetImage(views::Button::STATE_NORMAL,
                             &extension_info_.icon_image->image_skia());
-    grid_layout->AddView(image_button_);
+    AddChildView(image_button_);
   }
 
+  // Add Zoom Out ("-") button.
+  std::unique_ptr<views::Button> zoom_out_button =
+      CreateZoomButton(kRemoveIcon, IDS_ACCNAME_ZOOM_MINUS2);
+  zoom_out_button_ = zoom_out_button.get();
+  AddChildView(zoom_out_button.release());
+
   // 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());
-  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);
-
-  // Add "Reset to Default" button.
-  grid_layout->AddView(
-      views::MdTextButton::CreateSecondaryUiButton(
-          this, l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT)));
+  label_ = new views::Label();
+  UpdateZoomPercent();
+  AddChildView(label_);
+
+  // Add Zoom In ("+") button with extra padding on the right because "Reset"
+  // button has fixed size.
+  std::unique_ptr<views::Button> zoom_in_button =
+      CreateZoomButton(kAddIcon, IDS_ACCNAME_ZOOM_PLUS2);
+  views::View* zoom_in_button_padding = new views::View();

[tapted, 2017/05/02 01:28:02]

Rather than a container View with a border and layout, would it be nicer to have
a sibling View that overrides GetPreferredSize with the desired padding? 
There's actually a PaddingView in ash/system/tray/system_tray.cc already.

Otherwise... it feels like maybe we should be using GridLayout :/ or add
BoxLayout::SetPaddingForView(..). But I guess this is OK too.

[varkha, 2017/05/03 01:50:27]

Yes, I actually went back and forth on something like this. I think this is
better than
one more override with a fixed size. We have a few of those, they are never big
enough
to warrant factoring them into something reusable and so they end up
multiplying.
I think this is also more explicit. But I don't feel super strongly about it.

+  zoom_in_button_padding->SetLayoutManager(new views::FillLayout());
+  zoom_in_button_padding->SetBorder(views::CreateEmptyBorder(0, 0, 0, margin));
+  zoom_in_button_ = zoom_in_button.get();
+  zoom_in_button_padding->AddChildView(zoom_in_button.release());
+  AddChildView(zoom_in_button_padding);
+
+  // Add "Reset" button.
+  reset_button_ = views::MdTextButton::CreateSecondaryUiButton(
+      this, l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT));
+  reset_button_->SetTooltipText(
+      l10n_util::GetStringUTF16(IDS_ACCNAME_ZOOM_SET_DEFAULT));
+  AddChildView(reset_button_);
+  box_layout->SetFlexForView(reset_button_, 0);
 
   StartTimerIfNecessary();
 }
@@ -239,6 +268,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;
@@ -246,8 +278,26 @@ void ZoomBubbleView::CloseBubble() {
   LocationBarBubbleDelegateView::CloseBubble();
 }
 
+bool ZoomBubbleView::ShouldSnapFrameWidth() const {
+  return false;
+}
+
+gfx::Size ZoomBubbleView::GetPreferredSize() const {
+  const gfx::Size kPreferredZoomBubbleSize = gfx::Size(208, 32);

[tapted, 2017/05/02 01:28:02]

constexpr?  (the geometry classes have the syntax voodoo to make this work I
think)

[varkha, 2017/05/03 01:50:27]

Done.

+  return kPreferredZoomBubbleSize;
+}
+
 void ZoomBubbleView::ButtonPressed(views::Button* sender,
                                    const ui::Event& event) {
+  // No buttons presses in this dialog should cause the dialog to close,
+  // including when the zoom level is set to 100% as a result of a button press.
+  base::AutoReset<bool> auto_ignore_close_bubble(&ignore_close_bubble_, true);
+
+  // Extend the timer to give a user more time after any button is pressed.
+  StopTimer();
+  timer_duration_ = kBubbleCloseDelayLongMs;
+  StartTimerIfNecessary();
+
   if (sender == image_button_) {
     DCHECK(extension_info_.icon_image) << "Invalid button press.";
     Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
@@ -255,8 +305,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 +375,22 @@ void ZoomBubbleView::SetExtensionInfo(const extensions::Extension* extension) {
                                 this));
 }
 
+std::unique_ptr<views::Button> ZoomBubbleView::CreateZoomButton(
+    const gfx::VectorIcon& icon,
+    int tooltip_id) {
+  std::unique_ptr<views::ImageButton> button(
+      views::CreateVectorImageButton(this));
+  views::SetImageFromVectorIcon(button.get(), icon);
+  button->SetFocusForPlatform();
+  button->SetTooltipText(l10n_util::GetStringUTF16(tooltip_id));
+  return button;
+}
+
+void ZoomBubbleView::UpdateZoomPercent() {
+  label_->SetText(base::FormatPercent(
+      zoom::ZoomController::FromWebContents(web_contents_)->GetZoomPercent()));
+}
+
 void ZoomBubbleView::StartTimerIfNecessary() {
   if (!auto_close_)
     return;
@@ -326,12 +398,8 @@ void ZoomBubbleView::StartTimerIfNecessary() {
   if (timer_.IsRunning()) {

[tapted, 2017/05/02 01:28:02]

I don't think this case adds anything. Timer::Start(..) is literally
`AssignMembers() && Reset()`, so we can remove this and the call to StopTimer()
in ButtonPressed().

[varkha, 2017/05/03 01:50:27]

Done.

     timer_.Reset();
   } else {
-    // The number of milliseconds the bubble should stay on the screen if it
-    // will close automatically.
-    const int kBubbleCloseDelay = 1500;
-    timer_.Start(FROM_HERE,
-                 base::TimeDelta::FromMilliseconds(kBubbleCloseDelay), this,
-                 &ZoomBubbleView::CloseBubble);
+    timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(timer_duration_),
+                 this, &ZoomBubbleView::CloseBubble);
   }
 }