Adjust positioning of location bar icons.

Adjust positioning of location bar icons.

See bug for new layout spec. Note that this affects not just the 
trailing icons but also the location icon ,which is now slightly
closer to the left edge of the location bar.

Test cases:
- location icon, no text (such as an http:// site)
- location icon w/text (such as an https:// site)
- alignment of contents of omnibox popup, both icons and text, with contents of location bar
- keyword hint view, i.e. text on right of location bar when typing a searchable keyword
- selected keyword view, i.e. after pressing tab to search
- content settings icon, e.g. blocked microphone on permission.site
- content settings icon w/text, e.g. a blocked popup on popuptest.com
- multiple location icons, like star view + zoom view
- all of the above in touch mode (--top-chrome-md=material-hybrid)

BUG=678510
Review-Url: https://codereview.chromium.org/2642893002
Cr-Commit-Position: refs/heads/master@{#446199}
Committed: https://chromium.googlesource.com/chromium/src/+/39f5ad11373872bbb7e398fb7943b289cc790c39

[Evan Stade, 2017-01-20 19:55:49]

Description was changed from

==========
WIP - location bar trailing icon layout

BUG=678510
==========

to

==========
Adjust positioning of location bar icons.

See bug for layout spec.

Test cases:
- location icon, no text (such as an http:// site)
- location icon w/text (such as an https:// site)
- keyword hint view, i.e. text on right of location bar when typing a searchable
keyword
- selected keyword view, i.e. after pressing tab to search
- content settings icon, e.g. blocked microphone on permission.site
- content settings icon w/text, e.g. a blocked popup on popuptest.com
- multiple location icons, like star view + zoom view
- all of the above in touch mode (--top-chrome-md=material-hybrid)

BUG=678510
==========

[Evan Stade, 2017-01-20 19:56:23]

Description was changed from

==========
Adjust positioning of location bar icons.

See bug for layout spec.

Test cases:
- location icon, no text (such as an http:// site)
- location icon w/text (such as an https:// site)
- keyword hint view, i.e. text on right of location bar when typing a searchable
keyword
- selected keyword view, i.e. after pressing tab to search
- content settings icon, e.g. blocked microphone on permission.site
- content settings icon w/text, e.g. a blocked popup on popuptest.com
- multiple location icons, like star view + zoom view
- all of the above in touch mode (--top-chrome-md=material-hybrid)

BUG=678510
==========

to

==========
Adjust positioning of location bar icons.

See bug for layout spec.

Test cases:
- location icon, no text (such as an http:// site)
- location icon w/text (such as an https:// site)
- alignment of contents of omnibox popup, both icons and text, with contents of
location bar
- keyword hint view, i.e. text on right of location bar when typing a searchable
keyword
- selected keyword view, i.e. after pressing tab to search
- content settings icon, e.g. blocked microphone on permission.site
- content settings icon w/text, e.g. a blocked popup on popuptest.com
- multiple location icons, like star view + zoom view
- all of the above in touch mode (--top-chrome-md=material-hybrid)

BUG=678510
==========

[Evan Stade, 2017-01-21 02:10:05]

Description was changed from

==========
Adjust positioning of location bar icons.

See bug for layout spec.

Test cases:
- location icon, no text (such as an http:// site)
- location icon w/text (such as an https:// site)
- alignment of contents of omnibox popup, both icons and text, with contents of
location bar
- keyword hint view, i.e. text on right of location bar when typing a searchable
keyword
- selected keyword view, i.e. after pressing tab to search
- content settings icon, e.g. blocked microphone on permission.site
- content settings icon w/text, e.g. a blocked popup on popuptest.com
- multiple location icons, like star view + zoom view
- all of the above in touch mode (--top-chrome-md=material-hybrid)

BUG=678510
==========

to

==========
Adjust positioning of location bar icons.

See bug for new layout spec. Note that this affects not just the 
trailing icons but also the location icon ,which is now slightly
closer to the left edge of the location bar.

Test cases:
- location icon, no text (such as an http:// site)
- location icon w/text (such as an https:// site)
- alignment of contents of omnibox popup, both icons and text, with contents of
location bar
- keyword hint view, i.e. text on right of location bar when typing a searchable
keyword
- selected keyword view, i.e. after pressing tab to search
- content settings icon, e.g. blocked microphone on permission.site
- content settings icon w/text, e.g. a blocked popup on popuptest.com
- multiple location icons, like star view + zoom view
- all of the above in touch mode (--top-chrome-md=material-hybrid)

BUG=678510
==========

[Evan Stade, 2017-01-21 02:10:05]

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

[Evan Stade, 2017-01-21 02:10:12]

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

[commit-bot: I haz the power, 2017-01-21 02:10:28]

Dry run: 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, 2017-01-21 02:25:16]

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

[commit-bot: I haz the power, 2017-01-21 02:25:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-01-21 02:25:58]

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

[Peter Kasting, 2017-01-24 21:26:40]

LGTM

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

https://codereview.chromium.org/2642893002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:99: int image_x =
0;
Doesn't this effectively make |image_x| 0 everywhere below, and thus no longer
necessary?

https://codereview.chromium.org/2642893002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:109: // then we
want to shrink the leading padding down to zero.
This code, and the code in GetSizeForLabelWidth(), were very tricky to write
correctly, and assumed various things about where there would be padding in all
cases.  I don't have this paged in well enough to catch subtle errors here; the
biggest concern I have is with code that is now effectively dead and could be
simplified (a la the |image_x| comment above), but other issues might be
possible, e.g. incorrectly adding padding between the image and label compared
to today.

Please be very careful that all these things are correct, not just in terms of
their output, but that code structure/comments are still right.

https://codereview.chromium.org/2642893002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:231:
image_rect.Inset(-gfx::Insets(LocationBarView::kIconInteriorPadding));
Nit: Simpler:

  if (!image_rect.IsEmpty())
    image_rect.Inset(-gfx::Insets(LocationBarView::kIconInteriorPadding));

(No need for early return)

[Evan Stade, 2017-01-25 00:37:49]

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

https://codereview.chromium.org/2642893002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:99: int image_x =
0;
On 2017/01/24 21:26:39, Peter Kasting wrote:
> Doesn't this effectively make |image_x| 0 everywhere below, and thus no longer
> necessary?

I guess it does. I was worried that width() - image_preferred_width could be <
0, but it's not the case when !ShouldShowLabel.

https://codereview.chromium.org/2642893002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:109: // then we
want to shrink the leading padding down to zero.
On 2017/01/24 21:26:40, Peter Kasting wrote:
> This code, and the code in GetSizeForLabelWidth(), were very tricky to write
> correctly, and assumed various things about where there would be padding in
all
> cases.  I don't have this paged in well enough to catch subtle errors here;
the
> biggest concern I have is with code that is now effectively dead and could be
> simplified (a la the |image_x| comment above), but other issues might be
> possible, e.g. incorrectly adding padding between the image and label compared
> to today.
> 
> Please be very careful that all these things are correct, not just in terms of
> their output, but that code structure/comments are still right.

I too was afraid of touching this more than I had to, but I've tried to simplify
it and verify visually (with slowed down animation) as well as with the existing
unit tests.

https://codereview.chromium.org/2642893002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:231:
image_rect.Inset(-gfx::Insets(LocationBarView::kIconInteriorPadding));
On 2017/01/24 21:26:40, Peter Kasting wrote:
> Nit: Simpler:
> 
>   if (!image_rect.IsEmpty())
>     image_rect.Inset(-gfx::Insets(LocationBarView::kIconInteriorPadding));
> 
> (No need for early return)

Done.

[Evan Stade, 2017-01-25 00:37:53]

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

[commit-bot: I haz the power, 2017-01-25 00:38:45]

Dry run: 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, 2017-01-25 01:22:03]

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

[commit-bot: I haz the power, 2017-01-25 01:22:04]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Peter Kasting, 2017-01-25 04:25:18]

LGTM

https://codereview.chromium.org/2642893002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2642893002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:107: if
(ShouldShowLabel())
The comment above seems to imply this should check IsShrinking() instead.  I
suspect the code here is correct and it's the comment that needs to be clarified
(replace bits about the view expanding/contracting with comments about trying to
show the label).

https://codereview.chromium.org/2642893002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:185: const int
max_width = image_->GetPreferredSize().width() +
Nit: Use |size|?

[Evan Stade, 2017-01-26 00:52:59]

https://codereview.chromium.org/2642893002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2642893002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:107: if
(ShouldShowLabel())
On 2017/01/25 04:25:18, Peter Kasting wrote:
> The comment above seems to imply this should check IsShrinking() instead.  I
> suspect the code here is correct and it's the comment that needs to be
clarified
> (replace bits about the view expanding/contracting with comments about trying
to
> show the label).

thinking of it in terms of expanding vs contracting seemed clearer to me, but
I've updated the comment to cover the cases where there's no current movement
("steady-state").

https://codereview.chromium.org/2642893002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:185: const int
max_width = image_->GetPreferredSize().width() +
On 2017/01/25 04:25:18, Peter Kasting wrote:
> Nit: Use |size|?

Done.

[Evan Stade, 2017-01-26 00:53:04]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-01-26 00:53:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2642893002/#ps100001
(title: "nits")

[commit-bot: I haz the power, 2017-01-26 00:54:18]

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

[Peter Kasting, 2017-01-26 01:25:13]

https://codereview.chromium.org/2642893002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2642893002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:107: if
(ShouldShowLabel())
On 2017/01/26 00:52:59, Evan Stade wrote:
> On 2017/01/25 04:25:18, Peter Kasting wrote:
> > The comment above seems to imply this should check IsShrinking() instead.  I
> > suspect the code here is correct and it's the comment that needs to be
> clarified
> > (replace bits about the view expanding/contracting with comments about
trying
> to
> > show the label).
> 
> thinking of it in terms of expanding vs contracting seemed clearer to me, but
> I've updated the comment to cover the cases where there's no current movement
> ("steady-state").

The reason I'm anxious about referring to expanding and contracting without
going further and explaining the whole animation is that ShouldShowLabel() is a
virtual method whose implementation is potentially arbitrary (and is not
equivalent to !IsShrinking()).

So for example, if we're animating the label closed but it's still partly
visible, then IsShrinking() is true, ShouldShowLabel() is true, the view is
contracting, and we're not in the hidden-label steady state.  Both the old and
the new comments are unclear at this point.

It seems like either we could provide more detail about all the different
animation phases (a la the old comments) or we could try to change the code and
function names so that things are clearer, but I"m not sure how.

This is making me wonder if ShouldShowLabel() and !IsShrinking() are actually
the same in the case where space_shortage > 0 (even though they're not always
the same.  If they are, and we replaced the former with the latter, we could use
your original comment, and we could maybe simplify ShouldShowLabel().  But I'm
not convinced they are.

[commit-bot: I haz the power, 2017-01-26 01:45:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485391984439490,
"parent_rev": "22d3bc7d75a5f9180bca7860ee23c3f731607eda", "commit_rev":
"39f5ad11373872bbb7e398fb7943b289cc790c39"}

[commit-bot: I haz the power, 2017-01-26 01:45:55]

Description was changed from

==========
Adjust positioning of location bar icons.

See bug for new layout spec. Note that this affects not just the 
trailing icons but also the location icon ,which is now slightly
closer to the left edge of the location bar.

Test cases:
- location icon, no text (such as an http:// site)
- location icon w/text (such as an https:// site)
- alignment of contents of omnibox popup, both icons and text, with contents of
location bar
- keyword hint view, i.e. text on right of location bar when typing a searchable
keyword
- selected keyword view, i.e. after pressing tab to search
- content settings icon, e.g. blocked microphone on permission.site
- content settings icon w/text, e.g. a blocked popup on popuptest.com
- multiple location icons, like star view + zoom view
- all of the above in touch mode (--top-chrome-md=material-hybrid)

BUG=678510
==========

to

==========
Adjust positioning of location bar icons.

See bug for new layout spec. Note that this affects not just the 
trailing icons but also the location icon ,which is now slightly
closer to the left edge of the location bar.

Test cases:
- location icon, no text (such as an http:// site)
- location icon w/text (such as an https:// site)
- alignment of contents of omnibox popup, both icons and text, with contents of
location bar
- keyword hint view, i.e. text on right of location bar when typing a searchable
keyword
- selected keyword view, i.e. after pressing tab to search
- content settings icon, e.g. blocked microphone on permission.site
- content settings icon w/text, e.g. a blocked popup on popuptest.com
- multiple location icons, like star view + zoom view
- all of the above in touch mode (--top-chrome-md=material-hybrid)

BUG=678510

Review-Url: https://codereview.chromium.org/2642893002
Cr-Commit-Position: refs/heads/master@{#446199}
Committed:
https://chromium.googlesource.com/chromium/src/+/39f5ad11373872bbb7e398fb7943...
==========

[commit-bot: I haz the power, 2017-01-26 01:45:56]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/39f5ad11373872bbb7e398fb7943...