Fix padding in location bar bubbles to match specs on bug.

Fix padding in location bar bubbles to match specs on bug.

This autodetects the padding in the image inside the bubble and adjusts for it,
so the EV, keyword, and content settings bubbles all appear to have the same
padding despite having images with different transparent regions.  (This allows
eliminating a hack in the keyword bubble that tried to manually compensate.)

Then the padding constants are changed to match the desired values on the bug.

BUG=586423
TEST=Run Chrome in material design mode.  Visit Paypal.  Padding in EV bubble should match "proposed" image in comment 0 of bug.

Committed: https://crrev.com/30e625e5cc4ed92c320148decc0598f2cc534aaf
Cr-Commit-Position: refs/heads/master@{#386505}

[Peter Kasting, 2016-03-25 10:30:57]

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

[Peter Kasting, 2016-03-25 10:30:58]

(Can't submit tryjobs yet as this really depends on multiple different upstream
CLs, not just one)

[Peter Kasting, 2016-03-29 06:42:11]

Description was changed from

==========
Fix padding in location bar bubbles to match specs on bug.

This autodetects the padding in the image inside the bubble and adjusts for it,
so the EV, keyword, and content settings bubbles all appear to have the same
padding despite having images with different transparent regions.  (This allows
eliminating a hack in the keyword bubble that tried to manually compensate.)

Then the padding constants are changed to match the desired values on the bug.

BUG=586423
TEST=Run Chrome in material design mode.  Visit Paypal.  Padding in EV bubble
should match "proposed" image in comment 0 of bug.
==========

to

==========
Fix padding in location bar bubbles to match specs on bug.

This autodetects the padding in the image inside the bubble and adjusts for it,
so the EV, keyword, and content settings bubbles all appear to have the same
padding despite having images with different transparent regions.  (This allows
eliminating a hack in the keyword bubble that tried to manually compensate.)

Then the padding constants are changed to match the desired values on the bug.

BUG=586423
TEST=Run Chrome in material design mode.  Visit Paypal.  Padding in EV bubble
should match "proposed" image in comment 0 of bug.
==========

[Peter Kasting, 2016-03-29 06:42:11]

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

[Peter Kasting, 2016-03-29 07:27:50]

Evan, here's a very long writeup from my debugging session tonight to
investigate the VECTOR_ICON_NONE issue more deeply.

First, a portion of the first call stack that hit this after browser startup:

gfx::`anonymous namespace'::VectorIconSource::Draw(gfx::Canvas * canvas) Line
314
gfx::CanvasImageSource::GetImageForScale(float scale) Line 22
gfx::internal::ImageSkiaStorage::FindRepresentation(float scale, bool
fetch_new_image) Line 213
gfx::ImageSkia::GetRepresentation(float scale) Line 392
gfx::GetVisibleMargins(const gfx::ImageSkia & image, int * left, int * right)
Line 72
IconLabelBubbleView::SetImage(const gfx::ImageSkia & image_skia) Line 103
ContentSettingImageView::UpdateImage() Line 297
ContentSettingImageView::OnNativeThemeChanged(const ui::NativeTheme *
native_theme) Line 166
views::View::PropagateNativeThemeChanged(const ui::NativeTheme * theme) Line
1896
views::View::AddChildViewAt(views::View * view, int index) Line 205
views::View::AddChildView(views::View * view) Line 158
LocationBarView::Init() Line 276
ToolbarView::Init() Line 262
BrowserView::InitViews() Line 2095

Here's my summary of the relevant bits:

* LocationBarView::Init() gets the models for all 12 potentially-visible content
settings.  It then creates an image view for the first one --
CONTENT_SETTINGS_TYPE_COOKIES.  At this point, the |vector_icon_id_| field in
the model is VECTOR_ICON_NONE; this field is not set by the constructor, but
rather by ContentSettingBlockedImageModel::UpdateFromWebContents(), which will
be called before the image for this content setting becomes visible.  This is
because in the pre-MD code, the image ID depends on e.g. whether the page is
currently blocking or allowing this content type.  In MD, the vector icon ID is
fixed per image type, but the badge ID is still dynamic; both are set at once.
* Newly adding any child view to a hierarchy always propagates a "theme changed"
notification to it.  In the case of ContentSettingImageView, it has a clause for
MD that calls UpdateImage().
* ContentSettingImageView::UpdateImage() asks the model for the appropriate
image.  It gets an ImageSkia for VECTOR_ICON_NONE.
* IconLabelBubbleView::SetImage() tries to get the visible margins of this
image.  Because my previous CL made this unconditionally call
GetRepresentation(), this ends up calling CanvasImageSource::GetImageForScale().
* CanvasImageSource::GetImageForScale() calls Draw(), which -- without the
change in this CL -- would call PaintVecotrIcon() for VECTOR_ICON_NONE and
checkfail.

One thing this tells us is that your proposed change on the other CL -- to
CreateVectorIconWithBadge() -- wouldn't fix the problem here, because that
function is not in this call stack.

It's possible that we could avoid the problem in this particular stack by
changing how content setting models get their vector icon IDs.  We could try to
put in an MD-specific path that inits the vector_icon_id_ field early, and makes
only the badge ID get updated dynamically by UpdateFromWebContents().  I haven't
checked to see if this is feasible and if there are more cases beyond that,
because I thought the question of whether VectorIconSource::Draw() should be
legal to call with VECTOR_ICON_NONE was more fundamental.  Basically, anyone who
calls ImageSkia::GetRepresentation() on such an image will trigger this.  It's
not obvious callers at such a level would know when an image is
VECTOR_ICON_NONE, and as noted on my other CL, I don't think this can be
determined by using HasRepresentation() to check, because I think that will
always be false for newly-created, not-yet-drawn vector images.

So I see the possible paths as follows (not mutually exclusive):

(1) Fix HasRepresentation() to return whether an image _could_ have a particular
representation.  For bitmap images, this is effectively what it does today, I
think.  For vector images, this would basically return true iff the underlying
vector icon ID is not VECTOR_ICON_NONE.  Then return to the other CL and restore
the HasRepresentation() pre-check in front of the GetRepresentation() call.
(2) Like (1), but add some parallel function instead to do this, i.e.
distinguish at the API level between "currently has a representation" and "could
have a representation".
(3) Make the change suggested above to how content setting image models assign
their vector icon IDs, assuming this removes the cases today where we ask for a
representation for VECTOR_ICON_NONE.
(4) Make the change in this CL to safe VectorIconSource::Draw() against
checkfailure if reached for VECTOR_ICON_NONE.

I don't think (2) makes sense; I can't think of why the distinction it puts in
the API is relevant to callers.  However, (1), (3), and (4) could all make
sense.  I think (1) should be done as IMO the behavior today is a buggy
oversight for vector images.  I think (3) is probably a nice-to-have cleanup
change, that I would prefer to do separately.  IMO (4) is probably better than
the behavior today (I worry about the kind of callstack we have here being
reachable by someone else in the future, and it's not obvious to me callers who
do this must be wrong), but maybe you'll know better than me why that should or
should not be done.

So I would probably suggest doing (1) separately first, doing (4) either
separately or here unless you have a better reasoning for why it's wrong, then
doing (3) separately as cleanup, maybe later.

[Evan Stade, 2016-03-29 16:34:12]

On 2016/03/29 07:27:50, Peter Kasting wrote:
> Evan, here's a very long writeup from my debugging session tonight to
> investigate the VECTOR_ICON_NONE issue more deeply.
> 
> First, a portion of the first call stack that hit this after browser startup:
> 
> gfx::`anonymous namespace'::VectorIconSource::Draw(gfx::Canvas * canvas) Line
> 314
> gfx::CanvasImageSource::GetImageForScale(float scale) Line 22
> gfx::internal::ImageSkiaStorage::FindRepresentation(float scale, bool
> fetch_new_image) Line 213
> gfx::ImageSkia::GetRepresentation(float scale) Line 392
> gfx::GetVisibleMargins(const gfx::ImageSkia & image, int * left, int * right)
> Line 72
> IconLabelBubbleView::SetImage(const gfx::ImageSkia & image_skia) Line 103
> ContentSettingImageView::UpdateImage() Line 297
> ContentSettingImageView::OnNativeThemeChanged(const ui::NativeTheme *
> native_theme) Line 166
> views::View::PropagateNativeThemeChanged(const ui::NativeTheme * theme) Line
> 1896
> views::View::AddChildViewAt(views::View * view, int index) Line 205
> views::View::AddChildView(views::View * view) Line 158
> LocationBarView::Init() Line 276
> ToolbarView::Init() Line 262
> BrowserView::InitViews() Line 2095
> 
> Here's my summary of the relevant bits:
> 
> * LocationBarView::Init() gets the models for all 12 potentially-visible
content
> settings.  It then creates an image view for the first one --
> CONTENT_SETTINGS_TYPE_COOKIES.  At this point, the |vector_icon_id_| field in
> the model is VECTOR_ICON_NONE; this field is not set by the constructor, but
> rather by ContentSettingBlockedImageModel::UpdateFromWebContents(), which will
> be called before the image for this content setting becomes visible.  This is
> because in the pre-MD code, the image ID depends on e.g. whether the page is
> currently blocking or allowing this content type.  In MD, the vector icon ID
is
> fixed per image type, but the badge ID is still dynamic; both are set at once.
> * Newly adding any child view to a hierarchy always propagates a "theme
changed"
> notification to it.  In the case of ContentSettingImageView, it has a clause
for
> MD that calls UpdateImage().
> * ContentSettingImageView::UpdateImage() asks the model for the appropriate
> image.  It gets an ImageSkia for VECTOR_ICON_NONE.
> * IconLabelBubbleView::SetImage() tries to get the visible margins of this
> image.  Because my previous CL made this unconditionally call
> GetRepresentation(), this ends up calling
CanvasImageSource::GetImageForScale().
> * CanvasImageSource::GetImageForScale() calls Draw(), which -- without the
> change in this CL -- would call PaintVecotrIcon() for VECTOR_ICON_NONE and
> checkfail.
> 
> One thing this tells us is that your proposed change on the other CL -- to
> CreateVectorIconWithBadge() -- wouldn't fix the problem here, because that
> function is not in this call stack.

I don't think this is correct. ContentSettingImageView::UpdateImage() calls into
it here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Did you try the suggested change? If it works, do you think it's a good
solution? I like it becuase it avoids creating and caching images that shouldn't
exist.

[Peter Kasting, 2016-03-30 03:39:57]

On 2016/03/29 16:34:12, Evan Stade wrote:
> On 2016/03/29 07:27:50, Peter Kasting wrote:
> > One thing this tells us is that your proposed change on the other CL -- to
> > CreateVectorIconWithBadge() -- wouldn't fix the problem here, because that
> > function is not in this call stack.
> 
> I don't think this is correct. ContentSettingImageView::UpdateImage() calls
into
> it here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> Did you try the suggested change? If it works, do you think it's a good
> solution? I like it becuase it avoids creating and caching images that
shouldn't
> exist.

You are correct.  This does work because it prevents the callstack I
investigated from happening to begin with.  I think this is a good change
regardless of other considerations, and I've included it here.

I also think the HasRepresentation() stuff is worth fixing anyway, so I've done
that as a separate CL you're a reviewer for.  Note that either one of these
changes alone makes the rest of the code in this CL work.

For now, I've tentatively omitted my previous change, to safe
VectorIconSource::Draw() against being called for VECTOR_ICON_NONE.  It's up to
you whether you think that would provide safety (yay) or hide errors (boo).

[varkha, 2016-03-30 04:04:07]

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

https://codereview.chromium.org/1829353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:245: (leading ?
builtin_leading_padding_ : 0);
Wouldn't that increase the visible trailing padding compared to what it was
before this CL because the constants have grown but you are not subtracting a
built-in padding in the trailing case?

