Add feature for condensed NTP tiles.

Add feature for condensed NTP tiles.

* Separate params in feature for small and large screens
* Exposed in chrome:flags
* See bug for screenshots

BUG=688338
Review-Url: https://codereview.chromium.org/2714723002
Cr-Commit-Position: refs/heads/master@{#453251}
Committed: https://chromium.googlesource.com/chromium/src/+/e387a17ee7d04432f8f28efff66b76e9fbe182e7

[Michael van Ouwerkerk, 2017-02-23 14:52:13]

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

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

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

[Michael van Ouwerkerk, 2017-02-23 14:52:43]

mvanouwerkerk@chromium.org changed reviewers:
+ dgn@chromium.org

[Michael van Ouwerkerk, 2017-02-23 14:52:44]

Nicolas, could you take a look please?

[dgn, 2017-02-23 15:17:45]

Would be nice to rebase, yes. I'll land my CL asap

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/tile_view.xml (right):

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/tile_view.xml:29:
android:layout_gravity="top|end"
that makes the badge move to the corner, right? what if the size of the tile
increases but the icon size stays the same. Won't we end up with the badge being
next to the icon instead of on top of it?

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/res/val...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/res/val...
chrome/android/java/res/values/dimens.xml:284: <dimen
name="tile_view_title_margin_top">66dp</dimen>
looks like weird numbers. 64 +- 2 dp essensially? Why this 2dp difference? A
comment would be helpful.

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java
(right):

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java:55:
setPadding(0, 0, 0, 0);
it would be nice to have a todo with a bug to move the measurements to XML once
the experiment is completed. And if that bug also specifies when the experiment
should be completed and the flags removed, so we actually get around to doing
that at some point.

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java:62:
iconParams.setMargins(
is it only the padding that gets reset when the image changes? If the margins
are affected too it will need to be moved to renderIcon()

[commit-bot: I haz the power, 2017-02-23 16:16:48]

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

[commit-bot: I haz the power, 2017-02-23 16:16:49]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2017-02-24 11:31:19]

Thanks for the quick review! Please take another look.

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/tile_view.xml (right):

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/tile_view.xml:29:
android:layout_gravity="top|end"
On 2017/02/23 15:17:44, dgn wrote:
> that makes the badge move to the corner, right? what if the size of the tile
> increases but the icon size stays the same. Won't we end up with the badge
being
> next to the icon instead of on top of it?

Yes, this is just a more flexible way of specifying the top right corner (in LTR
layouts). With this CL we will have the regular layout and the condensed layout.
In the condensed layout, the badge overlaps the icon slightly more. Of course if
we make further changes those could introduce regressions, but for now this is
the more flexible way of specifying the layout, it does not require adjust from
TileView#initialize.

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/res/val...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/res/val...
chrome/android/java/res/values/dimens.xml:284: <dimen
name="tile_view_title_margin_top">66dp</dimen>
On 2017/02/23 15:17:44, dgn wrote:
> looks like weird numbers. 64 +- 2 dp essensially? Why this 2dp difference? A
> comment would be helpful.

This value is moved unchanged from tile_view.xml, I'm not sure what the
reasoning was there. It determines the gap between the icon and the title, which
is (tile_view_title_margin_top 66dp) - (tile_view_icon_margin_top 12dp) -
(tile_view_icon_size 48dp) = 6dp. In the condensed layout this is still 6dp.

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java
(right):

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java:55:
setPadding(0, 0, 0, 0);
On 2017/02/23 15:17:44, dgn wrote:
> it would be nice to have a todo with a bug to move the measurements to XML
once
> the experiment is completed. And if that bug also specifies when the
experiment
> should be completed and the flags removed, so we actually get around to doing
> that at some point.

Done.

https://codereview.chromium.org/2714723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java:62:
iconParams.setMargins(
On 2017/02/23 15:17:44, dgn wrote:
> is it only the padding that gets reset when the image changes? If the margins
> are affected too it will need to be moved to renderIcon()

I haven't seen any problems with this. Looking at the ImageView#setImageDrawable
source code and the methods it calls, it seems to only interact with padding,
not margin.

[Michael van Ouwerkerk, 2017-02-24 11:32:33]

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

[commit-bot: I haz the power, 2017-02-24 11:32:44]

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

[Michael van Ouwerkerk, 2017-02-24 11:34:06]

mvanouwerkerk@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Michael van Ouwerkerk, 2017-02-24 11:34:07]

Alexei, could you take a look for histograms.xml please?

[dgn, 2017-02-24 12:14:41]

lgtm

[Michael van Ouwerkerk, 2017-02-24 13:03:33]

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

[commit-bot: I haz the power, 2017-02-24 13:03:46]

Dry run: 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, 2017-02-24 14:50:29]

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

[commit-bot: I haz the power, 2017-02-24 14:50:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Alexei Svitkine (slow), 2017-02-24 15:46:48]

We now recommend to use field trial params, rather than features, for different
variations of the same overall feature.

