Use the same shadow for the Android NTP fakebox than for the top omnibox.

Use the same shadow for the Android NTP fakebox than for the top omnibox.

Because it's a nine-patch, this ensures the shadow will be visible when drawing into a bitmap (which disables hardware rendering). It also allows drawing the shadow pre-L.

Screenshot: http://i.imgur.com/dro1OQ7.png

BUG=613088
Committed: https://crrev.com/204f8d869de0afed17f9d7ddd2a8326e449e5ffc
Cr-Commit-Position: refs/heads/master@{#397121}

[Bernhard Bauer, 2016-05-24 15:24:37]

Description was changed from

==========
Draw elevation shadow on the Android NTP manually rather than in hardware.

This ensures the shadow will also be visible when drawing into a bitmap (which
disables hardware rendering).

BUG=613088
==========

to

==========
Draw elevation shadow on the Android NTP fakebox manually rather than in
hardware.

This ensures the shadow will also be visible when drawing into a bitmap (which
disables hardware rendering).

BUG=613088
==========

[Bernhard Bauer, 2016-05-24 15:56:00]

Description was changed from

==========
Draw elevation shadow on the Android NTP fakebox manually rather than in
hardware.

This ensures the shadow will also be visible when drawing into a bitmap (which
disables hardware rendering).

BUG=613088
==========

to

==========
Draw elevation shadow on the Android NTP fakebox manually rather than in
hardware.

This ensures the shadow will be visible when drawing into a bitmap (which
disables hardware rendering). It also allows drawing the shadow pre-L.

BUG=613088
==========

[Bernhard Bauer, 2016-05-24 15:57:54]

bauerb@chromium.org changed reviewers:
+ twellington@chromium.org

[Bernhard Bauer, 2016-05-24 15:57:55]

Please review.

I haven't found a way to make this completely reusable, although that arguably
has more to do with the way we interact with the fakebox here (setting the width
manually).

[Bernhard Bauer, 2016-05-26 16:07:45]

Description was changed from

==========
Draw elevation shadow on the Android NTP fakebox manually rather than in
hardware.

This ensures the shadow will be visible when drawing into a bitmap (which
disables hardware rendering). It also allows drawing the shadow pre-L.

BUG=613088
==========

to

==========
Use the same shadow for the Android NTP fakebox than for the top omnibox.

Because it's a nine-patch, this ensures the shadow will be visible when drawing
into a bitmap (which disables hardware rendering). It also allows drawing the
shadow pre-L.

BUG=613088
==========

[Bernhard Bauer, 2016-05-26 16:13:48]

Description was changed from

==========
Use the same shadow for the Android NTP fakebox than for the top omnibox.

Because it's a nine-patch, this ensures the shadow will be visible when drawing
into a bitmap (which disables hardware rendering). It also allows drawing the
shadow pre-L.

BUG=613088
==========

to

==========
Use the same shadow for the Android NTP fakebox than for the top omnibox.

Because it's a nine-patch, this ensures the shadow will be visible when drawing
into a bitmap (which disables hardware rendering). It also allows drawing the
shadow pre-L.

Screenshot: http://i.imgur.com/dro1OQ7.png

BUG=613088
==========

[Bernhard Bauer, 2016-05-26 16:14:54]

Per off-review discussion, updated to use a nine-patch. PTAL!

[Theresa, 2016-05-26 17:47:26]

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:178:
Drawable background = mSearchBoxView.getBackground();
Can the padding be added via xml? I'm hoping we can avoid all of this manual
calculation.

Since the current background (R.drawable.bg_ntp_search_box) is just being
replaced with R.drawable.textbox, can we change new_tab_page_layout.xml to use
R.drawable.textbox as the search_box background instead?

Just to double check, the screenshots in the email thread were using
R.drawable.textbox as the background?

[Bernhard Bauer, 2016-05-26 19:43:57]

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:178:
Drawable background = mSearchBoxView.getBackground();
On 2016/05/26 17:47:26, Theresa Wellington wrote:
> Can the padding be added via xml? I'm hoping we can avoid all of this manual
> calculation.
> 
> Since the current background (R.drawable.bg_ntp_search_box) is just being
> replaced with R.drawable.textbox, can we change new_tab_page_layout.xml to use
> R.drawable.textbox as the search_box background instead?

The reason I'm doing this in code is that we still have a flag to turn this off
(the shadow goes with the more-material NTP background). Once that flag is
removed, I can switch over to doing everything in XML.

> Just to double check, the screenshots in the email thread were using
> R.drawable.textbox as the background?

That's correct.

[Theresa, 2016-05-26 21:54:41]

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(left):

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:52:

nit: please add this line back.

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:178:
Drawable background = mSearchBoxView.getBackground();
On 2016/05/26 19:43:57, Bernhard Bauer wrote:
> On 2016/05/26 17:47:26, Theresa Wellington wrote:
> > Can the padding be added via xml? I'm hoping we can avoid all of this manual
> > calculation.
> > 
> > Since the current background (R.drawable.bg_ntp_search_box) is just being
> > replaced with R.drawable.textbox, can we change new_tab_page_layout.xml to
use
> > R.drawable.textbox as the search_box background instead?
> 
> The reason I'm doing this in code is that we still have a flag to turn this
off
> (the shadow goes with the more-material NTP background). Once that flag is
> removed, I can switch over to doing everything in XML.
> 
> > Just to double check, the screenshots in the email thread were using
> > R.drawable.textbox as the background?
> 
> That's correct.

Can you use defined dimens that represent the correct new layout params/padding
instead of calculating it?

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:183:
Drawable shadow = resources.getDrawable(R.drawable.textbox);
Resources.getDrawable() is deprecated. Use ApiCompatibilityUtils#getDrawable().

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:187:
// Adjust the margins to remove the padding.
Why does the padding need to be removed just to be added back?

[Bernhard Bauer, 2016-05-27 12:16:41]

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:178:
Drawable background = mSearchBoxView.getBackground();
On 2016/05/26 21:54:41, Theresa Wellington wrote:
> On 2016/05/26 19:43:57, Bernhard Bauer wrote:
> > On 2016/05/26 17:47:26, Theresa Wellington wrote:
> > > Can the padding be added via xml? I'm hoping we can avoid all of this
manual
> > > calculation.
> > > 
> > > Since the current background (R.drawable.bg_ntp_search_box) is just being
> > > replaced with R.drawable.textbox, can we change new_tab_page_layout.xml to
> use
> > > R.drawable.textbox as the search_box background instead?
> > 
> > The reason I'm doing this in code is that we still have a flag to turn this
> off
> > (the shadow goes with the more-material NTP background). Once that flag is
> > removed, I can switch over to doing everything in XML.
> > 
> > > Just to double check, the screenshots in the email thread were using
> > > R.drawable.textbox as the background?
> > 
> > That's correct.
> 
> Can you use defined dimens that represent the correct new layout
params/padding
> instead of calculating it?

Done. Note however that keeping the exactly same positioning of e.g. the search
box contents isn't possible without calculations, because the padding for the
default background is specified in pixels, whereas the padding for the
nine-patch is in dp. I've adjusted the values so that it looks as closely to the
original on an XXHDPI device as possible without having to go into fractional
dp's.

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:183:
Drawable shadow = resources.getDrawable(R.drawable.textbox);
On 2016/05/26 21:54:41, Theresa Wellington wrote:
> Resources.getDrawable() is deprecated. Use
ApiCompatibilityUtils#getDrawable().

Thanks! That code is gone now though.

https://codereview.chromium.org/2006273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:187:
// Adjust the margins to remove the padding.
On 2016/05/26 21:54:41, Theresa Wellington wrote:
> Why does the padding need to be removed just to be added back?

Sorry, that comment should have been a bit more detailed. The |textbox| drawable
extends further than the default one does, because it includes the shadow. So,
in order to keep the apparent border and the contents in the same position, I
have to subtract the extrusion value (which is the |textbox| drawable's padding)
from the margins but add them to the existing padding for the default
background. Anyway, that code is gone now.

[Bernhard Bauer, 2016-05-31 21:18:29]

Theresa, ping?

[Theresa, 2016-06-01 00:26:49]

https://codereview.chromium.org/2006273002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/2006273002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:178:
// Adjust the margins to remove the padding.
nit: does this comment need to get updated?

[Theresa, 2016-06-01 00:26:56]

lgtm

[Bernhard Bauer, 2016-06-01 12:25:58]

https://codereview.chromium.org/2006273002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/2006273002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:178:
// Adjust the margins to remove the padding.
On 2016/06/01 00:26:49, Theresa Wellington wrote:
> nit: does this comment need to get updated?

Yes, updated. Thanks!

[Bernhard Bauer, 2016-06-01 12:26:09]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2016-06-01 12:26:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/2006273002/#ps180001
(title: "comment")

[commit-bot: I haz the power, 2016-06-01 12:26:24]

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

[commit-bot: I haz the power, 2016-06-01 13:11:52]

Description was changed from

==========
Use the same shadow for the Android NTP fakebox than for the top omnibox.

Because it's a nine-patch, this ensures the shadow will be visible when drawing
into a bitmap (which disables hardware rendering). It also allows drawing the
shadow pre-L.

Screenshot: http://i.imgur.com/dro1OQ7.png

BUG=613088
==========

to

==========
Use the same shadow for the Android NTP fakebox than for the top omnibox.

Because it's a nine-patch, this ensures the shadow will be visible when drawing
into a bitmap (which disables hardware rendering). It also allows drawing the
shadow pre-L.

Screenshot: http://i.imgur.com/dro1OQ7.png

BUG=613088
==========

[commit-bot: I haz the power, 2016-06-01 13:11:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-01 13:13:09]

Description was changed from

==========
Use the same shadow for the Android NTP fakebox than for the top omnibox.

Because it's a nine-patch, this ensures the shadow will be visible when drawing
into a bitmap (which disables hardware rendering). It also allows drawing the
shadow pre-L.

Screenshot: http://i.imgur.com/dro1OQ7.png

BUG=613088
==========

to

==========
Use the same shadow for the Android NTP fakebox than for the top omnibox.

Because it's a nine-patch, this ensures the shadow will be visible when drawing
into a bitmap (which disables hardware rendering). It also allows drawing the
shadow pre-L.

Screenshot: http://i.imgur.com/dro1OQ7.png

BUG=613088

Committed: https://crrev.com/204f8d869de0afed17f9d7ddd2a8326e449e5ffc
Cr-Commit-Position: refs/heads/master@{#397121}
==========

[commit-bot: I haz the power, 2016-06-01 13:13:11]

Patchset 10 (id:??) landed as
https://crrev.com/204f8d869de0afed17f9d7ddd2a8326e449e5ffc
Cr-Commit-Position: refs/heads/master@{#397121}