Corrects ContentSettingBubbleContents to show bubble with arrow when animation is paused

Corrects ContentSettingBubbleContents to show bubble with arrow when animation is paused.

BUG=518941
Committed: https://crrev.com/b4b7bbadfa900c542a4c2c7c495969d7864d6060
Cr-Commit-Position: refs/heads/master@{#379728}

[varkha, 2016-03-04 19:48:47]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 19:49:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762373002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762373002/1

[varkha, 2016-03-04 19:49:52]

varkha@chromium.org changed reviewers:
+ estade@chromium.org

[varkha, 2016-03-04 19:49:53]

estade@, Did you have something like this in mind when you commented here?
https://codereview.chromium.org/1762163002/diff/20001/chrome/browser/ui/views...

[commit-bot: I haz the power, 2016-03-04 20:00:25]

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

[commit-bot: I haz the power, 2016-03-04 20:00:26]

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

[varkha, 2016-03-04 20:23:43]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 20:24:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762373002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762373002/20001

[commit-bot: I haz the power, 2016-03-04 20:42:39]

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

[commit-bot: I haz the power, 2016-03-04 20:42:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[varkha, 2016-03-04 22:26:31]

Patchset #2 (id:20001) has been deleted

[varkha, 2016-03-04 22:26:42]

Patchset #1 (id:1) has been deleted

[varkha, 2016-03-04 22:27:23]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 22:28:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762373002/40001

[commit-bot: I haz the power, 2016-03-04 23:23:15]

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

[commit-bot: I haz the power, 2016-03-04 23:23:16]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-03-06 01:59:03]

Description was changed from

==========
Makes ContentSettingBubbleContents a subclass of LocationBarBubbleDelegateView

BUG=None
==========

to

==========
Moves a bubble widget creation and show into
ContentSettingBubbleContents::Show().

BUG=None
==========

[Evan Stade, 2016-03-07 18:10:37]

issue title and description mismatch is confusing

https://codereview.chromium.org/1762373002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

https://codereview.chromium.org/1762373002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:259:
views::Widget* bubble_widget = bubble_view_->Show(pause_animation_);
seems like there is a behavior change here. The arrow used to never show in MD,
now it depends on pause_animation_?

[varkha, 2016-03-07 18:28:51]

https://codereview.chromium.org/1762373002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

https://codereview.chromium.org/1762373002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:259:
views::Widget* bubble_widget = bubble_view_->Show(pause_animation_);
On 2016/03/07 18:10:37, Evan Stade wrote:
> seems like there is a behavior change here. The arrow used to never show in
MD,
> now it depends on pause_animation_?

Yes, since there is no ink ripple active state when a label is shown during a
paused animation.

[Evan Stade, 2016-03-07 18:37:43]

On 2016/03/07 18:28:51, varkha wrote:
>
https://codereview.chromium.org/1762373002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/location_bar/content_setting_image_view.cc
(right):
> 
>
https://codereview.chromium.org/1762373002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/location_bar/content_setting_image_view.cc:259:
> views::Widget* bubble_widget = bubble_view_->Show(pause_animation_);
> On 2016/03/07 18:10:37, Evan Stade wrote:
> > seems like there is a behavior change here. The arrow used to never show in
> MD,
> > now it depends on pause_animation_?
> 
> Yes, since there is no ink ripple active state when a label is shown during a
> paused animation.

well in this case I don't think adding a parameter makes it clearer. When it was
unconditional, I thought it belonged in the bubble, but since it depends on
outside state, setting externally is more readable.

[varkha, 2016-03-08 01:53:47]

Description was changed from

==========
Moves a bubble widget creation and show into
ContentSettingBubbleContents::Show().

BUG=None
==========

to

==========
Corrects ContentSettingBubbleContents to show bubble with arrow when animation
is paused.

BUG=518941
==========

[varkha, 2016-03-08 01:54:14]

Makes sense, PTAL.

[varkha, 2016-03-08 01:54:33]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 01:54:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762373002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762373002/60001

[Evan Stade, 2016-03-08 02:12:39]

lgtm

[commit-bot: I haz the power, 2016-03-08 02:42:57]

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

[commit-bot: I haz the power, 2016-03-08 02:42:58]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-03-08 02:54:11]

varkha@chromium.org changed reviewers:
+ pkasting@chromium.org

[varkha, 2016-03-08 02:54:11]

pkasting@chromium.org, can you please take a look for OWNERS in
chrome/browser/ui/views/location_bar/? Once we get the bookmark buttons style
rectangular fill ink drop, maybe this can be corrected to use it but for now I
have restored the arrow only if a bubble gets opened while text label is shown
to match the ink absence.

Thanks!

[Peter Kasting, 2016-03-08 03:16:28]

LGTM

https://codereview.chromium.org/1762373002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

https://codereview.chromium.org/1762373002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:264: // view
doesn't show ink drop so the arrow is still shown.
Nit: Grammar.  How about:

This is triggered by an input event.  If the user clicks the icon while it's not
animating, the icon will be placed in an active state, so the bubble doesn't
need an arrow.  If the user clicks during an animation, the animation simply
pauses and no other visible state change occurs, so show the arrow in this case.

(Though, frankly, I'd almost rather just always show the arrows than have this
kind of complicated logic, but whatever.)

[varkha, 2016-03-08 03:22:13]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-03-08 03:22:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1762373002/#ps80001
(title: "Corrects ContentSettingBubbleContents to show bubble with arrow when
animation is paused (nit)")

[commit-bot: I haz the power, 2016-03-08 03:22:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762373002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762373002/80001

[commit-bot: I haz the power, 2016-03-08 04:06:43]

Description was changed from

==========
Corrects ContentSettingBubbleContents to show bubble with arrow when animation
is paused.

BUG=518941
==========

to

==========
Corrects ContentSettingBubbleContents to show bubble with arrow when animation
is paused.

BUG=518941
==========

[commit-bot: I haz the power, 2016-03-08 04:06:44]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-03-08 04:08:43]

Description was changed from

==========
Corrects ContentSettingBubbleContents to show bubble with arrow when animation
is paused.

BUG=518941
==========

to

==========
Corrects ContentSettingBubbleContents to show bubble with arrow when animation
is paused.

BUG=518941

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

[commit-bot: I haz the power, 2016-03-08 04:08:44]

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