Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671793003:
Turn Popular sites on by default. (Closed)
|
Created:
3 years, 10 months ago by noyau (Ping after 24h) Modified:
3 years, 10 months ago CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTurn Popular sites on by default.
In M56 stable Popular Sites is turned on for 10% of the users, plus 10% in a
control group. The 80% left have the feature turned off. This CL changes the
default behavior to have the feature turned on. The distribution in M57 stable
after this CL is to keep the two 10% groups (to do long term retention
analysis) but to turn the 80% in the default group on. In short, the
feature will be turned on for 90% of users.
On canary, dev, and beta, M56 is turned at 50%. This CL changes the behavior to
simply be on for everyone.
BUG=660123
Review-Url: https://codereview.chromium.org/2671793003
Cr-Commit-Position: refs/heads/master@{#451755}
Committed: https://chromium.googlesource.com/chromium/src/+/41bd1080dc17dddd6b39b058c36165f22033a83c
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebase and feedback. #Messages
Total messages: 14 (6 generated)
noyau@chromium.org changed reviewers: + rkaplow@chromium.org, sfiera@chromium.org
To reviewers: rkaplow: To make sure I'm not doing anything stupid. sfiera: for owner
Description was changed from ========== Turn Popular sites on by default. In M56 stable Popular Sites is turned on for 10% of the users, plus 10% in a control group. The 80% left have the feature turned off. This CL changes the default behavior to have the feature turned on. The distribution in M57 stable after this CL is to keep the two 10% groups (to do long term retention analysis) but to turn the 80% in the default group on. In short, the feature will be turned on for 90% of users. On canary, dev, and beta, M56 is turned at 50%. This CL changes the behavior to simply be on for everyone. BUG=660123 ========== to ========== Turn Popular sites on by default. In M56 stable Popular Sites is turned on for 10% of the users, plus 10% in a control group. The 80% left have the feature turned off. This CL changes the default behavior to have the feature turned on. The distribution in M57 stable after this CL is to keep the two 10% groups (to do long term retention analysis) but to turn the 80% in the default group on. In short, the feature will be turned on for 90% of users. On canary, dev, and beta, M56 is turned at 50%. This CL changes the behavior to simply be on for everyone. BUG=660123 ==========
https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... File components/ntp_tiles/field_trial.cc (right): https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... components/ntp_tiles/field_trial.cc:46: // The experiment is only for stable. I wouldn't describe this as an experiment anymore with 90% enabled. It looks like you're keeping around a holdback so that you can continue to get metrics. I'd describe it that way. https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... components/ntp_tiles/field_trial.cc:101: // The experiment is enabled by default. This should be restricted to iOS.
lgtm looks ok % comments https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... File components/ntp_tiles/field_trial.cc (right): https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... components/ntp_tiles/field_trial.cc:47: if (!is_stable_channel) maybe also comment on intended behavior of other channels
Chris, PTAL. https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... File components/ntp_tiles/field_trial.cc (right): https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... components/ntp_tiles/field_trial.cc:46: // The experiment is only for stable. On 2017/02/03 14:02:34, sfiera wrote: > I wouldn't describe this as an experiment anymore with 90% enabled. It looks > like you're keeping around a holdback so that you can continue to get metrics. > I'd describe it that way. It's an experiment to keep 10% disabled :) https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... components/ntp_tiles/field_trial.cc:47: if (!is_stable_channel) On 2017/02/07 03:56:46, rkaplow wrote: > maybe also comment on intended behavior of other channels Done. https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... components/ntp_tiles/field_trial.cc:101: // The experiment is enabled by default. On 2017/02/03 14:02:34, sfiera wrote: > This should be restricted to iOS. Done. But is this really true? Does that mean that Android is off by default? I thought the feature was always turned on for Android?
LGTM https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... File components/ntp_tiles/field_trial.cc (right): https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... components/ntp_tiles/field_trial.cc:101: // The experiment is enabled by default. On 2017/02/21 12:24:16, noyau wrote: > On 2017/02/03 14:02:34, sfiera wrote: > > This should be restricted to iOS. > Done. > > But is this really true? Does that mean that Android is off by default? I > thought the feature was always turned on for Android? Nope. We still don't have launch approval for all countries.
On 2017/02/21 13:10:22, sfiera wrote: > LGTM > > https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... > File components/ntp_tiles/field_trial.cc (right): > > https://codereview.chromium.org/2671793003/diff/1/components/ntp_tiles/field_... > components/ntp_tiles/field_trial.cc:101: // The experiment is enabled by > default. > On 2017/02/21 12:24:16, noyau wrote: > > On 2017/02/03 14:02:34, sfiera wrote: > > > This should be restricted to iOS. > > Done. > > > > But is this really true? Does that mean that Android is off by default? I > > thought the feature was always turned on for Android? > > Nope. We still don't have launch approval for all countries. Ha. Good point. And good catch.
The CQ bit was checked by noyau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2671793003/#ps20001 (title: "Rebase and feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487684462477620,
"parent_rev": "9c58d92ce27ae171141b775599361cfb769a692e", "commit_rev":
"41bd1080dc17dddd6b39b058c36165f22033a83c"}
Message was sent while issue was closed.
Description was changed from ========== Turn Popular sites on by default. In M56 stable Popular Sites is turned on for 10% of the users, plus 10% in a control group. The 80% left have the feature turned off. This CL changes the default behavior to have the feature turned on. The distribution in M57 stable after this CL is to keep the two 10% groups (to do long term retention analysis) but to turn the 80% in the default group on. In short, the feature will be turned on for 90% of users. On canary, dev, and beta, M56 is turned at 50%. This CL changes the behavior to simply be on for everyone. BUG=660123 ========== to ========== Turn Popular sites on by default. In M56 stable Popular Sites is turned on for 10% of the users, plus 10% in a control group. The 80% left have the feature turned off. This CL changes the default behavior to have the feature turned on. The distribution in M57 stable after this CL is to keep the two 10% groups (to do long term retention analysis) but to turn the 80% in the default group on. In short, the feature will be turned on for 90% of users. On canary, dev, and beta, M56 is turned at 50%. This CL changes the behavior to simply be on for everyone. BUG=660123 Review-Url: https://codereview.chromium.org/2671793003 Cr-Commit-Position: refs/heads/master@{#451755} Committed: https://chromium.googlesource.com/chromium/src/+/41bd1080dc17dddd6b39b058c361... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/41bd1080dc17dddd6b39b058c361... |