Could you use that instead - or is there a reason you prefer features? Note:
Using multiple distinct features in a server-side experiment config has issues
with groups that don't specify any of them not being reported at all.

[tschumann, 2017-02-25 17:05:40]

Description was changed from

==========
Add features for condensed NTP tiles.

* Seperate features for small and large screens
* Exposed in chrome:flags
* See bug for screenshots.

BUG=688338
==========

to

==========
Add features for condensed NTP tiles.

* Seperate features for small and large screens
* Exposed in chrome:flags
* See bug for screenshots.

BUG=688338
==========

[Michael van Ouwerkerk, 2017-02-27 14:18:55]

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

[commit-bot: I haz the power, 2017-02-27 14:19:07]

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

[Michael van Ouwerkerk, 2017-02-27 14:19:27]

Description was changed from

==========
Add features for condensed NTP tiles.

* Seperate features for small and large screens
* Exposed in chrome:flags
* See bug for screenshots.

BUG=688338
==========

to

==========
Add feature for condensed NTP tiles.

* Separate params in feature for small and large screens
* Exposed in chrome:flags
* See bug for screenshots.

BUG=688338
==========

[Michael van Ouwerkerk, 2017-02-27 14:19:44]

Description was changed from

==========
Add feature for condensed NTP tiles.

* Separate params in feature for small and large screens
* Exposed in chrome:flags
* See bug for screenshots.

BUG=688338
==========

to

==========
Add feature for condensed NTP tiles.

* Separate params in feature for small and large screens
* Exposed in chrome:flags
* See bug for screenshots

BUG=688338
==========

[Michael van Ouwerkerk, 2017-02-27 14:23:19]

Thanks Alexei, please take another look.

[commit-bot: I haz the power, 2017-02-27 15:31:14]

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

[commit-bot: I haz the power, 2017-02-27 15:31:15]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-02-27 15:56:27]

lgtm % comments

https://codereview.chromium.org/2714723002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2714723002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:82:
"ntp_condensed_tile_layout_for_small_screens_enabled";
Nit: This param name is awfully long. Can you find a shorter name?

e.g.

condensed_layout_for_small_screens
condensed_layout_for_large_screens

https://codereview.chromium.org/2714723002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:846:
PARAM_NTP_CONDENSED_TILE_LAYOUT_FOR_LARGE_SCREENS_ENABLED, false);
Nit: Maybe make a helper function in this file that wraps this where you just
have to pass in param name and boolean?

This way, the calls will be simplified and easier to read.

[Michael van Ouwerkerk, 2017-02-27 16:53:58]

https://codereview.chromium.org/2714723002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2714723002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:82:
"ntp_condensed_tile_layout_for_small_screens_enabled";
On 2017/02/27 15:56:27, Alexei Svitkine (slow) wrote:
> Nit: This param name is awfully long. Can you find a shorter name?
> 
> e.g.
> 
> condensed_layout_for_small_screens
> condensed_layout_for_large_screens

Done. We can do without the "ntp" part if you prefer, but the "tile" part helps
distinguish it from a related experiment that we want to control separately.

https://codereview.chromium.org/2714723002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:846:
PARAM_NTP_CONDENSED_TILE_LAYOUT_FOR_LARGE_SCREENS_ENABLED, false);
On 2017/02/27 15:56:27, Alexei Svitkine (slow) wrote:
> Nit: Maybe make a helper function in this file that wraps this where you just
> have to pass in param name and boolean?
> 
> This way, the calls will be simplified and easier to read.

I'm not sure another helper function would help much, as both
shouldUseCondensedTileLayout() and
ChromeFeatureList.getFieldTrialParamByFeatureAsBoolean() are already helper
functions, and not that complex. The shorter param name helps a little though.

[Michael van Ouwerkerk, 2017-02-27 16:56:01]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2017-02-27 16:56:02]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2714723002/#ps80001
(title: "Cleanups.")

[commit-bot: I haz the power, 2017-02-27 16:56:20]

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, 2017-02-27 17:56:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488214561413940,
"parent_rev": "7b9838e8d245af689d35101e3ed3d64c03931e3a", "commit_rev":
"e387a17ee7d04432f8f28efff66b76e9fbe182e7"}

[commit-bot: I haz the power, 2017-02-27 17:56:49]

Description was changed from

==========
Add feature for condensed NTP tiles.

* Separate params in feature for small and large screens
* Exposed in chrome:flags
* See bug for screenshots

BUG=688338
==========

to

==========
Add feature for condensed NTP tiles.

* Separate params in feature for small and large screens
* Exposed in chrome:flags
* See bug for screenshots

BUG=688338

Review-Url: https://codereview.chromium.org/2714723002
Cr-Commit-Position: refs/heads/master@{#453251}
Committed:
https://chromium.googlesource.com/chromium/src/+/e387a17ee7d04432f8f28efff66b...
==========

[commit-bot: I haz the power, 2017-02-27 17:56:51]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e387a17ee7d04432f8f28efff66b...