[Peter Kasting, 2016-03-30 04:10:55]

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

https://codereview.chromium.org/1829353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:245: (leading ?
builtin_leading_padding_ : 0);
On 2016/03/30 04:04:07, varkha wrote:
> Wouldn't that increase the visible trailing padding compared to what it was
> before this CL because the constants have grown but you are not subtracting a
> built-in padding in the trailing case?

In some cases, yes; this looks more consistent (the content settings padding now
matches the other bubbles) and IMO better.  Sebastien agreed in principle, I
promised him pictures of the changes.

[varkha, 2016-03-30 04:35:10]

Not sure if this CL depends on something recent in the ToT that I don't have but
I am seeing icon and label overlapping in the popup bubble animation. Do you see
it as well?

https://codereview.chromium.org/1829353002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1829353002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:103:
gfx::GetVisibleMargins(image_skia, &builtin_leading_padding_,
gfx::VisibleMargins?

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

https://codereview.chromium.org/1829353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:245: (leading ?
builtin_leading_padding_ : 0);
On 2016/03/30 04:10:55, Peter Kasting wrote:
> On 2016/03/30 04:04:07, varkha wrote:
> > Wouldn't that increase the visible trailing padding compared to what it was
> > before this CL because the constants have grown but you are not subtracting
a
> > built-in padding in the trailing case?
> 
> In some cases, yes; this looks more consistent (the content settings padding
now
> matches the other bubbles) and IMO better.  Sebastien agreed in principle, I
> promised him pictures of the changes.

