Hard coded finch config for Popular site

Hard coded finch config for Popular site.

This is hard-coded in order to be available on first launch.

BUG=662350
R=asvitkine@chromium.org, rkaplow@chromium.org, sfiera@chromium.org

Committed: https://crrev.com/4793d9335b9ce611e2deb40863793fa785773200
Cr-Commit-Position: refs/heads/master@{#432511}

[noyau (Ping after 24h), 2016-11-08 14:07:43]

Patchset #1 (id:1) has been deleted

[noyau (Ping after 24h), 2016-11-09 10:29:38]

Patchset #1 (id:20001) has been deleted

[noyau (Ping after 24h), 2016-11-09 10:29:43]

Patchset #1 (id:40001) has been deleted

[noyau (Ping after 24h), 2016-11-09 10:50:11]

Description was changed from

==========
Finch stuff.

BUG=
==========

to

==========
Hard coded finch config for Popular site.

This is hard-coded in order to be available on first launch.

BUG=660123
==========

[noyau (Ping after 24h), 2016-11-09 10:56:38]

noyau@chromium.org changed reviewers:
+ rkaplow@chromium.org, sfiera@chromium.org

[noyau (Ping after 24h), 2016-11-09 10:56:39]

Hi Robert,

You've been designed as my finch ambassador, as such I'm sending you this CL and
adding you on the launch bug.

I'd like validation on this implementation of a hard coded finch experiment. On
iOS we can't unfortunately use the finch provided first run experiment framework
as it is not available. For this feature I instead hard coded the experiment,
but I have some questions below about things I'm unsure of.

What I'd like to understand is if it is possible to plan to override the
experiment from the finch server on subsequents runs, to eventually turn
everything off?

Thanks for your insight.

-- Éric

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
File components/ntp_tiles/field_trial.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:38: // Experiment enabled until March 15,
2017. By default, disabled.
Before creating the new experiment should I check that if it has been overriden
by a server side config? Or should I plan for two experiment, this one hardcoded
and an emergency one to turn everything off from the server side?

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:89: ShouldShowPopularSites()) {
I changed the order of the test, to only enroll in the experiment users who are
going to be shown popular site tiles. There is no point of counting existing
users who already have a bunch of tiles.

[noyau (Ping after 24h), 2016-11-09 13:55:49]

Description was changed from

==========
Hard coded finch config for Popular site.

This is hard-coded in order to be available on first launch.

BUG=660123
==========

to

==========
Hard coded finch config for Popular site.

This is hard-coded in order to be available on first launch.

BUG=662350
==========

[rkaplow, 2016-11-10 22:56:45]

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

[rkaplow, 2016-11-10 22:56:46]

>
What I'd like to understand is if it is possible to plan to override the
experiment from the finch server on subsequents runs, to eventually turn
everything off?

Regarding this, I don't see how we can "turn everything off", since there will
continue to be new users of iOS hitting first run - that is, assuming you mean
delete the code for the client side experiment config. I do think this should
use the server side config when possible though, if it needs to be controlled on
both first run and regular usage.

Overall I think this looks ok, but I'd prefer adding asvitkine to look as well
since I've not seen experiments on ios that are used both on first run and
regular usage... maybe there is a gotcha I am unaware of.

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
File components/ntp_tiles/field_trial.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:31: void
ntp_tiles::SetUpFirstLaunchFieldTrial(bool is_stable_channel) {
can you comment this is done as a workaround since ios first launch experiments
are not available

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:32: // Stable channels will run with 10%
probability.
this is higher a % that we usually experiment on stable, 1 is much more common

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:38: // Experiment enabled until March 15,
2017. By default, disabled.
On 2016/11/09 10:56:39, noyau wrote:
> Before creating the new experiment should I check that if it has been
overriden
> by a server side config? Or should I plan for two experiment, this one
hardcoded
> and an emergency one to turn everything off from the server side?

I think it would make sense to check in case here the study has been setup - you
will get bad behavior if it already is. 
If it is not, then you can set it up manually as you are doing here

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:89: ShouldShowPopularSites()) {
On 2016/11/09 10:56:39, noyau wrote:
> I changed the order of the test, to only enroll in the experiment users who
are
> going to be shown popular site tiles. There is no point of counting existing
> users who already have a bunch of tiles.

makes sense, field trial group should only be triggered IFF it will be used.

However this is a behavior change so make sure any analysis doesn't cross
version border.

[noyau (Ping after 24h), 2016-11-14 17:08:08]

PTAL

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
File components/ntp_tiles/field_trial.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:31: void
ntp_tiles::SetUpFirstLaunchFieldTrial(bool is_stable_channel) {
On 2016/11/10 22:56:46, rkaplow wrote:
> can you comment this is done as a workaround since ios first launch
experiments
> are not available

Done.

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:32: // Stable channels will run with 10%
probability.
On 2016/11/10 22:56:46, rkaplow wrote:
> this is higher a % that we usually experiment on stable, 1 is much more common

Not on iOS, were volume are low compared to desktop and android. Plus this
particular experiement will only touch new users, not all users, as users who
have used Chrome before will already have the slots on the NTP filed with most
likely or most visited.

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:38: // Experiment enabled until March 15,
2017. By default, disabled.
On 2016/11/10 22:56:46, rkaplow wrote:
> On 2016/11/09 10:56:39, noyau wrote:
> > Before creating the new experiment should I check that if it has been
> overriden
> > by a server side config? Or should I plan for two experiment, this one
> hardcoded
> > and an emergency one to turn everything off from the server side?
> 
> I think it would make sense to check in case here the study has been setup -
you
> will get bad behavior if it already is. 
> If it is not, then you can set it up manually as you are doing here

Is Find the correct way to test for this without triggering being put in a
group? This is too early to know if the user meets the conditions of the
experiment, and if I call FindFullName(kPopularSiteFieldTrialName) it will leads
to incorrect numbers as everyone will be counted.

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:89: ShouldShowPopularSites()) {
On 2016/11/10 22:56:46, rkaplow wrote:
> On 2016/11/09 10:56:39, noyau wrote:
> > I changed the order of the test, to only enroll in the experiment users who
> are
> > going to be shown popular site tiles. There is no point of counting existing
> > users who already have a bunch of tiles.
> 
> makes sense, field trial group should only be triggered IFF it will be used.
> 
> However this is a behavior change so make sure any analysis doesn't cross
> version border.

Ok, I'll make sure everyone is aware.

[rkaplow, 2016-11-14 23:42:52]

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
File components/ntp_tiles/field_trial.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:32: // Stable channels will run with 10%
probability.
On 2016/11/14 17:08:08, noyau wrote:
> On 2016/11/10 22:56:46, rkaplow wrote:
> > this is higher a % that we usually experiment on stable, 1 is much more
common
> 
> Not on iOS, were volume are low compared to desktop and android. Plus this
> particular experiement will only touch new users, not all users, as users who
> have used Chrome before will already have the slots on the NTP filed with most
> likely or most visited.

Ok.. in either case this will need to get approval by L-R - and I would
highlight this is 10%.

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:38: // Experiment enabled until March 15,
2017. By default, disabled.
On 2016/11/14 17:08:08, noyau wrote:
> On 2016/11/10 22:56:46, rkaplow wrote:
> > On 2016/11/09 10:56:39, noyau wrote:
> > > Before creating the new experiment should I check that if it has been
> > overriden
> > > by a server side config? Or should I plan for two experiment, this one
> > hardcoded
> > > and an emergency one to turn everything off from the server side?
> > 
> > I think it would make sense to check in case here the study has been setup -
> you
> > will get bad behavior if it already is. 
> > If it is not, then you can set it up manually as you are doing here
> 
> Is Find the correct way to test for this without triggering being put in a
> group? This is too early to know if the user meets the conditions of the
> experiment, and if I call FindFullName(kPopularSiteFieldTrialName) it will
leads
> to incorrect numbers as everyone will be counted.  

I think TrialExists should work since we don't need to know the specifics of
what the group is.

[noyau (Ping after 24h), 2016-11-15 09:05:50]

PTAL.

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
File components/ntp_tiles/field_trial.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:32: // Stable channels will run with 10%
probability.
On 2016/11/14 23:42:51, rkaplow wrote:
> On 2016/11/14 17:08:08, noyau wrote:
> > On 2016/11/10 22:56:46, rkaplow wrote:
> > > this is higher a % that we usually experiment on stable, 1 is much more
> common
> > 
> > Not on iOS, were volume are low compared to desktop and android. Plus this
> > particular experiement will only touch new users, not all users, as users
who
> > have used Chrome before will already have the slots on the NTP filed with
most
> > likely or most visited.
> 
> Ok.. in either case this will need to get approval by L-R - and I would
> highlight this is 10%.

The highlight is already there: The first sentence of the launch bug mention the
10%. This value already has approval from Chrome Mobile leads, all the involved
PMs and representatives from Search. Trust me, this was not set on a whim :)

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
components/ntp_tiles/field_trial.cc:38: // Experiment enabled until March 15,
2017. By default, disabled.
On 2016/11/14 23:42:51, rkaplow wrote:
> On 2016/11/14 17:08:08, noyau wrote:
> > On 2016/11/10 22:56:46, rkaplow wrote:
> > > On 2016/11/09 10:56:39, noyau wrote:
> > > > Before creating the new experiment should I check that if it has been
> > > overriden
> > > > by a server side config? Or should I plan for two experiment, this one
> > > hardcoded
> > > > and an emergency one to turn everything off from the server side?
> > > 
> > > I think it would make sense to check in case here the study has been setup
-
> > you
> > > will get bad behavior if it already is. 
> > > If it is not, then you can set it up manually as you are doing here
> > 
> > Is Find the correct way to test for this without triggering being put in a
> > group? This is too early to know if the user meets the conditions of the
> > experiment, and if I call FindFullName(kPopularSiteFieldTrialName) it will
> leads
> > to incorrect numbers as everyone will be counted.  
> 
> I think TrialExists should work since we don't need to know the specifics of
> what the group is.

Done.

[noyau (Ping after 24h), 2016-11-15 22:03:43]

On 2016/11/15 09:05:50, noyau wrote:
> PTAL.
> 
>
https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
> File components/ntp_tiles/field_trial.cc (right):
> 
>
https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
> components/ntp_tiles/field_trial.cc:32: // Stable channels will run with 10%
> probability.
> On 2016/11/14 23:42:51, rkaplow wrote:
> > On 2016/11/14 17:08:08, noyau wrote:
> > > On 2016/11/10 22:56:46, rkaplow wrote:
> > > > this is higher a % that we usually experiment on stable, 1 is much more
> > common
> > > 
> > > Not on iOS, were volume are low compared to desktop and android. Plus this
> > > particular experiement will only touch new users, not all users, as users
> who
> > > have used Chrome before will already have the slots on the NTP filed with
> most
> > > likely or most visited.
> > 
> > Ok.. in either case this will need to get approval by L-R - and I would
> > highlight this is 10%.
> 
> The highlight is already there: The first sentence of the launch bug mention
the
> 10%. This value already has approval from Chrome Mobile leads, all the
involved
> PMs and representatives from Search. Trust me, this was not set on a whim :)
> 
>
https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/fi...
> components/ntp_tiles/field_trial.cc:38: // Experiment enabled until March 15,
> 2017. By default, disabled.
> On 2016/11/14 23:42:51, rkaplow wrote:
> > On 2016/11/14 17:08:08, noyau wrote:
> > > On 2016/11/10 22:56:46, rkaplow wrote:
> > > > On 2016/11/09 10:56:39, noyau wrote:
> > > > > Before creating the new experiment should I check that if it has been
> > > > overriden
> > > > > by a server side config? Or should I plan for two experiment, this one
> > > > hardcoded
> > > > > and an emergency one to turn everything off from the server side?
> > > > 
> > > > I think it would make sense to check in case here the study has been
setup
> -
> > > you
> > > > will get bad behavior if it already is. 
> > > > If it is not, then you can set it up manually as you are doing here
> > > 
> > > Is Find the correct way to test for this without triggering being put in a
> > > group? This is too early to know if the user meets the conditions of the
> > > experiment, and if I call FindFullName(kPopularSiteFieldTrialName) it will
> > leads
> > > to incorrect numbers as everyone will be counted.  
> > 
> > I think TrialExists should work since we don't need to know the specifics of
> > what the group is.
> 
> Done.

Ping. We are doing the dreaded 24h turnaround review cycle. Add a french
vacation in the middle and this CL has been in flight with a very simple change
for a week now. I'd like to land this before branch point, as the target for
this is M56, so a quick morning turnaround would be appreciated. Thanks.

[Alexei Svitkine (slow), 2016-11-16 00:52:54]

lgtm % comments

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
File components/ntp_tiles/field_trial.cc (right):

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
components/ntp_tiles/field_trial.cc:23: const char kPopularSiteDefaultGroup[] =
"Default";
Nit: Put in anon namespace.

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
components/ntp_tiles/field_trial.cc:36: void
ntp_tiles::SetUpFirstLaunchFieldTrial(bool is_stable_channel) {
Nit: Specifying namespace like this is not usually done in Chrome code - suggest
explicitly surrounding in a namespace block instead.

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
components/ntp_tiles/field_trial.cc:52: base::FieldTrial::ONE_TIME_RANDOMIZED,
NULL));
nullptr

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
components/ntp_tiles/field_trial.cc:66: trial->group();
Suggest leaving this out.

