Views - draw omnibox popup shadows

Views - draw omnibox popup shadows

They come out looking a little different than the png assets we were
using, which is intentional. Screenshots at 1x and 2x are sgabriel-
approved.

BUG=649545
TBR=oshima@chromium.org

Committed: https://crrev.com/10019689c629250717e1035fcdd9365852109ff6
Cr-Commit-Position: refs/heads/master@{#421266}

[Evan Stade, 2016-09-23 19:03:07]

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

[Evan Stade, 2016-09-23 19:03:56]

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

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

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

[Peter Kasting, 2016-09-23 19:38:44]

Might have been nice to rip out the relevant pre-MD bits separately, but
whatever.

LGTM

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:59:
std::max(height, shadow.y() + static_cast<int>(shadow.blur()) / 2);
Functionally, shouldn't this be more like "ceil(blur() / 2)" to make sure we
cover all affected pixels?

Nit: Prefer gfx::ToRoundedInt() family of functions over direct static-casting,
to make the intent around rounding/flooring etc. clear.

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:122: // Blur by
1dp. See comment below about blur accounting.
Argument for another day: It sure is weird to me that we use a 1 dp blur atop
the omnibox instead of just making the toolbar bottom edge the omnibox top edge.
 Would love to kill the "top shadow" idea entirely.

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:126:
g_top_shadow.Get() = gfx::ImageSkia(source, source->size());
Musing: Wow, we really should fix
https://bugs.chromium.org/p/chromium/issues/detail?id=637898 .

All these .Get()s everywhere are sure ugly too.  Can't wait for C++20 to solve
the operator.() issue :P

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:137:
shadows.emplace_back(gfx::Vector2d(0, 0), 2 * kSmallShadowBlur,
Nit: Omit 0, 0

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:264: // necessary
is the client edge thickness plus the shadow height.
Nit: Wondering if the last sentence just restates the prior one + the code too
much

[commit-bot: I haz the power, 2016-09-23 20:32:34]

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

[commit-bot: I haz the power, 2016-09-23 20:32:35]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-09-26 18:11:43]

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:59:
std::max(height, shadow.y() + static_cast<int>(shadow.blur()) / 2);
On 2016/09/23 19:38:44, Peter Kasting wrote:
> Functionally, shouldn't this be more like "ceil(blur() / 2)" to make sure we
> cover all affected pixels?
> 
> Nit: Prefer gfx::ToRoundedInt() family of functions over direct
static-casting,
> to make the intent around rounding/flooring etc. clear.

the blur value should always be even, but I've updated to use ToCeiledInt.

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:122: // Blur by
1dp. See comment below about blur accounting.
On 2016/09/23 19:38:44, Peter Kasting wrote:
> Argument for another day: It sure is weird to me that we use a 1 dp blur atop
> the omnibox instead of just making the toolbar bottom edge the omnibox top
edge.
>  Would love to kill the "top shadow" idea entirely.

that works better when there's no bookmark bar --- with the bookmarks showing
there's not a horizontal rule to fall back on.

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:137:
shadows.emplace_back(gfx::Vector2d(0, 0), 2 * kSmallShadowBlur,
On 2016/09/23 19:38:44, Peter Kasting wrote:
> Nit: Omit 0, 0

Done.

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:264: // necessary
is the client edge thickness plus the shadow height.
On 2016/09/23 19:38:44, Peter Kasting wrote:
> Nit: Wondering if the last sentence just restates the prior one + the code too
> much

Done.

[Evan Stade, 2016-09-26 18:11:46]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-09-26 18:11:46]

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/2364163002/#ps60001
(title: "pkasting review")

[Peter Kasting, 2016-09-26 19:57:34]

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/2364163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:122: // Blur by
1dp. See comment below about blur accounting.
On 2016/09/26 18:11:42, Evan Stade wrote:
> On 2016/09/23 19:38:44, Peter Kasting wrote:
> > Argument for another day: It sure is weird to me that we use a 1 dp blur
atop
> > the omnibox instead of just making the toolbar bottom edge the omnibox top
> edge.
> >  Would love to kill the "top shadow" idea entirely.
> 
> that works better when there's no bookmark bar --- with the bookmarks showing
> there's not a horizontal rule to fall back on.

Yeah, we'd have to draw it ourselves, I just think the "shadow" effect looks
strange compared to the 1 px stroke, and it's weird it's not in the same
position as the no-bookmark-bar stroke.

Whatever.  Not for this CL.

[Evan Stade, 2016-09-26 22:20:34]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-09-26 22:20:35]

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/2364163002/#ps80001
(title: "shorten comment")

[commit-bot: I haz the power, 2016-09-26 22:21:17]

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, 2016-09-26 22:33:04]

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

[commit-bot: I haz the power, 2016-09-26 22:33:05]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2016-09-27 17:37:14]

Description was changed from

==========
Views - draw omnibox popup shadows

They come out looking a little different than the png assets we were
using, which is intentional. Screenshots at 1x and 2x are sgabriel-
approved.

BUG=649545
==========

to

==========
Views - draw omnibox popup shadows

They come out looking a little different than the png assets we were
using, which is intentional. Screenshots at 1x and 2x are sgabriel-
approved.

BUG=649545
TBR=oshima@chromium.org
==========

[Evan Stade, 2016-09-27 17:37:14]

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

[Evan Stade, 2016-09-27 17:37:25]

tbr oshima for c/a/t (deletions only)

[Evan Stade, 2016-09-27 17:37:33]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-27 17:38:05]

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, 2016-09-27 18:08:51]

Description was changed from

==========
Views - draw omnibox popup shadows

They come out looking a little different than the png assets we were
using, which is intentional. Screenshots at 1x and 2x are sgabriel-
approved.

BUG=649545
TBR=oshima@chromium.org
==========

to

==========
Views - draw omnibox popup shadows

They come out looking a little different than the png assets we were
using, which is intentional. Screenshots at 1x and 2x are sgabriel-
approved.

BUG=649545
TBR=oshima@chromium.org
==========

[commit-bot: I haz the power, 2016-09-27 18:08:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-27 18:17:12]

Description was changed from

==========
Views - draw omnibox popup shadows

They come out looking a little different than the png assets we were
using, which is intentional. Screenshots at 1x and 2x are sgabriel-
approved.

BUG=649545
TBR=oshima@chromium.org
==========

to

==========
Views - draw omnibox popup shadows

They come out looking a little different than the png assets we were
using, which is intentional. Screenshots at 1x and 2x are sgabriel-
approved.

BUG=649545
TBR=oshima@chromium.org

Committed: https://crrev.com/10019689c629250717e1035fcdd9365852109ff6
Cr-Commit-Position: refs/heads/master@{#421266}
==========

[commit-bot: I haz the power, 2016-09-27 18:17:13]

Patchset 5 (id:??) landed as
https://crrev.com/10019689c629250717e1035fcdd9365852109ff6
Cr-Commit-Position: refs/heads/master@{#421266}