[MD] Don't allow overflowing location icons to appear on top of toolbar

[MD] Don't allow overflowing location icons to appear on top of toolbar

BUG=588331
Committed: https://crrev.com/901e730a6fca2e528807b873aaa650be82edc635
Cr-Commit-Position: refs/heads/master@{#377445}

[Evan Stade, 2016-02-24 01:40:14]

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

[Peter Kasting, 2016-02-24 01:46:08]

RS LGTM.  I don't really understand anything about layers so I don't actually
know what this is doing, but it looks plausible.

I vaguely wonder if we have some of the default settings for views wrong
regarding any of these, if we're winding up with views drawing outside their
bounds unless we explicitly enable clipping.  Seems like that should be the
default.  But again, I don't know what I'm talking about.

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

https://codereview.chromium.org/1724333002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:187: } else {
Nit: Can combine this else with subsequent if and flatten to one three-arm
conditional

[Evan Stade, 2016-02-24 01:55:20]

On 2016/02/24 01:46:08, Peter Kasting wrote:
> RS LGTM.  I don't really understand anything about layers so I don't actually
> know what this is doing, but it looks plausible.
> 
> I vaguely wonder if we have some of the default settings for views wrong
> regarding any of these, if we're winding up with views drawing outside their
> bounds unless we explicitly enable clipping.  Seems like that should be the
> default.  But again, I don't know what I'm talking about.

I agree. My understanding is that non-layer views typically do clip by default,
but in the case of layers, they don't, so when we go around adding layers we
find a lot of places that are laying out children outside their own bounds and
it looks broken. I'm not sure if there's a good way to fix this more generally.

> 
>
https://codereview.chromium.org/1724333002/diff/1/chrome/browser/ui/views/loc...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/1724333002/diff/1/chrome/browser/ui/views/loc...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:187: } else {
> Nit: Can combine this else with subsequent if and flatten to one three-arm
> conditional

[Peter Kasting, 2016-02-24 02:05:50]

On 2016/02/24 01:55:20, Evan Stade wrote:
> On 2016/02/24 01:46:08, Peter Kasting wrote:
> > RS LGTM.  I don't really understand anything about layers so I don't
actually
> > know what this is doing, but it looks plausible.
> > 
> > I vaguely wonder if we have some of the default settings for views wrong
> > regarding any of these, if we're winding up with views drawing outside their
> > bounds unless we explicitly enable clipping.  Seems like that should be the
> > default.  But again, I don't know what I'm talking about.
> 
> I agree. My understanding is that non-layer views typically do clip by
default,
> but in the case of layers, they don't, so when we go around adding layers we
> find a lot of places that are laying out children outside their own bounds and
> it looks broken. I'm not sure if there's a good way to fix this more
generally.

Is there somebody who really understands layers?  Maybe we should file a bug
against them about "consider having layers default to clipping to their bounds"
or something?

[Evan Stade, 2016-02-24 02:08:45]

Description was changed from

==========
[MD] Don't allow overflowing location icons to appear on top of toolbar

BUG=588331
==========

to

==========
[MD] Don't allow overflowing location icons to appear on top of toolbar

BUG=588331
==========

[Evan Stade, 2016-02-24 02:08:45]

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

[Evan Stade, 2016-02-24 02:09:22]

On 2016/02/24 02:05:50, Peter Kasting wrote:
> On 2016/02/24 01:55:20, Evan Stade wrote:
> > On 2016/02/24 01:46:08, Peter Kasting wrote:
> > > RS LGTM.  I don't really understand anything about layers so I don't
> actually
> > > know what this is doing, but it looks plausible.
> > > 
> > > I vaguely wonder if we have some of the default settings for views wrong
> > > regarding any of these, if we're winding up with views drawing outside
their
> > > bounds unless we explicitly enable clipping.  Seems like that should be
the
> > > default.  But again, I don't know what I'm talking about.
> > 
> > I agree. My understanding is that non-layer views typically do clip by
> default,
> > but in the case of layers, they don't, so when we go around adding layers we
> > find a lot of places that are laying out children outside their own bounds
and
> > it looks broken. I'm not sure if there's a good way to fix this more
> generally.
> 
> Is there somebody who really understands layers?  Maybe we should file a bug
> against them about "consider having layers default to clipping to their
bounds"
> or something?

Technically it's clipping to the parent's bounds, and the parent needs a layer
too. Perhaps layers should mask to bounds by default, but the parent would still
need a layer. It's probably better to not use layers if we can avoid it, so
that's perhaps how we got here.

+bruthig who understands layers better than me

[bruthig, 2016-02-24 15:37:18]

On 2016/02/24 02:09:22, Evan Stade wrote:
> On 2016/02/24 02:05:50, Peter Kasting wrote:
> > On 2016/02/24 01:55:20, Evan Stade wrote:
> > > On 2016/02/24 01:46:08, Peter Kasting wrote:
> > > > RS LGTM.  I don't really understand anything about layers so I don't
> > actually
> > > > know what this is doing, but it looks plausible.
> > > > 
> > > > I vaguely wonder if we have some of the default settings for views wrong
> > > > regarding any of these, if we're winding up with views drawing outside
> their
> > > > bounds unless we explicitly enable clipping.  Seems like that should be
> the
> > > > default.  But again, I don't know what I'm talking about.
> > > 
> > > I agree. My understanding is that non-layer views typically do clip by
> > default,
> > > but in the case of layers, they don't, so when we go around adding layers
we
> > > find a lot of places that are laying out children outside their own bounds
> and
> > > it looks broken. I'm not sure if there's a good way to fix this more
> > generally.
> > 
> > Is there somebody who really understands layers?  Maybe we should file a bug
> > against them about "consider having layers default to clipping to their
> bounds"
> > or something?
> 
> Technically it's clipping to the parent's bounds, and the parent needs a layer
> too. Perhaps layers should mask to bounds by default, but the parent would
still
> need a layer. It's probably better to not use layers if we can avoid it, so
> that's perhaps how we got here.
> 
> +bruthig who understands layers better than me

Background:
Non-layer View's are clipped to their bounds by virtue of the |canvas| instance
passed to View::OnPaint() from the containing View. However, layers are clipped
to their parent Layer bounds if the parent has SetMaskToBounds(true) set. I'm
not sure of a good solution off the top of my head but I will reach out to some
Layer experts to see if they have any good suggestions.

[bruthig, 2016-02-24 17:02:58]

I spoke with ajuma@ in regards to better solutions than using
View::SetPaintToLayer(true).  He suggested that we use a ui::Layer of type
LAYER_NOT_DRAWN because it is much lighter weight than the LAYER_TEXTURED Layer
that will be used due to the View::SetPaintToLayer(true). However, I looked at
how View would work with a LAYER_NOT_DRAWN type of Layer and I don't think it
will work as-is.

I think we should land this as a short term fix and use http://crbug.com/589497
to track solving this in the more general case.  WDYT?

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

https://codereview.chromium.org/1724333002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:184:
SetPaintToLayer(true);
Calling SetPaintToLayer(true) causes the View to create a
LayerType::LAYER_TEXTURED Layer which does have some associated costs, however,
I don't think View will currently work with a LAYER_NOT_DRAWN type Layer.  I
have filed http://crbug.com/589497 to address this issue on a more general
basis.  I suspect that it will require at least a moderate amount of effort to
solve this in the general case.
Can you add a TODO here (and maybe in the two other spots that use this
technique) that references that bug?

[Evan Stade, 2016-02-24 23:14:18]

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

https://codereview.chromium.org/1724333002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:184:
SetPaintToLayer(true);
On 2016/02/24 17:02:58, bruthig wrote:
> Calling SetPaintToLayer(true) causes the View to create a
> LayerType::LAYER_TEXTURED Layer which does have some associated costs,
however,
> I don't think View will currently work with a LAYER_NOT_DRAWN type Layer.  I
> have filed http://crbug.com/589497 to address this issue on a more general
> basis.  I suspect that it will require at least a moderate amount of effort to
> solve this in the general case.
> Can you add a TODO here (and maybe in the two other spots that use this
> technique) that references that bug?

done. (Only two spots use this technique AFAIK --- the infobar is a special case
as even without layers we had to do a special dance for clipping. This is a
result of the funky shape necessary to draw the arrow.)

https://codereview.chromium.org/1724333002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:187: } else {
On 2016/02/24 01:46:08, Peter Kasting wrote:
> Nit: Can combine this else with subsequent if and flatten to one three-arm
> conditional

Done.

[Evan Stade, 2016-02-24 23:14:42]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-02-24 23:14:42]

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/1724333002/#ps20001
(title: "streamline conditionals, add comments")

[commit-bot: I haz the power, 2016-02-24 23:14:52]

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

[bruthig, 2016-02-24 23:18:26]

lgtm

[commit-bot: I haz the power, 2016-02-25 00:55:50]

Description was changed from

==========
[MD] Don't allow overflowing location icons to appear on top of toolbar

BUG=588331
==========

to

==========
[MD] Don't allow overflowing location icons to appear on top of toolbar

BUG=588331
==========

[commit-bot: I haz the power, 2016-02-25 00:55:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-25 00:57:13]

Description was changed from

==========
[MD] Don't allow overflowing location icons to appear on top of toolbar

BUG=588331
==========

to

==========
[MD] Don't allow overflowing location icons to appear on top of toolbar

BUG=588331

Committed: https://crrev.com/901e730a6fca2e528807b873aaa650be82edc635
Cr-Commit-Position: refs/heads/master@{#377445}
==========

[commit-bot: I haz the power, 2016-02-25 00:57:15]

Patchset 2 (id:??) landed as
https://crrev.com/901e730a6fca2e528807b873aaa650be82edc635
Cr-Commit-Position: refs/heads/master@{#377445}