This way, the trial will only be marked active when the group is queried by code
that actually cares about the trial state. This should also make it consistent
with the TrialExists case.

[rkaplow, 2016-11-16 01:05:57]

lgtm

[sfiera, 2016-11-16 09:57:42]

LGTM

[noyau (Ping after 24h), 2016-11-16 12:27:45]

Patchset #6 (id:160001) has been deleted

[noyau (Ping after 24h), 2016-11-16 12:28:02]

Thanks for the review.

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
File components/ntp_tiles/field_trial.cc (right):

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
components/ntp_tiles/field_trial.cc:23: const char kPopularSiteDefaultGroup[] =
"Default";
On 2016/11/16 00:52:53, Alexei Svitkine (very slow) wrote:
> Nit: Put in anon namespace.

Done.

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
components/ntp_tiles/field_trial.cc:36: void
ntp_tiles::SetUpFirstLaunchFieldTrial(bool is_stable_channel) {
On 2016/11/16 00:52:53, Alexei Svitkine (very slow) wrote:
> Nit: Specifying namespace like this is not usually done in Chrome code -
suggest
> explicitly surrounding in a namespace block instead.

Done.

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
components/ntp_tiles/field_trial.cc:52: base::FieldTrial::ONE_TIME_RANDOMIZED,
NULL));
On 2016/11/16 00:52:53, Alexei Svitkine (very slow) wrote:
> nullptr

