Change FieldTrial::State to not use StringPiece members.

Change FieldTrial::State to not use StringPiece members.

BUG=668538
Review-Url: https://codereview.chromium.org/2633383002
Cr-Commit-Position: refs/heads/master@{#445775}
Committed: https://chromium.googlesource.com/chromium/src/+/b97d949a6bad8c3c14e874c2783585fef63e0c30

[Alexei Svitkine (slow), 2017-01-17 16:38:59]

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

[Alexei Svitkine (slow), 2017-01-17 16:39:00]

Bruce, PTAL.

This is in response to an observation you made last year about StringPiece ->
string conversions in field trial code.

Originally, I though this would help more, but turns out this only eliminates a
pair of as_string() calls in PickleFieldTrial() - but doesn't help with the
others.

The reason is that for the ParseFieldTrialsString(), the entries do not have
existing corresponding std::string entries - since they're parsed from a single
string. (Also, this codepath is now just a fallback codepath with shared-memory
field trials and so won't get hit in practice for many users.)

Then, there's another bunch of code unrelated to FieldTrial::State that reads
pickled strings and creates trials from them. Unfortunately, I don't see a good
way to eliminate the extra copies there because they're used in map lookups
where the keys are std::strings. (I don't *think* we have a way to do that with
StringPieces yet.)

[brucedawson, 2017-01-24 00:23:37]

Sorry for the delay. Looking now...

[brucedawson, 2017-01-24 00:44:14]

Thanks for doing this. It looks fairly straightforward with some modest but
clear gains.

lgtm with one suggestion.

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:254: : trial_name(nullptr), group_name(nullptr),
activated(false) {}
See comment in header file and consider removing the initialization list here.

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:672: output->append(*trial.trial_name);
This reads much cleaner, as a minor side effect.

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.h#...
base/metrics/field_trial.h:131: const std::string* trial_name;
Consider doing "= nullptr;" here to make it crystal clear that an uninitialized
pointer is impossible. At that point you might as well also initialize
'activated', and then the constructor in the .cc file will just be empty.

[Alexei Svitkine (slow), 2017-01-24 16:40:19]

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:254: : trial_name(nullptr), group_name(nullptr),
activated(false) {}
On 2017/01/24 00:44:14, brucedawson wrote:
> See comment in header file and consider removing the initialization list here.

Done.

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:672: output->append(*trial.trial_name);
On 2017/01/24 00:44:14, brucedawson wrote:
> This reads much cleaner, as a minor side effect.

Acknowledged.

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2633383002/diff/1/base/metrics/field_trial.h#...
base/metrics/field_trial.h:131: const std::string* trial_name;
On 2017/01/24 00:44:14, brucedawson wrote:
> Consider doing "= nullptr;" here to make it crystal clear that an
uninitialized
> pointer is impossible. At that point you might as well also initialize
> 'activated', and then the constructor in the .cc file will just be empty.

Done.

[Alexei Svitkine (slow), 2017-01-24 16:40:35]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-01-24 16:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/2633383002/#ps20001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-01-24 16:40:43]

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, 2017-01-24 16:44:21]

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

[commit-bot: I haz the power, 2017-01-24 16:44:21]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Alexei Svitkine (slow), 2017-01-24 17:41:43]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-01-24 17:41:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/2633383002/#ps40001
(title: "Fix initialization in new struct.")

[commit-bot: I haz the power, 2017-01-24 17:42:10]

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, 2017-01-24 19:15:51]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485279703144260,
"parent_rev": "89f261bcbcfa23d14ac1362a600784ce1d75f3e2", "commit_rev":
"b97d949a6bad8c3c14e874c2783585fef63e0c30"}

[commit-bot: I haz the power, 2017-01-24 19:16:28]

Description was changed from

==========
Change FieldTrial::State to not use StringPiece members.

BUG=668538
==========

to

==========
Change FieldTrial::State to not use StringPiece members.

BUG=668538

Review-Url: https://codereview.chromium.org/2633383002
Cr-Commit-Position: refs/heads/master@{#445775}
Committed:
https://chromium.googlesource.com/chromium/src/+/b97d949a6bad8c3c14e874c27835...
==========

[commit-bot: I haz the power, 2017-01-24 19:16:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b97d949a6bad8c3c14e874c27835...

[brucedawson, 2017-01-24 19:24:15]

lgtm