So in the popup animated bubble the visible left padding before the icon will be
smaller than the trailing padding after the label?

[Peter Kasting, 2016-03-30 04:44:57]

On 2016/03/30 04:35:10, varkha wrote:
> Not sure if this CL depends on something recent in the ToT that I don't have
but
> I am seeing icon and label overlapping in the popup bubble animation. Do you
see
> it as well?

How are you even compiling this?  It should not compile unless you have landed
my changes to the visible margin calculation function.

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

https://codereview.chromium.org/1829353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:245: (leading ?
builtin_leading_padding_ : 0);
On 2016/03/30 04:35:10, varkha wrote:
> On 2016/03/30 04:10:55, Peter Kasting wrote:
> > On 2016/03/30 04:04:07, varkha wrote:
> > > Wouldn't that increase the visible trailing padding compared to what it
was
> > > before this CL because the constants have grown but you are not
subtracting
> a
> > > built-in padding in the trailing case?
> > 
> > In some cases, yes; this looks more consistent (the content settings padding
> now
> > matches the other bubbles) and IMO better.  Sebastien agreed in principle, I
> > promised him pictures of the changes.
> 
> So in the popup animated bubble the visible left padding before the icon will
be
> smaller than the trailing padding after the label?   

No.  The visible padding will be equal.  The point of subtracting out this
value, in fact, is to make these values appear equal.

In the blocked popup animation, they already would have, since that uses a
full-width icon.  That's not true of the EV/keyword search bubbles.

