Fix getting initially active trials with shared memory.

Fix getting initially active trials with shared memory.

With the shared memory implementation, the logic for
getting initially active trials was no longer valid. This
change fixes that by getting the active trials from the
shared memory allocator if available.

BUG=666497
Committed: https://crrev.com/0cb7b5282e216024f887c9addc24b8fc5731f6f5
Cr-Commit-Position: refs/heads/master@{#433969}

[Alexei Svitkine (slow), 2016-11-17 22:33:47]

asvitkine@chromium.org changed reviewers:
+ bcwhite@chromium.org, lawrencewu@chromium.org

[Alexei Svitkine (slow), 2016-11-17 22:33:48]

Noticed this during my flight while working on something else. Thought I'd just
fix it.

(Note: I don't think it's related to the crash Lawrence is investigating.)

[Alexei Svitkine (slow), 2016-11-17 22:34:08]

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

[Alexei Svitkine (slow), 2016-11-17 22:34:17]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-17 22:36:39]

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, 2016-11-17 23:57:48]

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

[commit-bot: I haz the power, 2016-11-17 23:57:49]

Dry run: 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_...)

[lawrencewu, 2016-11-18 02:45:59]

On 2016/11/17 22:33:48, Alexei Svitkine (very slow) wrote:
> Noticed this during my flight while working on something else. Thought I'd
just
> fix it.
> 
> (Note: I don't think it's related to the crash Lawrence is investigating.)

Good catch, thanks! lgtm

[lawrencewu, 2016-11-18 16:33:11]

https://codereview.chromium.org/2504163005/diff/20001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2504163005/diff/20001/base/metrics/field_tria...
base/metrics/field_trial.cc:631:
DCHECK(global_->create_trials_from_command_line_called_);
This function is called or can be called before CreateTrialsFromCommandLine().
Does this have to be called after it? Getting the active groups and creating the
trials seems independent from each other.

[Alexei Svitkine (slow), 2016-11-18 16:35:47]

https://codereview.chromium.org/2504163005/diff/20001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2504163005/diff/20001/base/metrics/field_tria...
base/metrics/field_trial.cc:631:
DCHECK(global_->create_trials_from_command_line_called_);
On 2016/11/18 16:33:10, lawrencewu wrote:
> This function is called or can be called before CreateTrialsFromCommandLine().
> Does this have to be called after it? Getting the active groups and creating
the
> trials seems independent from each other.

If it's called before it, then the field_trial_allocator_ won't be created yet.
So it needs to be called after.

I will take a look at why the order isn't what I expected.

[bcwhite, 2016-11-21 15:56:22]

lgtm

https://codereview.chromium.org/2504163005/diff/20001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2504163005/diff/20001/base/metrics/field_tria...
base/metrics/field_trial.cc:656: active_groups->push_back(group);
push_back(std::move(group))

[Alexei Svitkine (slow), 2016-11-21 19:24:04]

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

[commit-bot: I haz the power, 2016-11-21 19:24:39]

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

[Alexei Svitkine (slow), 2016-11-21 20:44:53]

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

[commit-bot: I haz the power, 2016-11-21 20:45:37]

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

[Alexei Svitkine (slow), 2016-11-21 20:47:10]

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

[Alexei Svitkine (slow), 2016-11-21 20:47:11]

Thanks. Comments addressed and fixed the tests.

Some tests were not running FeatureList::CreateFromCommandLine():
  - Tests that were based on content/public/test/render_view_test.cc - which
I've now added code to, to initialize FieldTrialList.
  - Tests that were using --single-process - which is addressed by not running
this code in this case (since this code exists to sync state between processes
and is not needed in that mode).

+jochen for content/ OWNERS

https://codereview.chromium.org/2504163005/diff/20001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2504163005/diff/20001/base/metrics/field_tria...
base/metrics/field_trial.cc:656: active_groups->push_back(group);
On 2016/11/21 15:56:22, bcwhite wrote:
> push_back(std::move(group))

Done.

[Alexei Svitkine (slow), 2016-11-21 20:49:30]

(Er I meant FieldTrialList::CreateTrialsFromCommandLine() above.)

On Mon, Nov 21, 2016 at 3:47 PM, <asvitkine@chromium.org> wrote:

