[Mac] Don't show tooltip when zoom decoration is hidden.

[Mac] Don't show tooltip when zoom decoration is hidden.

In Material Design the zoom decoration is hidden when the zoom factor is
100%. When the zoom decoration's tooltip should only be visible when the
decoration is visible.

R=avi@chromium.org, dbeam@chromium.org
BUG=635480
Committed: https://crrev.com/c12fa88b187817e55ad6ccfc0416aec46cfd8314
Cr-Commit-Position: refs/heads/master@{#425389}

[shrike, 2016-10-12 19:29:56]

PTAL

[Avi (use Gerrit), 2016-10-12 19:40:10]

LGTM with fix.

https://codereview.chromium.org/2415453004/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://codereview.chromium.org/2415453004/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:107:
tooltip_.reset(vector_icon_id_ == gfx::VectorIconId::VECTOR_ICON_NONE
It's equivalent, but it feels better to do:

tooltip_.reset(relative_zoom == zoom::ZoomController::ZOOM_AT_DEFAULT_ZOOM
 ? @""
 : [tooltip_string retain]);

where we're gating on the zoom controller's response rather than an id
interpretation of it.

[shrike, 2016-10-12 20:43:36]

https://codereview.chromium.org/2415453004/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://codereview.chromium.org/2415453004/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:107:
tooltip_.reset(vector_icon_id_ == gfx::VectorIconId::VECTOR_ICON_NONE
On 2016/10/12 19:40:10, Avi wrote:
> It's equivalent, but it feels better to do:
> 
> tooltip_.reset(relative_zoom == zoom::ZoomController::ZOOM_AT_DEFAULT_ZOOM
>  ? @""
>  : [tooltip_string retain]);
> 
> where we're gating on the zoom controller's response rather than an id
> interpretation of it.

I like checking the icon id because it's explicit that no vector icon means no
tool tip either, but I'm fine to change it.

[shrike, 2016-10-12 20:43:50]

The CQ bit was checked by shrike@chromium.org

[shrike, 2016-10-12 20:43:51]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2415453004/#ps20001
(title: "Address review feedback.")

[commit-bot: I haz the power, 2016-10-12 20:44:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 22:02:32]

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

[commit-bot: I haz the power, 2016-10-12 22:02:33]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shrike, 2016-10-12 23:47:19]

UpdateIfNecessary() only calls ShowAndUpdateUI() if necessary, and part of
how it figures this out is by checking the current tooltip string. In the
previous patchset, UpdateIfNecessary() called ShowAndUpdateUI()
with a tooltip string for 100% zoom, but ShowAndUpdateUI() set the tooltip
string to @"". As a result, UpdateIfNecessary() called ShowAndUpdateUI()
when it didn't really need to, which broke a test.

To fix this, UpdateIfNecessary() needs to determine the final tooltip
string and pass that to ShowAndUpdateUI(), rather than have ShowAndUpdateUI()
compute that string.

PTAL

[Avi (use Gerrit), 2016-10-13 01:20:17]

lgtm

[shrike, 2016-10-13 17:07:27]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2016-10-13 17:07:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 18:16:27]

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

[commit-bot: I haz the power, 2016-10-13 18:16:27]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shrike, 2016-10-13 21:29:54]

Description was changed from

==========
[Mac] Don't show tooltip when zoom decoration is hidden.

In Material Design the zoom decoration is hidden when the zoom factor is
100%. When the zoom decoration's tooltip should only be visible when the
decoration is visible.

R=avi@chromium.org
BUG=635480
==========

to

==========
[Mac] Don't show tooltip when zoom decoration is hidden.

In Material Design the zoom decoration is hidden when the zoom factor is
100%. When the zoom decoration's tooltip should only be visible when the
decoration is visible.

R=avi@chromium.org, dbeam@chromium.org
BUG=635480
==========

[shrike, 2016-10-13 21:29:54]

shrike@chromium.org changed reviewers:
+ dbeam@chromium.org

[shrike, 2016-10-13 21:30:50]

avi@ - please take one more look at my changes to get the test to pass.

+dbeam@ for components/zoom/zoom_controller.h

PTAL

[Dan Beam, 2016-10-13 21:34:02]

lgtm w/suggestion

https://codereview.chromium.org/2415453004/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm (right):

https://codereview.chromium.org/2415453004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm:38: bool
IsAtDefaultZoom() const override { return zoom_percent_ == 100; }
nit: you could just make zoom_percent_ = 80/100 into at_default_zoom_ =
true/false

[shrike, 2016-10-13 21:47:48]

avi@ - hold off on reviewing anything. I need to make more changes.

[shrike, 2016-10-13 22:22:34]

OK, PTAL

[Avi (use Gerrit), 2016-10-13 22:26:55]

lgtm

[shrike, 2016-10-14 17:19:55]

The CQ bit was checked by shrike@chromium.org

[shrike, 2016-10-14 17:19:56]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2415453004/#ps80001
(title: "More fixes.")

[commit-bot: I haz the power, 2016-10-14 17:20:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 18:22:23]

Description was changed from

==========
[Mac] Don't show tooltip when zoom decoration is hidden.

In Material Design the zoom decoration is hidden when the zoom factor is
100%. When the zoom decoration's tooltip should only be visible when the
decoration is visible.

R=avi@chromium.org, dbeam@chromium.org
BUG=635480
==========

to

==========
[Mac] Don't show tooltip when zoom decoration is hidden.

In Material Design the zoom decoration is hidden when the zoom factor is
100%. When the zoom decoration's tooltip should only be visible when the
decoration is visible.

R=avi@chromium.org, dbeam@chromium.org
BUG=635480
==========

[commit-bot: I haz the power, 2016-10-14 18:22:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-14 18:25:33]

Description was changed from

==========
[Mac] Don't show tooltip when zoom decoration is hidden.

In Material Design the zoom decoration is hidden when the zoom factor is
100%. When the zoom decoration's tooltip should only be visible when the
decoration is visible.

R=avi@chromium.org, dbeam@chromium.org
BUG=635480
==========

to

==========
[Mac] Don't show tooltip when zoom decoration is hidden.

In Material Design the zoom decoration is hidden when the zoom factor is
100%. When the zoom decoration's tooltip should only be visible when the
decoration is visible.

R=avi@chromium.org, dbeam@chromium.org
BUG=635480

Committed: https://crrev.com/c12fa88b187817e55ad6ccfc0416aec46cfd8314
Cr-Commit-Position: refs/heads/master@{#425389}
==========

[commit-bot: I haz the power, 2016-10-14 18:25:34]

Patchset 5 (id:??) landed as
https://crrev.com/c12fa88b187817e55ad6ccfc0416aec46cfd8314
Cr-Commit-Position: refs/heads/master@{#425389}