[varkha, 2016-03-30 05:04:57]

Aha!. I didn't realize this depended on
https://codereview.chromium.org/1836483002/ so I "just" changed
GetVisibleMargins into VisibleMargins to compile. Do you want to wait till
https://codereview.chromium.org/1836483002 lands? That will probably solve the
problems that I am seeing.

[Peter Kasting, 2016-03-30 05:16:58]

On 2016/03/30 05:04:57, varkha wrote:
> Aha!. I didn't realize this depended on
> https://codereview.chromium.org/1836483002/ so I "just" changed
> GetVisibleMargins into VisibleMargins to compile. Do you want to wait till
> https://codereview.chromium.org/1836483002 lands? That will probably solve the
> problems that I am seeing.

Yes, I didn't intend to run tryjobs, take screenshots, etc. until that landed.

Unless there's some way to mark a CL as dependent on two distinct parents.

[varkha, 2016-03-30 05:21:58]

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

https://codereview.chromium.org/1829353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:245: (leading ?
builtin_leading_padding_ : 0);
On 2016/03/30 04:44:57, Peter Kasting wrote:
> On 2016/03/30 04:35:10, varkha wrote:
> > On 2016/03/30 04:10:55, Peter Kasting wrote:
> > > On 2016/03/30 04:04:07, varkha wrote:
> > > > Wouldn't that increase the visible trailing padding compared to what it
> was
> > > > before this CL because the constants have grown but you are not
> subtracting
> > a
> > > > built-in padding in the trailing case?
> > > 
> > > In some cases, yes; this looks more consistent (the content settings
padding
> > now
> > > matches the other bubbles) and IMO better.  Sebastien agreed in principle,
I
> > > promised him pictures of the changes.
> > 
> > So in the popup animated bubble the visible left padding before the icon
will
> be
> > smaller than the trailing padding after the label?   
> 
> No.  The visible padding will be equal.  The point of subtracting out this
> value, in fact, is to make these values appear equal.
> 
> In the blocked popup animation, they already would have, since that uses a
> full-width icon.  That's not true of the EV/keyword search bubbles.

OK, but in general case if builtin_*_padding_ values are not zero would you not
end in a state when visible margins around the icon are same (leading padding
and spacing) but the padding right of the label is larger?

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

https://codereview.chromium.org/1829353002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:259: // Leading
and trailing padding are equal.
nit: The comment seems to be no longer accurate.

[Peter Kasting, 2016-03-30 05:33:37]

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

https://codereview.chromium.org/1829353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:245: (leading ?
builtin_leading_padding_ : 0);
On 2016/03/30 05:21:58, varkha wrote:
> On 2016/03/30 04:44:57, Peter Kasting wrote:
> > On 2016/03/30 04:35:10, varkha wrote:
> > > On 2016/03/30 04:10:55, Peter Kasting wrote:
> > > > On 2016/03/30 04:04:07, varkha wrote:
> > > > > Wouldn't that increase the visible trailing padding compared to what
it
> > was
> > > > > before this CL because the constants have grown but you are not
> > subtracting
> > > a
> > > > > built-in padding in the trailing case?
> > > > 
> > > > In some cases, yes; this looks more consistent (the content settings
> padding
> > > now
> > > > matches the other bubbles) and IMO better.  Sebastien agreed in
principle,
> I
> > > > promised him pictures of the changes.
> > > 
> > > So in the popup animated bubble the visible left padding before the icon
> will
> > be
> > > smaller than the trailing padding after the label?   
> > 
> > No.  The visible padding will be equal.  The point of subtracting out this
> > value, in fact, is to make these values appear equal.
> > 
> > In the blocked popup animation, they already would have, since that uses a
> > full-width icon.  That's not true of the EV/keyword search bubbles.
> 
> OK, but in general case if builtin_*_padding_ values are not zero would you
not
> end in a state when visible margins around the icon are same (leading padding
> and spacing) but the padding right of the label is larger?

No.

ICON_LABEL_VIEW_TRAILING_PADDING is the number of pixels to the left of the
apparent edge of the icon, and to the right of the label.  Since there's only
one constant in MD, these two values are always the same.

If the icon actually has n px of space on its left edge, then if we were to use
the constant's value for both layout values, the apparent leading padding would
be n px larger than the label's trailing padding.  By subtracting n, we ensure
the apparent padding is always exactly the same as the post-label padding.

Note that the space between icon and label is completely distinct from the above
-- it's based on a different constant (ICON_LABEL_VIEW_INTERNAL_SPACING) and
subtracts a different built-in padding value (builtin_trailing_padding_).  So if
you're mentally assuming this value is the same as the padding to the left of
the icon, you're not thinking about the situation correctly.

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

https://codereview.chromium.org/1829353002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:259: // Leading
and trailing padding are equal.
On 2016/03/30 05:21:58, varkha wrote:
> nit: The comment seems to be no longer accurate.

The intent of the comment is to say that the apparent leading and trailing
padding are equal.  I could probably expand the wording to make that more
obvious.