> Thanks. Comments addressed and fixed the tests.
>
> Some tests were not running FeatureList::CreateFromCommandLine():
> - Tests that were based on content/public/test/render_view_test.cc - which
> I've now added code to, to initialize FieldTrialList.
> - Tests that were using --single-process - which is addressed by not
> running
> this code in this case (since this code exists to sync state between
> processes
> and is not needed in that mode).
>
> +jochen for content/ OWNERS
>
>
> https://codereview.chromium.org/2504163005/diff/20001/
> base/metrics/field_trial.cc
> File base/metrics/field_trial.cc (right):
>
> https://codereview.chromium.org/2504163005/diff/20001/
> base/metrics/field_trial.cc#newcode656
> base/metrics/field_trial.cc:656: active_groups->push_back(group);
> On 2016/11/21 15:56:22, bcwhite wrote:
> > push_back(std::move(group))
>
> Done.
>
> https://codereview.chromium.org/2504163005/
>

-- 
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.

[lawrencewu, 2016-11-21 20:53:18]

lgtm % nits

https://codereview.chromium.org/2504163005/diff/100001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2504163005/diff/100001/base/metrics/field_tri...
base/metrics/field_trial.cc:640: SharedPersistentMemoryAllocator* allocator =
nit: use the FieldTrialAllocator typedef (and below)

https://codereview.chromium.org/2504163005/diff/100001/base/metrics/field_tri...
base/metrics/field_trial.cc:643: SharedPersistentMemoryAllocator::Reference ref;
FieldTrial::FieldTrialRef ref;

[Alexei Svitkine (slow), 2016-11-21 21:11:19]

https://codereview.chromium.org/2504163005/diff/100001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2504163005/diff/100001/base/metrics/field_tri...
base/metrics/field_trial.cc:640: SharedPersistentMemoryAllocator* allocator =
On 2016/11/21 20:53:18, lawrencewu wrote:
> nit: use the FieldTrialAllocator typedef (and below)

Done.

https://codereview.chromium.org/2504163005/diff/100001/base/metrics/field_tri...
base/metrics/field_trial.cc:643: SharedPersistentMemoryAllocator::Reference ref;
On 2016/11/21 20:53:18, lawrencewu wrote:
> FieldTrial::FieldTrialRef ref;

Done.

[Alexei Svitkine (slow), 2016-11-22 15:30:21]

jochen: friendly ping

[Alexei Svitkine (slow), 2016-11-22 16:25:23]

asvitkine@chromium.org changed reviewers:
+ piman@chromium.org
- jochen@chromium.org

[Alexei Svitkine (slow), 2016-11-22 16:25:23]

asvitkine@chromium.org changed reviewers:
+ piman@chromium.org
- jochen@chromium.org

[Alexei Svitkine (slow), 2016-11-22 16:25:23]

-jochen +piman for content/ OWNERS

(Hoping to land this today and I think it's getting outside of working hours for
jochen's timezone.)

[Alexei Svitkine (slow), 2016-11-22 16:25:24]

-jochen +piman for content/ OWNERS

(Hoping to land this today and I think it's getting outside of working hours for
jochen's timezone.)

[piman, 2016-11-22 18:51:13]

lgtm

[Alexei Svitkine (slow), 2016-11-22 18:51:24]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-11-22 18:51:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
bcwhite@chromium.org, lawrencewu@chromium.org
Link to the patchset: https://codereview.chromium.org/2504163005/#ps120001
(title: "Use typedefs")

[commit-bot: I haz the power, 2016-11-22 18:51:44]

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-22 19:24:05]

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

[commit-bot: I haz the power, 2016-11-22 19:24:06]

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

[Alexei Svitkine (slow), 2016-11-22 20:05:30]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2016-11-22 20:06:18]

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-22 20:40:08]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479845130164770,
"parent_rev": "ddc934cc025893daa1f18e5602b5ec9a0cb6de7b", "commit_rev":
"2682a5de8f10618418e80b6f9dbfff6759418030"}

[commit-bot: I haz the power, 2016-11-22 20:40:34]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-11-22 20:44:47]

Description was changed from

==========
Fix getting initially active trials with shared memory.

With the shared memory implementation, the logic for
getting initially active trials was no longer valid. This
change fixes that by getting the active trials from the
shared memory allocator if available.

BUG=666497
==========

to

==========
Fix getting initially active trials with shared memory.

With the shared memory implementation, the logic for
getting initially active trials was no longer valid. This
change fixes that by getting the active trials from the
shared memory allocator if available.

BUG=666497

Committed: https://crrev.com/0cb7b5282e216024f887c9addc24b8fc5731f6f5
Cr-Commit-Position: refs/heads/master@{#433969}
==========

[commit-bot: I haz the power, 2016-11-22 20:44:48]

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