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 that the bubble should stay on the screen if it will close
+// automatically.
+constexpr base::TimeDelta kBubbleCloseDelayDefault =
+    base::TimeDelta::FromMilliseconds(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.
+constexpr base::TimeDelta kBubbleCloseDelayLong =
+    base::TimeDelta::FromMilliseconds(5000);
+
+// Creates an ImageButton using vector |icon|, sets a tooltip with |tooltip_id|.
+// Returns the button.
+std::unique_ptr<views::Button> CreateZoomButton(views::ButtonListener* listener,
+                                                const gfx::VectorIcon& icon,
+                                                int tooltip_id) {
+  std::unique_ptr<views::ImageButton> button(
+      views::CreateVectorImageButton(listener));
+  views::SetImageFromVectorIcon(button.get(), icon);
+  button->SetFocusForPlatform();
+  button->SetTooltipText(l10n_util::GetStringUTF16(tooltip_id));
+  return button;
+}
+
+class ZoomValue : public views::Label {
+ public:
+  explicit ZoomValue(const content::WebContents* web_contents) {
+    SetText(base::FormatPercent(web_contents->GetMaximumZoomPercent()));

[Peter Kasting, 2017/05/16 00:14:35]

I worry a little about this, in the sense that it's not clear to me the maximum
zoom percent will always be the maximum width string.  The max percent is
currently 500, and if, say, "4" is 1 px wider than "5" in the user's font, then
a 400% zoom will be 1 px wider than a 500%.

The safest thing to do is probably to actually iterate through the zoom levels
(at least the ones at default and above) and get the width for each, then use
the max of all those.  There should be a small number of these so this should
still be fast.

[tapted, 2017/05/16 06:29:01]

true it would be safer. Although I suspect it would be unusual for any digital
UI font to use non-"tabular" [1] numerals. Even for languages like Persian,
which don't use western-arabic numerals.


[1]
https://www.fonts.com/content/learning/fontology/level-3/numbers/proportional...

[tapted, 2017/05/16 06:29:02]

this should also take a font style (it currently works out the same, but it's
good to be explicit, and if UX later decide to refine the mock to have a
different style, we'll know where to look). The best way of doing this will be
to call the Label constructor in an initializer list

explicit ZoomValue(const content::WebContents* web_contents)
    : Label(base::FormatPercent(web_contents->GetMaximumZoomPercent()),
            views::style::CONTEXT_LABEL,
            views::style::STYLE_PRIMARY),
      preferred_size_(Label::GetPreferredSize()) {
  SetText(base::string16());
}

I guess preferred_size_ could be const then too (edit: unless we need to
iterate)

[varkha, 2017/05/16 22:53:06]

This is something we also do in [1] (we also cache the value there but we don't
need that here since this is only invoked once).

[1] -

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/app_menu...

[varkha, 2017/05/16 22:53:07]

Done.

[varkha, 2017/05/16 22:53:07]

Acknowledged.

+    preferred_size_ = views::Label::GetPreferredSize();

[tapted, 2017/05/16 06:29:01]

I tend to drop the views:: prefix (inheriting effectively acts as a `using
views::Label;` directive)

[varkha, 2017/05/16 22:53:06]

Acknowledged.

+    SetText(base::string16());
+  }
+  ~ZoomValue() override{};

[tapted, 2017/05/16 06:29:02]

nit: space before {}, no semicolon after

[varkha, 2017/05/16 22:53:06]

Done.

+
+  gfx::Size GetPreferredSize() const override { return preferred_size_; }

[tapted, 2017/05/16 06:29:02]

nit: // Label:

[varkha, 2017/05/16 22:53:06]

Done.

+
+ private:
+  gfx::Size preferred_size_;
+
+  DISALLOW_COPY_AND_ASSIGN(ZoomValue);
+};
+
+}  // 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),
+      auto_close_duration_(kBubbleCloseDelayDefault),
       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, margin);
+
+  box_layout->set_main_axis_alignment(
+      views::BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER);
+  box_layout->set_cross_axis_alignment(
+      views::BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER);
+  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(this, 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 ZoomValue(web_contents_);
+  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.
+  std::unique_ptr<views::Button> zoom_in_button =
+      CreateZoomButton(this, kAddIcon, IDS_ACCNAME_ZOOM_PLUS2);
+  zoom_in_button_ = zoom_in_button.get();
+  AddChildView(zoom_in_button.release());
 
-  // 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);

[Peter Kasting, 2017/05/16 00:14:35]

This call shouldn't be necessary, I don't think.

[varkha, 2017/05/16 22:53:06]

Done.

 
   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 {

[Peter Kasting, 2017/05/16 00:14:35]

You no longer need to provide this -- Elly's recently-landed change should mean
you get this behavior by default.

[varkha, 2017/05/16 22:53:06]

Done.

+  return false;
+}
+
 void ZoomBubbleView::ButtonPressed(views::Button* sender,
                                    const ui::Event& event) {
+  // No buttons presses in this dialog should cause the dialog to close,

[tapted, 2017/05/16 06:29:01]

"No buttons pressed" or "No button presses" or "A button press.. should not"

[varkha, 2017/05/16 22:53:06]

Done.

+  // 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.
+  auto_close_duration_ = kBubbleCloseDelayLong;
+  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));
 }
 
+void ZoomBubbleView::UpdateZoomPercent() {
+  label_->SetText(base::FormatPercent(
+      zoom::ZoomController::FromWebContents(web_contents_)->GetZoomPercent()));
+}
+
 void ZoomBubbleView::StartTimerIfNecessary() {
   if (!auto_close_)
     return;
 
-  if (timer_.IsRunning()) {
-    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, auto_close_duration_, this,
+               &ZoomBubbleView::CloseBubble);
 }
 
 void ZoomBubbleView::StopTimer() {
   timer_.Stop();
 }
 
 ZoomBubbleView::ZoomBubbleExtensionInfo::ZoomBubbleExtensionInfo() {}
 
 ZoomBubbleView::ZoomBubbleExtensionInfo::~ZoomBubbleExtensionInfo() {}