[Peter Kasting, 2016-03-30 05:55:02]

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

https://codereview.chromium.org/1829353002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:259: // Leading
and trailing padding are equal.
On 2016/03/30 05:33:37, Peter Kasting wrote:
> On 2016/03/30 05:21:58, varkha wrote:
> > nit: The comment seems to be no longer accurate.
> 
> The intent of the comment is to say that the apparent leading and trailing
> padding are equal.  I could probably expand the wording to make that more
> obvious.

Done.

[varkha, 2016-03-30 06:17:29]

The change LG. Hope it is OK if I wait for the GetVisibleMargins change to land
on ToT so I could see the new looks (?)

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

https://codereview.chromium.org/1829353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:245: (leading ?
builtin_leading_padding_ : 0);
On 2016/03/30 05:33:37, Peter Kasting wrote:
> On 2016/03/30 05:21:58, varkha wrote:
> > On 2016/03/30 04:44:57, Peter Kasting wrote:
> > > On 2016/03/30 04:35:10, varkha wrote:
> > > > On 2016/03/30 04:10:55, Peter Kasting wrote:
> > > > > On 2016/03/30 04:04:07, varkha wrote:
> > > > > > Wouldn't that increase the visible trailing padding compared to what
> it
> > > was
> > > > > > before this CL because the constants have grown but you are not
> > > subtracting
> > > > a
> > > > > > built-in padding in the trailing case?
> > > > > 
> > > > > In some cases, yes; this looks more consistent (the content settings
> > padding
> > > > now
> > > > > matches the other bubbles) and IMO better.  Sebastien agreed in
> principle,
> > I
> > > > > promised him pictures of the changes.
> > > > 
> > > > So in the popup animated bubble the visible left padding before the icon
> > will
> > > be
> > > > smaller than the trailing padding after the label?   
> > > 
> > > No.  The visible padding will be equal.  The point of subtracting out this
> > > value, in fact, is to make these values appear equal.
> > > 
> > > In the blocked popup animation, they already would have, since that uses a
> > > full-width icon.  That's not true of the EV/keyword search bubbles.
> > 
> > OK, but in general case if builtin_*_padding_ values are not zero would you
> not
> > end in a state when visible margins around the icon are same (leading
padding
> > and spacing) but the padding right of the label is larger?
> 
> No.
> 
> ICON_LABEL_VIEW_TRAILING_PADDING is the number of pixels to the left of the
> apparent edge of the icon, and to the right of the label.  Since there's only
> one constant in MD, these two values are always the same.
> 
> If the icon actually has n px of space on its left edge, then if we were to
use
> the constant's value for both layout values, the apparent leading padding
would
> be n px larger than the label's trailing padding.  By subtracting n, we ensure
> the apparent padding is always exactly the same as the post-label padding.
> 
> Note that the space between icon and label is completely distinct from the
above
> -- it's based on a different constant (ICON_LABEL_VIEW_INTERNAL_SPACING) and
> subtracts a different built-in padding value (builtin_trailing_padding_).  So
if
> you're mentally assuming this value is the same as the padding to the left of
> the icon, you're not thinking about the situation correctly.

I see now, thanks for the explanation. I guess I was confused because this CL
will actually change the trailing padding to be a bit larger and the leading
visible padding to be exactly same as the trailing padding.

[Peter Kasting, 2016-03-30 06:50:38]

On 2016/03/30 06:17:29, varkha wrote:
> The change LG. Hope it is OK if I wait for the GetVisibleMargins change to
land
> on ToT so I could see the new looks (?)

Yes.  I'll need to wait for that to run tryjobs anyway.

[Evan Stade, 2016-03-30 21:44:13]

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

[Evan Stade, 2016-03-30 21:44:13]

https://codereview.chromium.org/1829353002/diff/120001/ui/gfx/paint_vector_ic...
File ui/gfx/paint_vector_icon.cc (right):

https://codereview.chromium.org/1829353002/diff/120001/ui/gfx/paint_vector_ic...
ui/gfx/paint_vector_icon.cc:408: return (id == VectorIconId::VECTOR_ICON_NONE)
After thinking about this more, the thing that worries me is that now the
returned image doesn't have the same dimensions (dip_size x dip_size). I'd be
concerned that things like ImageView might need a relayout when the actual image
is set (SetImage calls PreferredSizeChanged() but most containers will ignore
that), or that VECTOR_ICON_NONE images might validly be used as a placeholder to
reserve space for a real image. I wonder if it's easy to create an empty
gfx::ImageSkia of a specified size? Does oshima@ know?

[Peter Kasting, 2016-03-30 21:53:23]

https://codereview.chromium.org/1829353002/diff/120001/ui/gfx/paint_vector_ic...
File ui/gfx/paint_vector_icon.cc (right):

