Update location bar bubble/icon behavior.

Update location bar bubble/icon behavior.

- the icon is never blue, only grey, except the star is sometimes blue.
- only show arrow when the bubble has been shown automatically. On user action, no arrow.
- share more code and make dismiss behavior more consistent across bubbles

This is not controlled by the material design flag so it changes present-day behavior as well.

BUG=552561, 512440
Committed: https://crrev.com/e191b197ccb4177de1ac0c40371b305a68616b2a
Cr-Commit-Position: refs/heads/master@{#359231}

[Evan Stade, 2015-11-09 22:59:41]

Description was changed from

==========
Update location bar bubble/icon behavior.

- the icon is never blue, only grey, except the star is sometimes blue.
- only show arrow when the bubble has been shown automatically. On user action,
no arrow.
- share more code and make dismiss behavior more consistent across bubbles

BUG=552561, 512440
==========

to

==========
Update location bar bubble/icon behavior.

- the icon is never blue, only grey, except the star is sometimes blue.
- only show arrow when the bubble has been shown automatically. On user action,
no arrow.
- share more code and make dismiss behavior more consistent across bubbles

This is not controlled by the material design flag so it changes present-day
behavior as well.

BUG=552561, 512440
==========

[Evan Stade, 2015-11-09 23:00:05]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org, sky@chromium.org

[Evan Stade, 2015-11-09 23:00:05]

pkasting: all
sky:
  chrome/test/base/test_browser_window.cc
  chrome/test/base/test_browser_window.h

[Evan Stade, 2015-11-09 23:00:10]

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

[commit-bot: I haz the power, 2015-11-09 23:03:45]

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

[commit-bot: I haz the power, 2015-11-09 23:58:31]

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

[commit-bot: I haz the power, 2015-11-09 23:58:32]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Peter Kasting, 2015-11-10 00:24:18]

LGTM

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:31: void
ShowBubble(bool user_action);
Various functions in this CL use "user_action", "is_user_gesture", and
"user_gesture" for this; pick one and use it consistently.

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:86:
GetWidget()->Close();
Seems like this should be calling Close() instead.

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h
(right):

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:25:
enum DisplayReason { AUTOMATIC, USER_ACTION };
I'm not opposed to adding this, but it seems like if it's worth adding (versus
just using a bool), we should at least use it in every subclass of this (e.g.
SaveCardBubbleViews::Show()), if not everywhere in the CL.

This would also be useful in replacing some similar one-off ways we already
record this kind of thing, e.g. ManagePasswordsBubbleModel::AUTOMATIC, though I
haven't looked closely to ensure it's possible to refer to this everywhere
necessary.

Also, you might want to clarify here what these mean.  For example, if the
translate prompt is showing, and I click to "translate" the current page, and as
a result the "this page has been translated" bubble appears, is that a "user
action" or "automatic"?  "User action" could mean "icon clicked", or it could
mean "bubble shown during processing of some sort of user action somewhere in
the browser", or something in between.  (If it's the first, BTW, maybe
ICON_CLICKED would be a better enum value?)

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:31: //
Displays the bubble as a result of |reason|.
You might want to note what we do differently in these cases.  For example, I
didn't realize that the bubble's active state (not just its appearance) differed
between the two.

[sky, 2015-11-10 00:57:44]

test_browser_window* LGTM

[Evan Stade, 2015-11-10 19:18:26]

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:31: void
ShowBubble(bool user_action);
On 2015/11/10 00:24:17, Peter Kasting wrote:
> Various functions in this CL use "user_action", "is_user_gesture", and
> "user_gesture" for this; pick one and use it consistently.

both of these equivalent terms already exist before this CL, but I changed all
new things to "gesture"

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:86:
GetWidget()->Close();
On 2015/11/10 00:24:17, Peter Kasting wrote:
> Seems like this should be calling Close() instead.

Done.

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h
(right):

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:25:
enum DisplayReason { AUTOMATIC, USER_ACTION };
On 2015/11/10 00:24:18, Peter Kasting wrote:
> I'm not opposed to adding this, but it seems like if it's worth adding (versus
> just using a bool), we should at least use it in every subclass of this (e.g.
> SaveCardBubbleViews::Show()), if not everywhere in the CL.
> 
> This would also be useful in replacing some similar one-off ways we already
> record this kind of thing, e.g. ManagePasswordsBubbleModel::AUTOMATIC, though
I
> haven't looked closely to ensure it's possible to refer to this everywhere
> necessary.
> 
> Also, you might want to clarify here what these mean.  For example, if the
> translate prompt is showing, and I click to "translate" the current page, and
as
> a result the "this page has been translated" bubble appears, is that a "user
> action" or "automatic"?  "User action" could mean "icon clicked", or it could
> mean "bubble shown during processing of some sort of user action somewhere in
> the browser", or something in between.  (If it's the first, BTW, maybe
> ICON_CLICKED would be a better enum value?)

Done, but I left the enum value as USER_GESTURE to match all the various call
sites for bubbles.

https://codereview.chromium.org/1402363013/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:31: //
Displays the bubble as a result of |reason|.
On 2015/11/10 00:24:18, Peter Kasting wrote:
> You might want to note what we do differently in these cases.  For example, I
> didn't realize that the bubble's active state (not just its appearance)
differed
> between the two.

I don't like writing this type of comment because what's true today is not true
tomorrow, and there's no compiler for comments to make sure they stay correct.
The implementation is really short and easy to read, so that is the best
documentation. But I tried to improve this comment.

[Evan Stade, 2015-11-11 22:47:10]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-11-11 22:47:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1402363013/#ps40001
(title: "fix browser tests")

[commit-bot: I haz the power, 2015-11-11 22:48:26]

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

[commit-bot: I haz the power, 2015-11-11 22:53:06]

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

[commit-bot: I haz the power, 2015-11-11 22:53:07]

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

[Evan Stade, 2015-11-12 00:04:05]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-11-12 00:04:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1402363013/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-12 00:08:03]

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

[commit-bot: I haz the power, 2015-11-12 03:20:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-12 20:03:52]

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