zoom bubble: Close if anchor is clicked while bubble is showing.

zoom bubble: Close if anchor is clicked while bubble is showing.

R=msw@chromium.org
BUG=366092
Committed: https://crrev.com/8621073547c0a06b5a260185200da6c537175f93
Cr-Commit-Position: refs/heads/master@{#301043}

[msw, 2014-07-24 22:07:14]

https://codereview.chromium.org/420533002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_view.h (right):

https://codereview.chromium.org/420533002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_view.h:42: // Hide the zoom bubble if
it's showing. Show the zoom bubble if it's hidden.
It seems like ZoomView should be a BubbleIconView ("Represents an icon on the
omnibox that shows a bubble when clicked."). Then it'd get the toggle behavior
more cheaply via |suppress_mouse_released_action_|, and be more consistent with
StarView, TranslateIconView, and ManagePasswordsIconView. WDYT?

[Dan Beam, 2014-10-23 23:31:20]

Patchset #2 (id:80001) has been deleted

[Dan Beam, 2014-10-23 23:32:46]

Patchset #2 (id:100001) has been deleted

[Dan Beam, 2014-10-23 23:34:03]

-30 LOC :)

https://codereview.chromium.org/420533002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_view.h (right):

https://codereview.chromium.org/420533002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_view.h:42: // Hide the zoom bubble if
it's showing. Show the zoom bubble if it's hidden.
On 2014/07/24 22:07:13, msw wrote:
> It seems like ZoomView should be a BubbleIconView ("Represents an icon on the
> omnibox that shows a bubble when clicked."). Then it'd get the toggle behavior
> more cheaply via |suppress_mouse_released_action_|, and be more consistent
with
> StarView, TranslateIconView, and ManagePasswordsIconView. WDYT?

Done.

[msw, 2014-10-24 00:00:56]

Sweet, better late than never!

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:11:
BubbleIconView::BubbleIconView() : command_updater_(nullptr), command_id_(0) {
This ctor should also SetAccessibilityFocusable(true);

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right):

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:21: BubbleIconView();
Remove this, ZoomBubbleView can just pass BubbleIconView(nullptr, 0).
(bonus points if you wanted to actually hook up IDC_ZOOM_MENU)

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:23: virtual
~BubbleIconView();
optional nit: virtual -> override here and below.

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:43: // Executes the
given |command_id_| with |command_updater_| if specified when
optional nit: one-liner "// Calls OnExecuting and runs |command_id_| with a
valid |command_updater_|."

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_view.h (right):

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_view.h:22: virtual ~ZoomView();
nit: remove "virtual" keywords here and below, use "override".
See this guide for more info: http://chromium-cpp.appspot.com/

[Dan Beam, 2014-10-24 00:26:19]

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:11:
BubbleIconView::BubbleIconView() : command_updater_(nullptr), command_id_(0) {
On 2014/10/24 00:00:55, msw wrote:
> This ctor should also SetAccessibilityFocusable(true);

meh, just removed this ctor

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right):

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:21: BubbleIconView();
On 2014/10/24 00:00:55, msw wrote:
> Remove this, ZoomBubbleView can just pass BubbleIconView(nullptr, 0).

Done.

> (bonus points if you wanted to actually hook up IDC_ZOOM_MENU)

i'm hesitant to do this as it's already used in the flapjacks menu

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:23: virtual
~BubbleIconView();
On 2014/10/24 00:00:55, msw wrote:
> optional nit: virtual -> override here and below.

Done.

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:43: // Executes the
given |command_id_| with |command_updater_| if specified when
On 2014/10/24 00:00:55, msw wrote:
> optional nit: one-liner "// Calls OnExecuting and runs |command_id_| with a
> valid |command_updater_|."

Done.

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_view.h (right):

https://codereview.chromium.org/420533002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_view.h:22: virtual ~ZoomView();
On 2014/10/24 00:00:56, msw wrote:
> nit: remove "virtual" keywords here and below, use "override".
> See this guide for more info: http://chromium-cpp.appspot.com/

Done. (mind == blown)

[msw, 2014-10-24 00:39:48]

Awesome; lgtm with a nit!

https://codereview.chromium.org/420533002/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

https://codereview.chromium.org/420533002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_view.cc:42: void
ZoomView::GetAccessibleState(ui::AXViewState* state) {
nit: make the definition order match the decl order.

[Dan Beam, 2014-10-24 00:44:19]

https://codereview.chromium.org/420533002/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

https://codereview.chromium.org/420533002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_view.cc:42: void
ZoomView::GetAccessibleState(ui::AXViewState* state) {
On 2014/10/24 00:39:48, msw wrote:
> nit: make the definition order match the decl order.

Done.

[Dan Beam, 2014-10-24 00:44:24]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-10-24 00:46:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/420533002/180001

[commit-bot: I haz the power, 2014-10-24 01:58:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-24 01:58:12]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2014-10-24 02:42:04]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-10-24 02:43:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/420533002/200001

[commit-bot: I haz the power, 2014-10-24 03:54:16]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2014-10-24 03:55:18]

Patchset 6 (id:??) landed as
https://crrev.com/8621073547c0a06b5a260185200da6c537175f93
Cr-Commit-Position: refs/heads/master@{#301043}