[MD] replace infobar bottom solid line separator with a shadow.

[MD] replace infobar bottom solid line separator with a shadow.

The solid line separator is retained for both the top and between infobars.

Now both the bottom and the top have pieces that overlap the rest of the
browser. On the top, it's the arrow, and on the bottom, it's the shadow.

This changes the layout of InfoBarContainerView such that it's always
sized with zero overlap. This simplifies layout in the browser view and
is now possible because layers allow child views to draw outside their
bounds. This also makes event hit testing simpler (no need to override
the event targeter for the container).

BUG=520266
Committed: https://crrev.com/ed6800f22cb908b03fa72d876296a87e1e747b48
Cr-Commit-Position: refs/heads/master@{#381582}

[Evan Stade, 2016-03-16 00:52:56]

Description was changed from

==========
[MD] replace infobar bottom solid line separator with a shadow.

Now both the bottom and the top have pieces that overlap the rest of the
browser. On the top, it's the arrow, and on the bottom, it's the shadow.

This changes the layout of InfoBarContainerView such that it's always
sized with zero overlap. This simplifies layout in the browser view and
is now possible because layers allow child views to draw outside their
bounds. This also makes event hit testing simpler (no need to override
the event targeter for the container).

BUG=520266
==========

to

==========
[MD] replace infobar bottom solid line separator with a shadow.

The solid line separator is retained for both the top and between infobars.

Now both the bottom and the top have pieces that overlap the rest of the
browser. On the top, it's the arrow, and on the bottom, it's the shadow.

This changes the layout of InfoBarContainerView such that it's always
sized with zero overlap. This simplifies layout in the browser view and
is now possible because layers allow child views to draw outside their
bounds. This also makes event hit testing simpler (no need to override
the event targeter for the container).

BUG=520266
==========

[Evan Stade, 2016-03-16 00:52:57]

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

[Evan Stade, 2016-03-16 00:54:01]

+pkasting for review
+sky for (minor) changes to ui/

https://codereview.chromium.org/1800373002/diff/1/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (left):

https://codereview.chromium.org/1800373002/diff/1/ui/gfx/skia_util.cc#oldcode133
ui/gfx/skia_util.cc:133: grad_points, grad_colors, NULL, 2,
SkShader::kRepeat_TileMode));
this utility function is only used in a couple places right now, and in those
places the edge behavior doesn't matter because the grad points match the edges
of the fill area.

https://codereview.chromium.org/1800373002/diff/1/ui/views/view.h
File ui/views/view.h (left):

https://codereview.chromium.org/1800373002/diff/1/ui/views/view.h#oldcode237
ui/views/view.h:237: gfx::Rect GetLayerBoundsInPixel() const;
unrelated to this CL; unused

[Peter Kasting, 2016-03-16 03:10:27]

LGTM

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/infobar_container_view.cc (right):

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:29: ContentShadow() {
Nit: I tend to find out-of-line definitions of methods even for file-scope
classes more readable, but the style guide allows both, so up to you.

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:36: void
OnPaint(gfx::Canvas* canvas) override {
Nit: Could be private

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:39:
skia::RefPtr<SkShader> shader = gfx::CreateGradientShader(
I assume we want a gradient shader because at scales 2x and above it will look
better than using a solid color for this one?

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:41:
SkColorSetA(SK_ColorBLACK, 0));
Nit: Could use SK_AlphaTRANSPARENT in place of 0, that might read more clearly
(or not) (2 places)

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:82: int overlap =
GetVerticalOverlap(&total_height);
Nit: Can inline into next line

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:113: i ==
child_count() - 2) {
Nit: It might be good for a comment to explain why this works, since it looks a
bit cryptic/fragile.

[sky, 2016-03-16 15:39:43]

https://codereview.chromium.org/1800373002/diff/20001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/1800373002/diff/20001/ui/gfx/skia_util.cc#new...
ui/gfx/skia_util.cc:133: grad_points, grad_colors, NULL, 2,
SkShader::kClamp_TileMode));
Why does this change matter?

[Evan Stade, 2016-03-16 19:29:52]

