Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2312253003:
Add Field Trial factor to NewTabPageLayout calculations. (Closed)
DescriptionAdd Field Trial factor to NewTabPageLayout calculations.
Add support for a field trial that pushes the first card upwards and disables
it's peeking to make the Cards UI more discoverable.
BUG=622975
Committed: https://crrev.com/711590dd54eb9f369e8f94f281a028849ccfeaf2
Cr-Commit-Position: refs/heads/master@{#417006}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Nits. #
Total comments: 3
Patch Set 3 : Deduplicate field trial name. #
Messages
Total messages: 26 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
peconn@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
PTAL https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:246: return; This is duplicating the body of the if statement above, but this is easier to merge.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with nits https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java (right): https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:16: public class CardsFieldTrial { This is a static utility class (like VariationsAssociatedData), it should be final and have a private constructor. https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:19: private static final String FIELD_TRIAL_NAME = "cards"; We have a document named "Variation parameters", I wonder whether this name is aligned with that? For example, I kind of expected to see "NTPSnippets" here. https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:24: * with a command line flag). It will return 0f is there is no such field trial. s/is there/if there/ https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:36: return Integer.parseInt(value); This only ever returns an int. Why does getFirstCardOffsetDp have a float as return value? https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:246: return; On 2016/09/06 16:46:15, PEConn wrote: > This is duplicating the body of the if statement above, but this is easier to > merge. That seems ok anyway, this is easier to read.
treib@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java (right): https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:19: private static final String FIELD_TRIAL_NAME = "cards"; On 2016/09/07 10:51:50, Michael van Ouwerkerk wrote: > We have a document named "Variation parameters", I wonder whether this name is > aligned with that? For example, I kind of expected to see "NTPSnippets" here. Yes, please use "NTPSnippets". Once your CL lands, please update the doc: https://docs.google.com/spreadsheets/d/1TeNLcMHWG2B-lRbYFtYwPr6PaT8dGXaBIABPS...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
peconn@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java (right): https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:16: public class CardsFieldTrial { On 2016/09/07 10:51:50, Michael van Ouwerkerk wrote: > This is a static utility class (like VariationsAssociatedData), it should be > final and have a private constructor. Done. https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:19: private static final String FIELD_TRIAL_NAME = "cards"; On 2016/09/07 10:51:50, Michael van Ouwerkerk wrote: > We have a document named "Variation parameters", I wonder whether this name is > aligned with that? For example, I kind of expected to see "NTPSnippets" here. Done. https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:24: * with a command line flag). It will return 0f is there is no such field trial. On 2016/09/07 10:51:51, Michael van Ouwerkerk wrote: > s/is there/if there/ Done. https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:36: return Integer.parseInt(value); On 2016/09/07 10:51:50, Michael van Ouwerkerk wrote: > This only ever returns an int. Why does getFirstCardOffsetDp have a float as > return value? Done. https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2312253003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:246: return; On 2016/09/07 10:51:51, Michael van Ouwerkerk wrote: > On 2016/09/06 16:46:15, PEConn wrote: > > This is duplicating the body of the if statement above, but this is easier to > > merge. > > That seems ok anyway, this is easier to read. Acknowledged.
https://codereview.chromium.org/2312253003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java (right): https://codereview.chromium.org/2312253003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:19: private static final String FIELD_TRIAL_NAME = "NTPSnippets"; This constant already exists in NewTabPage.java: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Can we reuse the value? (not a huge deal if we can't)
https://codereview.chromium.org/2312253003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java (right): https://codereview.chromium.org/2312253003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:19: private static final String FIELD_TRIAL_NAME = "NTPSnippets"; On 2016/09/07 16:15:38, Marc Treib wrote: > This constant already exists in NewTabPage.java: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > Can we reuse the value? (not a huge deal if we can't) Done - I'll move it and all uses into this class in a future CL (I want to touch as few files as possible).
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM (for completeness, not that you need it :) ) https://codereview.chromium.org/2312253003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java (right): https://codereview.chromium.org/2312253003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsFieldTrial.java:19: private static final String FIELD_TRIAL_NAME = "NTPSnippets"; On 2016/09/07 16:25:36, PEConn wrote: > On 2016/09/07 16:15:38, Marc Treib wrote: > > This constant already exists in NewTabPage.java: > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > Can we reuse the value? (not a huge deal if we can't) > > Done - I'll move it and all uses into this class in a future CL (I want to touch > as few files as possible). Alright, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2312253003/#ps80001 (title: "Deduplicate field trial name.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add Field Trial factor to NewTabPageLayout calculations. Add support for a field trial that pushes the first card upwards and disables it's peeking to make the Cards UI more discoverable. BUG=622975 ========== to ========== Add Field Trial factor to NewTabPageLayout calculations. Add support for a field trial that pushes the first card upwards and disables it's peeking to make the Cards UI more discoverable. BUG=622975 Committed: https://crrev.com/711590dd54eb9f369e8f94f281a028849ccfeaf2 Cr-Commit-Position: refs/heads/master@{#417006} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/711590dd54eb9f369e8f94f281a028849ccfeaf2 Cr-Commit-Position: refs/heads/master@{#417006} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