https://codereview.chromium.org/1829353002/diff/120001/ui/gfx/paint_vector_ic...
ui/gfx/paint_vector_icon.cc:408: return (id == VectorIconId::VECTOR_ICON_NONE)
On 2016/03/30 21:44:13, Evan Stade wrote:
> After thinking about this more, the thing that worries me is that now the
> returned image doesn't have the same dimensions (dip_size x dip_size). I'd be
> concerned that things like ImageView might need a relayout when the actual
image
> is set (SetImage calls PreferredSizeChanged() but most containers will ignore
> that), or that VECTOR_ICON_NONE images might validly be used as a placeholder
to
> reserve space for a real image. I wonder if it's easy to create an empty
> gfx::ImageSkia of a specified size? Does oshima@ know?

Why do you say most containers will ignore PreferredSizeChanged()? 
View::PreferredSizeChanged() calls InvalidateLayout().  Unless you override
PreferredSizeChanged() to specifically not do that (which no one does; we have 2
overrides in the codebase and both call the base class method), that will force
a relayout.

As for using VECTOR_ICON_NONE with a specific dip_size to reserve space for
something, this seems unlikely, since in general "reserving space" is not a
pattern we use in views.  When something changes, we normally just relayout. 
And conceptually, I can't think of a case where I'd have an empty spot but want
to hold a gap there for some possible later object to fill.  We want empty
sections of the UI to collapse during layout, so that e.g. the omnibox takes all
the available space in the address bar, or the tab content takes all the
available space in the tab, or whatever similar concept you may be dealing with.

I think we're safe here.

[Evan Stade, 2016-03-30 22:01:57]

On 2016/03/30 21:53:23, Peter Kasting wrote:
>
https://codereview.chromium.org/1829353002/diff/120001/ui/gfx/paint_vector_ic...
> File ui/gfx/paint_vector_icon.cc (right):
> 
>
https://codereview.chromium.org/1829353002/diff/120001/ui/gfx/paint_vector_ic...
> ui/gfx/paint_vector_icon.cc:408: return (id == VectorIconId::VECTOR_ICON_NONE)
> On 2016/03/30 21:44:13, Evan Stade wrote:
> > After thinking about this more, the thing that worries me is that now the
> > returned image doesn't have the same dimensions (dip_size x dip_size). I'd
be
> > concerned that things like ImageView might need a relayout when the actual
> image
> > is set (SetImage calls PreferredSizeChanged() but most containers will
ignore
> > that), or that VECTOR_ICON_NONE images might validly be used as a
placeholder
> to
> > reserve space for a real image. I wonder if it's easy to create an empty
> > gfx::ImageSkia of a specified size? Does oshima@ know?
> 
> Why do you say most containers will ignore PreferredSizeChanged()? 
> View::PreferredSizeChanged() calls InvalidateLayout().  Unless you override
> PreferredSizeChanged() to specifically not do that (which no one does; we have
2
> overrides in the codebase and both call the base class method), that will
force
> a relayout.

I don't see how that gets around to calling Layout()? All it does is send
InvalidateLayout() up the chain. If what you're saying is correct, why does this
function:

  // Called when the preferred size of a child view changed.  This gives the
  // parent an opportunity to do a fresh layout if that makes sense.
  virtual void ChildPreferredSizeChanged(View* child) {}

have 52 overrides in Chrome, usually of the form:

void SearchResultView::ChildPreferredSizeChanged(views::View* child) {
  Layout();
}

> 
> As for using VECTOR_ICON_NONE with a specific dip_size to reserve space for
> something, this seems unlikely, since in general "reserving space" is not a
> pattern we use in views.  When something changes, we normally just relayout. 
> And conceptually, I can't think of a case where I'd have an empty spot but
want
> to hold a gap there for some possible later object to fill.  We want empty
> sections of the UI to collapse during layout, so that e.g. the omnibox takes
all
> the available space in the address bar, or the tab content takes all the
> available space in the tab, or whatever similar concept you may be dealing
with.
> 
> I think we're safe here.

I'm willing to go with it and hope that we notice any issues it might cause.

[Peter Kasting, 2016-03-30 22:12:36]

On 2016/03/30 22:01:57, Evan Stade wrote:
> On 2016/03/30 21:53:23, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/1829353002/diff/120001/ui/gfx/paint_vector_ic...
> > File ui/gfx/paint_vector_icon.cc (right):
> > 
> >
>
https://codereview.chromium.org/1829353002/diff/120001/ui/gfx/paint_vector_ic...
> > ui/gfx/paint_vector_icon.cc:408: return (id ==
VectorIconId::VECTOR_ICON_NONE)
> > On 2016/03/30 21:44:13, Evan Stade wrote:
> > > After thinking about this more, the thing that worries me is that now the
> > > returned image doesn't have the same dimensions (dip_size x dip_size). I'd
> be
> > > concerned that things like ImageView might need a relayout when the actual
> > image
> > > is set (SetImage calls PreferredSizeChanged() but most containers will
> ignore
> > > that), or that VECTOR_ICON_NONE images might validly be used as a
> placeholder
> > to
> > > reserve space for a real image. I wonder if it's easy to create an empty
> > > gfx::ImageSkia of a specified size? Does oshima@ know?
> > 
> > Why do you say most containers will ignore PreferredSizeChanged()? 
> > View::PreferredSizeChanged() calls InvalidateLayout().  Unless you override
> > PreferredSizeChanged() to specifically not do that (which no one does; we
have
> 2
> > overrides in the codebase and both call the base class method), that will
> force
> > a relayout.
> 
> I don't see how that gets around to calling Layout()? All it does is send
> InvalidateLayout() up the chain. If what you're saying is correct, why does
this
> function:
> 
>   // Called when the preferred size of a child view changed.  This gives the
>   // parent an opportunity to do a fresh layout if that makes sense.
>   virtual void ChildPreferredSizeChanged(View* child) {}
> 
> have 52 overrides in Chrome, usually of the form:
> 
> void SearchResultView::ChildPreferredSizeChanged(views::View* child) {
>   Layout();
> }