will upload new patchset soon, for now just responding to comments

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/infobar_container_view.cc (right):

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:29: ContentShadow() {
On 2016/03/16 03:10:27, Peter Kasting wrote:
> Nit: I tend to find out-of-line definitions of methods even for file-scope
> classes more readable, but the style guide allows both, so up to you.

Acknowledged.

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:36: void
OnPaint(gfx::Canvas* canvas) override {
On 2016/03/16 03:10:27, Peter Kasting wrote:
> Nit: Could be private

I guess I should make it protected. I prefer to keep the same visibility as what
the parent class defines, rather than defining to private, because to me private
suggests that it will only be called from within this class (which is false).

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:39:
skia::RefPtr<SkShader> shader = gfx::CreateGradientShader(
On 2016/03/16 03:10:27, Peter Kasting wrote:
> I assume we want a gradient shader because at scales 2x and above it will look
> better than using a solid color for this one?

right, that's why I decided against drawing a solid color. It's possible this
could be simplified to a single gradient with extra points in it, but this is
more of a verbatim copy of Sebastien's shadow definitions (which is useful I
guess in case he wants to tweak some value it should be easy to tweak here as
well).

Because we're drawing to a layer paints are pretty uncommon (unless you're
resizing the browser I guess?).

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:41:
SkColorSetA(SK_ColorBLACK, 0));
On 2016/03/16 03:10:27, Peter Kasting wrote:
> Nit: Could use SK_AlphaTRANSPARENT in place of 0, that might read more clearly
> (or not) (2 places)

will give it a go.

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:82: int overlap =
GetVerticalOverlap(&total_height);
On 2016/03/16 03:10:27, Peter Kasting wrote:
> Nit: Can inline into next line

perhaps this works but it's not clear to me that it works (due to order of
operations):

total_height -= overlap - GetVerticalOverlap(&total_height);

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:113: i ==
child_count() - 2) {
On 2016/03/16 03:10:27, Peter Kasting wrote:
> Nit: It might be good for a comment to explain why this works, since it looks
a
> bit cryptic/fragile.

will add

https://codereview.chromium.org/1800373002/diff/20001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/1800373002/diff/20001/ui/gfx/skia_util.cc#new...
ui/gfx/skia_util.cc:133: grad_points, grad_colors, NULL, 2,
SkShader::kClamp_TileMode));
On 2016/03/16 15:39:42, sky wrote:
> Why does this change matter?

ASCII art of what we want is something like:

---------------------
black 10%
black 10%
black 10%
black 10%
black 8%
black 6%
black 4%
black 2%
black 0%
---------------------

So we define the middle gradient point and the final gradient point. Clamp means
the black 10% is then repeated all the way to the top edge. With tile, this
would end up looking something like

---------------------
black 4%
black 2%
black 0%
black 10%
black 8%
black 6%
black 4%
black 2%
black 0%
---------------------

[Evan Stade, 2016-03-16 21:22:01]

updated

[sky, 2016-03-16 21:56:55]

Assuming the skia_util change won't effect other places LGTM

https://codereview.chromium.org/1800373002/diff/20001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/1800373002/diff/20001/ui/gfx/skia_util.cc#new...
ui/gfx/skia_util.cc:133: grad_points, grad_colors, NULL, 2,
SkShader::kClamp_TileMode));
On 2016/03/16 19:29:52, Evan Stade wrote:
> On 2016/03/16 15:39:42, sky wrote:
> > Why does this change matter?
> 
> ASCII art of what we want is something like:
> 
> ---------------------
> black 10%
> black 10%
> black 10%
> black 10%
> black 8%
> black 6%
> black 4%
> black 2%
> black 0%
> ---------------------
> 
> So we define the middle gradient point and the final gradient point. Clamp
means
> the black 10% is then repeated all the way to the top edge. With tile, this
> would end up looking something like
> 
> ---------------------
> black 4%
> black 2%
> black 0%
> black 10%
> black 8%
> black 6%
> black 4%
> black 2%
> black 0%
> ---------------------

Are you sure other call sites don't want the current behavior?

[Evan Stade, 2016-03-16 21:59:41]