Done.

https://codereview.chromium.org/2484233002/diff/120001/components/ntp_tiles/f...
components/ntp_tiles/field_trial.cc:66: trial->group();
On 2016/11/16 00:52:53, Alexei Svitkine (very slow) wrote:
> Suggest leaving this out.
> 
> This way, the trial will only be marked active when the group is queried by
code
> that actually cares about the trial state. This should also make it consistent
> with the TrialExists case.

Done.

[noyau (Ping after 24h), 2016-11-16 12:28:57]

The CQ bit was checked by noyau@chromium.org

[noyau (Ping after 24h), 2016-11-16 12:28:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, asvitkine@chromium.org, sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2484233002/#ps140001
(title: "Feedback")

[commit-bot: I haz the power, 2016-11-16 12:29:14]

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

[Marc Treib, 2016-11-16 12:50:29]

treib@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-11-16 12:50:29]

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:89: ShouldShowPopularSites()) {
On 2016/11/14 17:08:08, noyau wrote:
> On 2016/11/10 22:56:46, rkaplow wrote:
> > On 2016/11/09 10:56:39, noyau wrote:
> > > I changed the order of the test, to only enroll in the experiment users
who
> > are
> > > going to be shown popular site tiles. There is no point of counting
existing
> > > users who already have a bunch of tiles.
> > 
> > makes sense, field trial group should only be triggered IFF it will be used.
> > 
> > However this is a behavior change so make sure any analysis doesn't cross
> > version border.
> 
> Ok, I'll make sure everyone is aware.

Drive-by: This is subtle and not obvious from reading the code. Could you add a
comment saying that the order matters? (Separate CL is fine)

[noyau (Ping after 24h), 2016-11-16 13:02:19]

The CQ bit was unchecked by noyau@chromium.org

[noyau (Ping after 24h), 2016-11-16 13:06:03]

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2484233002/diff/60001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:89: ShouldShowPopularSites()) {
On 2016/11/16 12:50:29, Marc Treib wrote:
> On 2016/11/14 17:08:08, noyau wrote:
> > On 2016/11/10 22:56:46, rkaplow wrote:
> > > On 2016/11/09 10:56:39, noyau wrote:
> > > > I changed the order of the test, to only enroll in the experiment users
> who
> > > are
> > > > going to be shown popular site tiles. There is no point of counting
> existing
> > > > users who already have a bunch of tiles.
> > > 
> > > makes sense, field trial group should only be triggered IFF it will be
used.
> > > 
> > > However this is a behavior change so make sure any analysis doesn't cross
> > > version border.
> > 
> > Ok, I'll make sure everyone is aware.
> 
> Drive-by: This is subtle and not obvious from reading the code. Could you add
a
> comment saying that the order matters? (Separate CL is fine)

Done.

[noyau (Ping after 24h), 2016-11-16 13:06:11]

The CQ bit was checked by noyau@chromium.org

[noyau (Ping after 24h), 2016-11-16 13:06:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, asvitkine@chromium.org, sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2484233002/#ps180001
(title: "Adding a comment.")

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

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-11-16 16:13:38]

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

[commit-bot: I haz the power, 2016-11-16 16:13:39]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[noyau (Ping after 24h), 2016-11-16 16:24:20]

Description was changed from

==========
Hard coded finch config for Popular site.

This is hard-coded in order to be available on first launch.

BUG=662350
==========

to

==========
Hard coded finch config for Popular site.

This is hard-coded in order to be available on first launch.

BUG=662350
R=asvitkine@chromium.org, rkaplow@chromium.org, sfiera@chromium.org
==========

[noyau (Ping after 24h), 2016-11-16 16:24:22]

Committed patchset #6 (id:180001) to pending queue manually as
21425884818f47299cdedd3cb1d36e4cd38fd94d (presubmit successful).

[commit-bot: I haz the power, 2016-11-16 16:27:31]

Description was changed from

==========
Hard coded finch config for Popular site.

This is hard-coded in order to be available on first launch.

BUG=662350
R=asvitkine@chromium.org, rkaplow@chromium.org, sfiera@chromium.org
==========

to

==========
Hard coded finch config for Popular site.

This is hard-coded in order to be available on first launch.

BUG=662350
R=asvitkine@chromium.org, rkaplow@chromium.org, sfiera@chromium.org

Committed: https://crrev.com/4793d9335b9ce611e2deb40863793fa785773200
Cr-Commit-Position: refs/heads/master@{#432511}
==========

[commit-bot: I haz the power, 2016-11-16 16:27:32]

Patchset 6 (id:??) landed as
https://crrev.com/4793d9335b9ce611e2deb40863793fa785773200
Cr-Commit-Position: refs/heads/master@{#432511}

[rohitrao (ping after 24h), 2016-11-18 18:59:15]

rohitrao@chromium.org changed reviewers:
+ rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-11-18 18:59:15]

FYI this is causing startup crashes on iOS when the popular sites experiment is
enabled.  We're investigating, but what does everyone think about reverting (or
turning the experiment off on iOS) while we figure this out?

[Alexei Svitkine (slow), 2016-11-18 19:01:40]

I think reverting sounds reasonable if this is causing crashes. Are there
crash/ links?

(Note: Since this is a hardcoded experiment config, it can't be turned off
on the server side - at least not for new installs.)

On Fri, Nov 18, 2016 at 1:59 PM, <rohitrao@chromium.org> wrote:

> FYI this is causing startup crashes on iOS when the popular sites
> experiment is
> enabled. We're investigating, but what does everyone think about reverting
> (or
> turning the experiment off on iOS) while we figure this out?
>
> https://codereview.chromium.org/2484233002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rohitrao (ping after 24h), 2016-11-18 20:03:33]

> I think reverting sounds reasonable if this is causing crashes. Are there
> crash/ links?
> 
> (Note: Since this is a hardcoded experiment config, it can't be turned off
> on the server side - at least not for new installs.)

No crash links to share yet.  The crash is on first run, but only when you sign
in.  We're catching it on the internal bots, but official canary builds have
failed to build (for unrelated reasons) since this CL landed.

I think we've identified the buggy code, so we should be able to fix without
turning the feature off.