In fairness, we have more like 30 of these since the tool seems to be counting a
lot of the declarations as "overrides" of their own.  Quite a few do some form
of layout (though the majority doing that are doing more, e.g.
SizeToPreferredSize() or SizeToContents() or something), but a lot also just
call PreferredSizeChanged(), which makes utterly no sense.  This leaves me
suspicious that a lot of people overriding this don't know what they're doing. 
But probably at least a few do.

And I think you're correct that if you invalidate layout, but then do not
receive any size/bounds changes (anywhere up the hierarchy) or explicit layout
calls, you won't relayout.  I didn't realize this.

> > I think we're safe here.
> 
> I'm willing to go with it and hope that we notice any issues it might cause.

Me too -- I think using VECTOR_ICON_NONE like this seems kind of like an
anti-pattern -- but it's good to have thought about, at least.

[Peter Kasting, 2016-03-30 22:14:16]

(P.S. if you are indeed OK with this, and you don't think I should do the other
change I omitted, where I was safing Draw() against a call with
VECTOR_ICON_NONE, go ahead and sign off; I'll wait for varkha's OK as the
primary reviewer.)

[Evan Stade, 2016-03-30 22:27:54]

Yea, regarding the other change, I'm in the "hide errors (boo)" camp. LGTM

[Peter Kasting, 2016-03-31 00:38:43]

varkha: All precursor patches have landed; you can resync, apply this, and build
locally if you wish.

[varkha, 2016-03-31 16:55:22]

Not sure if the 1 pixel difference seen in a screenshot that I've attached to
the bug is real or just a result of a shadow on the last letter or something. I
see 5 pixels on the left, 6 pixels between and 6 pixels on the right (all
increased by 3 from the previous values).

[varkha, 2016-03-31 17:21:17]

Looking at different bubbles the apparent trailing padding as well as spacing
seems to depend on the combination of text and background color so it is likely
the internal font rendering discrepancy (paypal has 5px everywhere while
microsoft has much more - 8px - on the trailing edge probably as a result of
eliding the long string). I have also checked the animation (slowed down) and
the trailing padding when the label is cut is always 5px. The change LGTM since
the match seems to be correct.

[Peter Kasting, 2016-03-31 21:03:00]

On 2016/03/31 17:21:17, varkha wrote:
> Looking at different bubbles the apparent trailing padding as well as spacing
> seems to depend on the combination of text and background color so it is
likely
> the internal font rendering discrepancy (paypal has 5px everywhere while
> microsoft has much more - 8px - on the trailing edge probably as a result of
> eliding the long string).

What Microsoft case is this?  I'd like to look at this 8 px case you describe.

[varkha, 2016-03-31 21:12:41]

On 2016/03/31 21:03:00, Peter Kasting wrote:
> On 2016/03/31 17:21:17, varkha wrote:
> > Looking at different bubbles the apparent trailing padding as well as
spacing
> > seems to depend on the combination of text and background color so it is
> likely
> > the internal font rendering discrepancy (paypal has 5px everywhere while
> > microsoft has much more - 8px - on the trailing edge probably as a result of
> > eliding the long string).
> 
> What Microsoft case is this?  I'd like to look at this 8 px case you describe.

It looks like when you get to the size when the bubble is elided the trailing
padding will depend on the remainder from the last hidden character so it can be
anywhere from 5 to 5+N where N is the last hidden character width.
Open microsoft.com and check with different widths of the browser window
(resizing slowly).

[Peter Kasting, 2016-03-31 21:15:05]

On 2016/03/31 21:12:41, varkha wrote:
> On 2016/03/31 21:03:00, Peter Kasting wrote:
> > On 2016/03/31 17:21:17, varkha wrote:
> > > Looking at different bubbles the apparent trailing padding as well as
> spacing
> > > seems to depend on the combination of text and background color so it is
> > likely
> > > the internal font rendering discrepancy (paypal has 5px everywhere while
> > > microsoft has much more - 8px - on the trailing edge probably as a result
of
> > > eliding the long string).
> > 
> > What Microsoft case is this?  I'd like to look at this 8 px case you
describe.
> 
> It looks like when you get to the size when the bubble is elided the trailing
> padding will depend on the remainder from the last hidden character so it can
be
> anywhere from 5 to 5+N where N is the last hidden character width.
> Open http://microsoft.com and check with different widths of the browser
window
> (resizing slowly).

