[UMA] Include field trial tuples in the UMA upload.

[UMA] Include field trial tuples in the UMA upload.

BUG=109817
TEST=unit_tests --gtest_filter=MetricsLogTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124655

[jar (doing other things), 2012-03-01 05:32:42]

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log.cc (right):

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:179: system_profile->add_field_trial();
personal style nit: This API seems strange to me.  In general, it is nicer to
form the object (in this case the SystemProfileProto::FieldTrial) and flesh it
out, before inserting it into the container.  In this way, the contain contains
only valid instances, and can even "check" on insertion for some elements of
correctness.

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:698: WriteFieldTrials(field_trial_ids,
system_profile);
This is tempting to make a method on system_profile, rather than a static
method, that takes a pointer.

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:815:
omnibox_event->set_time(MetricsLogBase::GetCurrentTime());
I think this is the same pattern that had me wondering earlier.  Is there a
reason to have mutable instances in the protobuf, rather than completing their
construction before inserting?

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/common/metrics/m...
File chrome/common/metrics/metrics_log_base.h (right):

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_base.h:153:
metrics::ChromeUserMetricsExtension* uma_proto() { return &uma_proto_; }
This is a scary interface. Why do other folks need to know where you keep your
slot?

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_base.h:154: const
metrics::ChromeUserMetricsExtension& uma_proto() const {
This seems a bit confusing to me, as you have overloaded based on the const-ness
of the instance, but then returned different  values.  I'd expect this will be
confusing (surprising?) when used, as the const-ness is often not locally
apparent at the call site.  i.e., sometimes you could say:
uma_proto().blah
other times:
uma_proto()->blah

[Ilya Sherman, 2012-03-01 06:41:50]

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log.cc (right):

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:179: system_profile->add_field_trial();
On 2012/03/01 05:32:42, jar wrote:
> personal style nit: This API seems strange to me.  In general, it is nicer to
> form the object (in this case the SystemProfileProto::FieldTrial) and flesh it
> out, before inserting it into the container.  In this way, the contain
contains
> only valid instances, and can even "check" on insertion for some elements of
> correctness.  

Alas, this is just how the C++ protocol buffer API works.

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:698: WriteFieldTrials(field_trial_ids,
system_profile);
On 2012/03/01 05:32:42, jar wrote:
> This is tempting to make a method on system_profile, rather than a static
> method, that takes a pointer.

Again, system_profile is a protocol buffer, so I have no control over the
methods defined on it.  I could add a wrapper class, but that just doesn't seem
worth it.

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/common/metrics/m...
File chrome/common/metrics/metrics_log_base.h (right):

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_base.h:153:
metrics::ChromeUserMetricsExtension* uma_proto() { return &uma_proto_; }
On 2012/03/01 05:32:42, jar wrote:
> This is a scary interface. Why do other folks need to know where you keep your
> slot?

This method has protected visibility -- only MetricsLog gets to peek into this
implementation detail.  (Well, ok, testing classes theoretically could peek in
as well, but I don't see a clean way to avoid that.)

https://chromiumcodereview.appspot.com/9559017/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_base.h:154: const
metrics::ChromeUserMetricsExtension& uma_proto() const {
On 2012/03/01 05:32:42, jar wrote:
> This seems a bit confusing to me, as you have overloaded based on the
const-ness
> of the instance, but then returned different  values.  I'd expect this will be
> confusing (surprising?) when used, as the const-ness is often not locally
> apparent at the call site.  i.e., sometimes you could say:
> uma_proto().blah
> other times:
> uma_proto()->blah

Fair enough, updated this to be a pointer as well.

[jar (doing other things), 2012-03-01 07:07:16]

<sigh>.. sorry to hear this is the interface you were given.

Oh well.

LGTM

[SteveT, 2012-03-01 14:59:53]

Just a couple nits. Nicely done! C++ protobuf APIs are weird.

LGTM.

https://chromiumcodereview.appspot.com/9559017/diff/6001/chrome/browser/metri...
File chrome/browser/metrics/metrics_log.cc (right):

https://chromiumcodereview.appspot.com/9559017/diff/6001/chrome/browser/metri...
chrome/browser/metrics/metrics_log.cc:174: const
std::vector<base::FieldTrial::NameGroupId>& field_trial_ids,
Nit: Could typedef std::vector<base::FieldTrial::NameGroupId> into something
more readable.

https://chromiumcodereview.appspot.com/9559017/diff/6001/chrome/browser/metri...
chrome/browser/metrics/metrics_log.cc:272: 
Nit: Too many breaks between methods?

[Ilya Sherman, 2012-03-02 01:40:35]

https://chromiumcodereview.appspot.com/9559017/diff/6001/chrome/browser/metri...
File chrome/browser/metrics/metrics_log.cc (right):

https://chromiumcodereview.appspot.com/9559017/diff/6001/chrome/browser/metri...
chrome/browser/metrics/metrics_log.cc:174: const
std::vector<base::FieldTrial::NameGroupId>& field_trial_ids,
On 2012/03/01 14:59:53, SteveT wrote:
> Nit: Could typedef std::vector<base::FieldTrial::NameGroupId> into something
> more readable.

Done.

https://chromiumcodereview.appspot.com/9559017/diff/6001/chrome/browser/metri...
chrome/browser/metrics/metrics_log.cc:272: 
On 2012/03/01 14:59:53, SteveT wrote:
> Nit: Too many breaks between methods?

Done.

[commit-bot: I haz the power, 2012-03-02 02:49:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/9559017/9006

[Ilya Sherman, 2012-03-02 02:53:22]

(Note: I mocked out a bit more code for testing after y'allses LGTMs, but I'm
assuming that code change is non-controversial.)

[commit-bot: I haz the power, 2012-03-02 08:21:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/9559017/9006

[commit-bot: I haz the power, 2012-03-02 16:04:35]

Change committed as 124655