Updates Zoom bubble layout and adds +/- buttons

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

 // Copyright (c) 2012 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/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"
 #include "chrome/browser/ui/browser_finder.h"
 #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"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/zoom/page_zoom.h"
 #include "components/zoom/zoom_controller.h"
 #include "content/public/browser/notification_source.h"
 #include "extensions/browser/extension_zoom_request_client.h"
 #include "extensions/common/manifest_handlers/icons_handler.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/material_design/material_design_controller.h"
 #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;
 
 // static
 void ZoomBubbleView::ShowBubble(content::WebContents* web_contents,
                                 const gfx::Point& anchor_point,
                                 DisplayReason reason) {
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
   DCHECK(browser && browser->window() &&
          browser->exclusive_access_manager()->fullscreen_controller());
@@ skipping 74 matching lines @@
   if (zoom_bubble_)
     zoom_bubble_->CloseBubble();
 }
 
 // static
 ZoomBubbleView* ZoomBubbleView::GetZoomBubble() {
   return zoom_bubble_;
 }
 
 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();
 }
 
 ZoomBubbleView::ZoomBubbleView(
     views::View* anchor_view,
     const gfx::Point& anchor_point,
     content::WebContents* web_contents,
     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_)
     immersive_mode_controller_->AddObserver(this);
   UseCompactMargins();
 }
 
 ZoomBubbleView::~ZoomBubbleView() {
   if (immersive_mode_controller_)
     immersive_mode_controller_->RemoveObserver(this);
@@ skipping 12 matching lines @@
 
 void ZoomBubbleView::OnMouseEntered(const ui::MouseEvent& event) {
   StopTimer();
 }
 
 void ZoomBubbleView::OnMouseExited(const ui::MouseEvent& event) {
   StartTimerIfNecessary();
 }
 
 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.
   if (extension_info_.icon_image) {
     image_button_ = new views::ImageButton(this);
     image_button_->SetTooltipText(
         l10n_util::GetStringFUTF16(IDS_TOOLTIP_ZOOM_EXTENSION_ICON,
                                    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_);
+  label_ = new views::Label();
+  UpdateZoomPercent();
+  AddChildView(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 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 to Default" button.
-  grid_layout->AddView(
-      views::MdTextButton::CreateSecondaryUiButton(
-          this, l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT)));
+  // 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();
 }
 
 void ZoomBubbleView::WindowClosing() {
   // |zoom_bubble_| can be a new bubble by this point (as Close(); doesn't
   // call this right away). Only set to nullptr when it's this bubble.
   if (zoom_bubble_ == this)
     zoom_bubble_ = nullptr;
 }
 
 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;
   web_contents_ = nullptr;
   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_);
     chrome::AddSelectedTabWithURL(
         browser, GURL(base::StringPrintf("chrome://extensions?id=%s",
                                          extension_info_.id.c_str())),
         ui::PAGE_TRANSITION_FROM_API);
+  } 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 {
-    zoom::PageZoom::Zoom(web_contents_, content::PAGE_ZOOM_RESET);
+    NOTREACHED();
   }
 }
 
 void ZoomBubbleView::OnImmersiveRevealStarted() {
   CloseBubble();
 }
 
 void ZoomBubbleView::OnImmersiveModeControllerDestroyed() {
   immersive_mode_controller_ = nullptr;
 }
@@ skipping 42 matching lines @@
   icon_size = browser_action->default_icon.map().begin()->first;
   extension_info_.icon_image.reset(
       new extensions::IconImage(web_contents_->GetBrowserContext(),
                                 extension,
                                 browser_action->default_icon,
                                 icon_size,
                                 default_extension_icon_image,
                                 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;
 
   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);
   }
 }
 
 void ZoomBubbleView::StopTimer() {
   timer_.Stop();
 }
 
 ZoomBubbleView::ZoomBubbleExtensionInfo::ZoomBubbleExtensionInfo() {}
 
 ZoomBubbleView::ZoomBubbleExtensionInfo::~ZoomBubbleExtensionInfo() {}