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/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/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;
 
 // 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 77 matching lines @@
 
 // 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)));
+  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();
 }
 
 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),
       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 24 matching lines @@
   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);
+  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
   // 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_);
   }
 
   // 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());
+  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();
 }
 
 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();
 }
 
 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_);
     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 65 matching lines @@
   }
 }
 
 void ZoomBubbleView::StopTimer() {
   timer_.Stop();
 }
 
 ZoomBubbleView::ZoomBubbleExtensionInfo::ZoomBubbleExtensionInfo() {}
 
 ZoomBubbleView::ZoomBubbleExtensionInfo::~ZoomBubbleExtensionInfo() {}