Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(55)

Issue 2449143002: Actually update FieldTrialEntry's activated field (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months ago by lawrencewu
Modified:
7 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Actually update FieldTrialEntry's activated field This changes moves the creation of the field trial entry in shared memory a earlier, in FinalizeGroupChoice(), and sets the "activated" field on the entry in NotifyFieldTrialGroupSelection(). We still create the allocator lazily. BUG=659212 Committed: https://crrev.com/986a6385750dae1120a89c49be4ddf3f6a707ce0 Cr-Commit-Position: refs/heads/master@{#427749}

Patch Set 1 #

Patch Set 2 : fix compile issue, clean up code #

Patch Set 3 : add to the allocator if it's null #

Total comments: 6

Patch Set 4 : address comments #

Total comments: 4

Patch Set 5 : fix locking problem #

Total comments: 4

Patch Set 6 : fix nits #

Total comments: 2

Patch Set 7 : ActivateFieldTrialEntry += WhileLocked #

Messages

Total messages: 28 (14 generated)
lawrencewu
Initial implementation for this feature.
7 months ago (2016-10-25 18:28:23 UTC) #7
Alexei Svitkine (slow)
https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_trial.cc#newcode348 base/metrics/field_trial.cc:348: FieldTrialList::GetFieldTrialAllocator(); I'm not a fan of having a public ...
7 months ago (2016-10-25 19:18:53 UTC) #9
lawrencewu
Address comments. https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_trial.cc#newcode348 base/metrics/field_trial.cc:348: FieldTrialList::GetFieldTrialAllocator(); On 2016/10/25 19:18:53, Alexei Svitkine (slow) ...
7 months ago (2016-10-25 20:27:33 UTC) #12
Alexei Svitkine (slow)
https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h#newcode530 base/metrics/field_trial.h:530: static void AddToAllocator(FieldTrial* field_trial); I don't think these should ...
7 months ago (2016-10-25 21:12:29 UTC) #13
lawrencewu
https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h#newcode530 base/metrics/field_trial.h:530: static void AddToAllocator(FieldTrial* field_trial); On 2016/10/25 21:12:29, Alexei Svitkine ...
7 months ago (2016-10-26 13:43:15 UTC) #14
Alexei Svitkine (slow)
https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h#newcode530 base/metrics/field_trial.h:530: static void AddToAllocator(FieldTrial* field_trial); On 2016/10/26 13:43:15, lawrencewu wrote: ...
7 months ago (2016-10-26 14:49:03 UTC) #15
lawrencewu
Address comments. https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h#newcode530 base/metrics/field_trial.h:530: static void AddToAllocator(FieldTrial* field_trial); On 2016/10/26 14:49:02, ...
7 months ago (2016-10-26 15:41:38 UTC) #18
Alexei Svitkine (slow)
https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_trial.cc#newcode811 base/metrics/field_trial.cc:811: void FieldTrialList::ActivateFieldTrialEntry(FieldTrial* field_trial) { Nit: Please order the functions ...
7 months ago (2016-10-26 16:21:08 UTC) #19
lawrencewu
Address nits. https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_trial.cc#newcode811 base/metrics/field_trial.cc:811: void FieldTrialList::ActivateFieldTrialEntry(FieldTrial* field_trial) { On 2016/10/26 16:21:08, ...
7 months ago (2016-10-26 16:35:24 UTC) #20
Alexei Svitkine (slow)
lgtm https://codereview.chromium.org/2449143002/diff/100001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/100001/base/metrics/field_trial.cc#newcode903 base/metrics/field_trial.cc:903: void FieldTrialList::ActivateFieldTrialEntry(FieldTrial* field_trial) { Nit: ActivateFieldTrialEntryWhileLocked()
7 months ago (2016-10-26 16:37:20 UTC) #21
lawrencewu
fix nit https://codereview.chromium.org/2449143002/diff/100001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/100001/base/metrics/field_trial.cc#newcode903 base/metrics/field_trial.cc:903: void FieldTrialList::ActivateFieldTrialEntry(FieldTrial* field_trial) { On 2016/10/26 16:37:20, ...
7 months ago (2016-10-26 16:52:19 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2449143002/120001
7 months ago (2016-10-26 16:52:43 UTC) #25
commit-bot: I haz the power
Committed patchset #7 (id:120001)
7 months ago (2016-10-26 18:19:43 UTC) #26
commit-bot: I haz the power
7 months ago (2016-10-26 18:22:05 UTC) #28
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/986a6385750dae1120a89c49be4ddf3f6a707ce0
Cr-Commit-Position: refs/heads/master@{#427749}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06