Chromium Code Reviews
Help | Chromium Project | Sign in
(3)

Issue 2449783007: Use pickle for field trial entries in shared memory (Closed)

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

Description

Use pickle for field trial entries in shared memory This CL changes the data format we use to store field trials in shared memory from custom offsets to a pickled structure, which has several advantages. Namely, it handles offsets for us and requires less code. We will also probably end up using this for experiment parameters as well. BUG=660128 Committed: https://crrev.com/ab1da09492fa8e84ae6d90151bd718efdf97d3f4 Cr-Commit-Position: refs/heads/master@{#428423}

Patch Set 1 #

Patch Set 2 : turn flag back to false #

Total comments: 13

Patch Set 3 : address comments #

Patch Set 4 : address comments #

Total comments: 9

Patch Set 5 : address comments #

Total comments: 2

Patch Set 6 : address nits #

Messages

Total messages: 19 (6 generated)
lawrencewu
I'll add parameter support in the next CL, but this should be a decent start. ...
3 months, 3 weeks ago (2016-10-27 20:02:38 UTC) #4
Alexei Svitkine (slow)
https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc#newcode805 base/metrics/field_trial.cc:805: CreateFieldTrial(trial_name.as_string(), group_name.as_string()); Can you add a TODO above the ...
3 months, 3 weeks ago (2016-10-27 21:05:02 UTC) #5
lawrencewu
Address comments. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc#newcode805 base/metrics/field_trial.cc:805: CreateFieldTrial(trial_name.as_string(), group_name.as_string()); On 2016/10/27 21:05:02, Alexei Svitkine ...
3 months, 3 weeks ago (2016-10-27 21:24:19 UTC) #6
Alexei Svitkine (slow)
https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc#newcode801 base/metrics/field_trial.cc:801: continue; I'm not a fan of this error handling. ...
3 months, 3 weeks ago (2016-10-27 21:36:50 UTC) #7
lawrencewu
Address comments. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc#newcode801 base/metrics/field_trial.cc:801: continue; On 2016/10/27 21:36:50, Alexei Svitkine (slow) ...
3 months, 3 weeks ago (2016-10-28 14:23:04 UTC) #8
Alexei Svitkine (slow)
https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc#newcode869 base/metrics/field_trial.cc:869: size_t total_size = sizeof(bool) + +sizeof(uint32_t) + pickle.size(); On ...
3 months, 3 weeks ago (2016-10-28 15:25:29 UTC) #9
lawrencewu
On 2016/10/28 15:25:29, Alexei Svitkine (slow) wrote: > https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_trial.cc > File base/metrics/field_trial.cc (right): > > ...
3 months, 3 weeks ago (2016-10-28 15:28:22 UTC) #10
Alexei Svitkine (slow)
https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_trial.cc#newcode58 base/metrics/field_trial.cc:58: bool GetTrialAndGroupName(StringPiece* trial_name, Please add a comment, specifically mentioning ...
3 months, 3 weeks ago (2016-10-28 15:32:23 UTC) #11
lawrencewu
Address comments. https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_trial.cc#newcode63 base/metrics/field_trial.cc:63: Pickle pickle(src, this->size); On 2016/10/28 15:32:22, Alexei ...
3 months, 3 weeks ago (2016-10-28 15:38:02 UTC) #12
Alexei Svitkine (slow)
lgtm https://codereview.chromium.org/2449783007/diff/80001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/80001/base/metrics/field_trial.cc#newcode52 base/metrics/field_trial.cc:52: // base::Pickle object that we unpickle and read ...
3 months, 3 weeks ago (2016-10-28 15:41:32 UTC) #13
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/2449783007/100001
3 months, 3 weeks ago (2016-10-28 16:59:19 UTC) #16
commit-bot: I haz the power
Committed patchset #6 (id:100001)
3 months, 3 weeks ago (2016-10-28 18:06:21 UTC) #17
commit-bot: I haz the power
3 months, 3 weeks ago (2016-10-28 18:22:28 UTC) #19
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/ab1da09492fa8e84ae6d90151bd718efdf97d3f4
Cr-Commit-Position: refs/heads/master@{#428423}
Sign in to reply to this message.

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