On 2016/03/16 21:56:55, sky wrote:
> Assuming the skia_util change won't effect other places LGTM
> 
> https://codereview.chromium.org/1800373002/diff/20001/ui/gfx/skia_util.cc
> File ui/gfx/skia_util.cc (right):
> 
>
https://codereview.chromium.org/1800373002/diff/20001/ui/gfx/skia_util.cc#new...
> ui/gfx/skia_util.cc:133: grad_points, grad_colors, NULL, 2,
> SkShader::kClamp_TileMode));
> On 2016/03/16 19:29:52, Evan Stade wrote:
> > On 2016/03/16 15:39:42, sky wrote:
> > > Why does this change matter?
> > 
> > ASCII art of what we want is something like:
> > 
> > ---------------------
> > black 10%
> > black 10%
> > black 10%
> > black 10%
> > black 8%
> > black 6%
> > black 4%
> > black 2%
> > black 0%
> > ---------------------
> > 
> > So we define the middle gradient point and the final gradient point. Clamp
> means
> > the black 10% is then repeated all the way to the top edge. With tile, this
> > would end up looking something like
> > 
> > ---------------------
> > black 4%
> > black 2%
> > black 0%
> > black 10%
> > black 8%
> > black 6%
> > black 4%
> > black 2%
> > black 0%
> > ---------------------
> 
> Are you sure other call sites don't want the current behavior?

There are only two callsites I found and they don't care about the behavior
because they define gradient points that match the edge points. So yes, pretty
sure.

[Evan Stade, 2016-03-16 22:00:24]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-03-16 22:00:25]

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/1800373002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2016-03-16 22:00:55]

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

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

Description was changed from

==========
[MD] replace infobar bottom solid line separator with a shadow.

The solid line separator is retained for both the top and between infobars.

Now both the bottom and the top have pieces that overlap the rest of the
browser. On the top, it's the arrow, and on the bottom, it's the shadow.

This changes the layout of InfoBarContainerView such that it's always
sized with zero overlap. This simplifies layout in the browser view and
is now possible because layers allow child views to draw outside their
bounds. This also makes event hit testing simpler (no need to override
the event targeter for the container).

BUG=520266
==========

to

==========
[MD] replace infobar bottom solid line separator with a shadow.

The solid line separator is retained for both the top and between infobars.

Now both the bottom and the top have pieces that overlap the rest of the
browser. On the top, it's the arrow, and on the bottom, it's the shadow.

This changes the layout of InfoBarContainerView such that it's always
sized with zero overlap. This simplifies layout in the browser view and
is now possible because layers allow child views to draw outside their
bounds. This also makes event hit testing simpler (no need to override
the event targeter for the container).

BUG=520266
==========

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

Committed patchset #3 (id:40001)

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

Description was changed from

==========
[MD] replace infobar bottom solid line separator with a shadow.

The solid line separator is retained for both the top and between infobars.

Now both the bottom and the top have pieces that overlap the rest of the
browser. On the top, it's the arrow, and on the bottom, it's the shadow.

This changes the layout of InfoBarContainerView such that it's always
sized with zero overlap. This simplifies layout in the browser view and
is now possible because layers allow child views to draw outside their
bounds. This also makes event hit testing simpler (no need to override
the event targeter for the container).

BUG=520266
==========

to

==========
[MD] replace infobar bottom solid line separator with a shadow.

The solid line separator is retained for both the top and between infobars.

Now both the bottom and the top have pieces that overlap the rest of the
browser. On the top, it's the arrow, and on the bottom, it's the shadow.

This changes the layout of InfoBarContainerView such that it's always
sized with zero overlap. This simplifies layout in the browser view and
is now possible because layers allow child views to draw outside their
bounds. This also makes event hit testing simpler (no need to override
the event targeter for the container).

BUG=520266

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

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

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

[Peter Kasting, 2016-03-17 03:30:02]

(I forgot to mail this reply agreeing with you)

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/infobar_container_view.cc (right):

https://codereview.chromium.org/1800373002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/infobar_container_view.cc:82: int overlap =
GetVerticalOverlap(&total_height);
On 2016/03/16 19:29:51, Evan Stade wrote:
> On 2016/03/16 03:10:27, Peter Kasting wrote:
> > Nit: Can inline into next line
> 
> perhaps this works but it's not clear to me that it works (due to order of
> operations):
> 
> total_height -= overlap - GetVerticalOverlap(&total_height);

Oh, never mind, I didn't even see that the call was affecting |total_height|. 
Yeah, it would work correctly, but leave them separate.