I see, so this is only an issue when the bubble has less than its preferred
width?  In that case, while we could clamp the bubble size based on the text
size, I actually think the current behavior looks smoother and better as the
window is resized, so I'll consider it not a bug.

[varkha, 2016-03-31 21:43:06]

On 2016/03/31 21:15:05, Peter Kasting wrote:
> On 2016/03/31 21:12:41, varkha wrote:
> > On 2016/03/31 21:03:00, Peter Kasting wrote:
> > > On 2016/03/31 17:21:17, varkha wrote:
> > > > Looking at different bubbles the apparent trailing padding as well as
> > spacing
> > > > seems to depend on the combination of text and background color so it is
> > > likely
> > > > the internal font rendering discrepancy (paypal has 5px everywhere while
> > > > microsoft has much more - 8px - on the trailing edge probably as a
result
> of
> > > > eliding the long string).
> > > 
> > > What Microsoft case is this?  I'd like to look at this 8 px case you
> describe.
> > 
> > It looks like when you get to the size when the bubble is elided the
trailing
> > padding will depend on the remainder from the last hidden character so it
can
> be
> > anywhere from 5 to 5+N where N is the last hidden character width.
> > Open http://microsoft.com and check with different widths of the browser
> window
> > (resizing slowly).
> 
> I see, so this is only an issue when the bubble has less than its preferred
> width?  In that case, while we could clamp the bubble size based on the text
> size, I actually think the current behavior looks smoother and better as the
> window is resized, so I'll consider it not a bug.

Yes, I agree. It is better to keep the bubble itself resizing smoothly even if
the
trailing padding jumps when letters get added / removed.

[Peter Kasting, 2016-04-11 19:22:36]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-04-11 19:23:09]

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

[commit-bot: I haz the power, 2016-04-11 20:18:50]

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

[commit-bot: I haz the power, 2016-04-11 20:18:52]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Peter Kasting, 2016-04-11 20:56:24]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-04-11 20:57:10]

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

[commit-bot: I haz the power, 2016-04-11 22:57:44]

Description was changed from

==========
Fix padding in location bar bubbles to match specs on bug.

This autodetects the padding in the image inside the bubble and adjusts for it,
so the EV, keyword, and content settings bubbles all appear to have the same
padding despite having images with different transparent regions.  (This allows
eliminating a hack in the keyword bubble that tried to manually compensate.)

Then the padding constants are changed to match the desired values on the bug.

BUG=586423
TEST=Run Chrome in material design mode.  Visit Paypal.  Padding in EV bubble
should match "proposed" image in comment 0 of bug.
==========

to

==========
Fix padding in location bar bubbles to match specs on bug.

This autodetects the padding in the image inside the bubble and adjusts for it,
so the EV, keyword, and content settings bubbles all appear to have the same
padding despite having images with different transparent regions.  (This allows
eliminating a hack in the keyword bubble that tried to manually compensate.)

Then the padding constants are changed to match the desired values on the bug.

BUG=586423
TEST=Run Chrome in material design mode.  Visit Paypal.  Padding in EV bubble
should match "proposed" image in comment 0 of bug.
==========

[commit-bot: I haz the power, 2016-04-11 22:57:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-11 22:59:11]

Description was changed from

==========
Fix padding in location bar bubbles to match specs on bug.

This autodetects the padding in the image inside the bubble and adjusts for it,
so the EV, keyword, and content settings bubbles all appear to have the same
padding despite having images with different transparent regions.  (This allows
eliminating a hack in the keyword bubble that tried to manually compensate.)

Then the padding constants are changed to match the desired values on the bug.

BUG=586423
TEST=Run Chrome in material design mode.  Visit Paypal.  Padding in EV bubble
should match "proposed" image in comment 0 of bug.
==========

to

==========
Fix padding in location bar bubbles to match specs on bug.

This autodetects the padding in the image inside the bubble and adjusts for it,
so the EV, keyword, and content settings bubbles all appear to have the same
padding despite having images with different transparent regions.  (This allows
eliminating a hack in the keyword bubble that tried to manually compensate.)

Then the padding constants are changed to match the desired values on the bug.

BUG=586423
TEST=Run Chrome in material design mode.  Visit Paypal.  Padding in EV bubble
should match "proposed" image in comment 0 of bug.

Committed: https://crrev.com/30e625e5cc4ed92c320148decc0598f2cc534aaf
Cr-Commit-Position: refs/heads/master@{#386505}
==========

[commit-bot: I haz the power, 2016-04-11 22:59:13]

Patchset 7 (id:??) landed as
https://crrev.com/30e625e5cc4ed92c320148decc0598f2cc534aaf
Cr-Commit-Position: refs/heads/master@{#386505}