Retrieve client_id from GoogleUpdateSettings when its missing from Local State.

Retrieve client_id from GoogleUpdateSettings when its missing from Local State.

Precursor refactoring CL @ https://codereview.chromium.org/365133005/

Precursor kInstallDate move to metrics_pref_names @ https://codereview.chromium.org/370813003/

This is the Windows implementation, POSIX implementation to follow in https://codereview.chromium.org/377713002/ (easier to develop and test on Linux in a follow-up CL)

BUG=391338
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284805

[gab, 2014-07-07 22:26:16]

Ilya please take a look.

I had talked about this feature with Alexei, but it looks like he's on vacation
now.

The idea is to try and recover the client_id backed up in the registry when we
somehow lose Local State (happens surprisingly often on tampered machines, etc.
-- estimated to ~1% of the population daily) to avoid having this user look like
a brand new client again and to be able to see patterns better for such users.

Alexei asked me to also backup and recover the installation_date as it would be
weird to see a report with an old client_id yet a new installation_date (as it
gets regenerated when wiped).
Do you feel the same logic applies to kMetricsReportingEnabledTimestamp (for now
I just reset it to Time::Now()...)?

I split off two other CLs as precursor CLs
(https://codereview.chromium.org/370813003/ and
https://codereview.chromium.org/365133005/) please take a look at those as well;
I didn't send them separately as I think they make more sense in the global
context of this core CL.

I split another one as a follow-up CL for the POSIX implementation of this
(which is just a draft written on a Windows machine which I'll finish and test
on Linux once this core CL has landed):
https://codereview.chromium.org/377713002/

Thanks!
Gab

[Ilya Sherman, 2014-07-08 01:10:22]

+Vadim

Thanks, Gab!

On 2014/07/07 22:26:16, gab wrote:
> Alexei asked me to also backup and recover the installation_date as it would
be
> weird to see a report with an old client_id yet a new installation_date (as it
> gets regenerated when wiped).
> Do you feel the same logic applies to kMetricsReportingEnabledTimestamp (for
now
> I just reset it to Time::Now()...)?

Vadim, do you know whether uma_enabled_date is still used?  I see that it's
still referenced by some scripts, but I don't know whether those scripts are the
ones we run for the production pipelines.

https://codereview.chromium.org/372473004/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/372473004/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:38: // saved, migrate back to
a proper GUID if this is the case. This migration
nit: "saved, migrate" -> "saved. Migrate"

https://codereview.chromium.org/372473004/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:51:
client_id_with_dashes.push_back(*(client_id_it++));
nit: I'd prefer that you separate out reading from the iterator and incrementing
it onto two separate lines.

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:304: if (metrics_id) {
Does it ever make sense to call this method with a NULL metrics_id?

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:312: base::string16
installation_date_s;
Optional nit: I'd prefer "str" to just "s".

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.h:98: const int64
installation_date);
nit: No need for the const in front of an int parameter.

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.cc:106: if (installation_date_backup !=
0) {
Given lines 95-96, I don't think this check does anything.

https://codereview.chromium.org/372473004/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/372473004/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:31648: +    client id.
I think UMA_HISTOGRAM_CUSTOM_TIMES still records times in milliseconds.

[vadimb1, 2014-07-08 01:26:07]

On 2014/07/08 01:10:22, Ilya Sherman wrote:
> On 2014/07/07 22:26:16, gab wrote:
> > Alexei asked me to also backup and recover the installation_date as it would
> be
> > weird to see a report with an old client_id yet a new installation_date (as
it
> > gets regenerated when wiped).
> > Do you feel the same logic applies to kMetricsReportingEnabledTimestamp (for
> now
> > I just reset it to Time::Now()...)?
> 
> Vadim, do you know whether uma_enabled_date is still used?  I see that it's
> still referenced by some scripts, but I don't know whether those scripts are
the
> ones we run for the production pipelines.

Yes, we still use SystemProfileProto.uma_enabled_date to calculate the "user
age" (proxy from the time passed from the client installation).
Hint: search not only for uma_enabled_date but also for UmaEnabledDate. :)

[Ilya Sherman, 2014-07-08 03:08:58]

On 2014/07/08 01:26:07, vadimb1 wrote:
> On 2014/07/08 01:10:22, Ilya Sherman wrote:
> > On 2014/07/07 22:26:16, gab wrote:
> > > Alexei asked me to also backup and recover the installation_date as it
would
> > be
> > > weird to see a report with an old client_id yet a new installation_date
(as
> it
> > > gets regenerated when wiped).
> > > Do you feel the same logic applies to kMetricsReportingEnabledTimestamp
(for
> > now
> > > I just reset it to Time::Now()...)?
> > 
> > Vadim, do you know whether uma_enabled_date is still used?  I see that it's
> > still referenced by some scripts, but I don't know whether those scripts are
> the
> > ones we run for the production pipelines.
> 
> Yes, we still use SystemProfileProto.uma_enabled_date to calculate the "user
> age" (proxy from the time passed from the client installation).
> Hint: search not only for uma_enabled_date but also for UmaEnabledDate. :)

Ok.  In that case, it sounds like we should back up and restore the
kMetricsReportingEnabledTimestamp as well.  Thanks, Vadim.

[grt (UTC plus 2), 2014-07-08 14:48:02]

a few drive-by comments

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
File components/metrics/metrics_state_manager.h (right):

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:29: typedef base::Callback<bool(const
std::string& client_id,
why not void? the return value is ignored in the impl.

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:31: // A callback that can be invoked
to retrieve client info saved through the
newline above
how about Load/Store rather than Retrieve/Save?

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:33: typedef
base::Callback<bool(std::string* client_id, int64* installation_date)>
consider making this a void function too, and stipulating that implementations
clear client_id on error.

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:96: // |save_client_info| should back
up client info to solid state such that it
solid state -> persistent storage (not everyone rocks an SSD)

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:129: const SaveClientInfoCallback
save_client_info_;
add doc comments

[gab, 2014-07-09 19:17:00]

Feedback applied.

Also, in order to avoid adding yet another out-param for the
metrics_enabled_date pref we also want to backup/restore, I defined a ClientInfo
struct to pass this data around in one block.

I thought this was trivial at first, but it turns out that the struct can't be
defined in chrome/ as it wouldn't be usable from components/metrics, but putting
it in components/metrics was also troublesome as having chrome/installer/util
depend on components/metrics also caused problems...

So I introduced components/metrics/common, a new mini-component designed for
holding common interfaces shared with external components... despite this being
somewhat complicated, I still prefer this to having 3 out-params (which means I
also have to define what it means for them to be NULL, etc.).

PTAL!

Thanks!
Gab

https://codereview.chromium.org/372473004/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/372473004/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:38: // saved, migrate back to
a proper GUID if this is the case. This migration
On 2014/07/08 01:10:21, Ilya Sherman wrote:
> nit: "saved, migrate" -> "saved. Migrate"

Done.

https://codereview.chromium.org/372473004/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:51:
client_id_with_dashes.push_back(*(client_id_it++));
On 2014/07/08 01:10:21, Ilya Sherman wrote:
> nit: I'd prefer that you separate out reading from the iterator and
incrementing
> it onto two separate lines.

Done.

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:304: if (metrics_id) {
On 2014/07/08 01:10:21, Ilya Sherman wrote:
> Does it ever make sense to call this method with a NULL metrics_id?

Not really, changed the model anyways as we want to support another backup entry
for kMetricsReportingEnabledTimestamp.

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:312: base::string16
installation_date_s;
On 2014/07/08 01:10:21, Ilya Sherman wrote:
> Optional nit: I'd prefer "str" to just "s".

Done.

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/372473004/diff/80001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.h:98: const int64
installation_date);
On 2014/07/08 01:10:22, Ilya Sherman wrote:
> nit: No need for the const in front of an int parameter.

Indeed, not sure how this ended up there, it's not in the impl at least :-)!

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
File components/metrics/metrics_state_manager.h (right):

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:29: typedef base::Callback<bool(const
std::string& client_id,
On 2014/07/08 14:48:01, grt wrote:
> why not void? the return value is ignored in the impl.

Done.

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:31: // A callback that can be invoked
to retrieve client info saved through the
On 2014/07/08 14:48:01, grt wrote:
> newline above
> how about Load/Store rather than Retrieve/Save?

I like it, done in precursor rename CL.

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:33: typedef
base::Callback<bool(std::string* client_id, int64* installation_date)>
On 2014/07/08 14:48:01, grt wrote:
> consider making this a void function too, and stipulating that implementations
> clear client_id on error.

Actually changed the API altogether here, introducing a struct to contain the 3
bits we now want to backup.

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:96: // |save_client_info| should back
up client info to solid state such that it
On 2014/07/08 14:48:01, grt wrote:
> solid state -> persistent storage (not everyone rocks an SSD)

Done.

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:129: const SaveClientInfoCallback
save_client_info_;
On 2014/07/08 14:48:01, grt wrote:
> add doc comments

I find that in the case of typedef'ed callbacks, a comment here would either be
very similar to the comment on the ifdef type; or it would be useless (e.g., "An
instance of a SaveClientInfoCallback"...).

I put no comments here as I don't think it helps readability (it in fact hinders
it with unnecessary text IMO).

[Ilya Sherman, 2014-07-10 00:48:34]

On 2014/07/09 19:17:00, gab wrote:
> Also, in order to avoid adding yet another out-param for the
> metrics_enabled_date pref we also want to backup/restore, I defined a
ClientInfo
> struct to pass this data around in one block.
> 
> I thought this was trivial at first, but it turns out that the struct can't be
> defined in chrome/ as it wouldn't be usable from components/metrics, but
putting
> it in components/metrics was also troublesome as having chrome/installer/util
> depend on components/metrics also caused problems...

Hmm, what problems did you encounter with having chrome/installer/util depend on
components/metrics?  I'd prefer not to create a mini component to house a single
struct if we can avoid it...

[grt (UTC plus 2), 2014-07-10 14:03:33]

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
File components/metrics/metrics_state_manager.h (right):

https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
components/metrics/metrics_state_manager.h:129: const SaveClientInfoCallback
save_client_info_;
On 2014/07/09 19:16:59, gab wrote:
> On 2014/07/08 14:48:01, grt wrote:
> > add doc comments
> 
> I find that in the case of typedef'ed callbacks, a comment here would either
be
> very similar to the comment on the ifdef type; or it would be useless (e.g.,
"An
> instance of a SaveClientInfoCallback"...).
> 
> I put no comments here as I don't think it helps readability (it in fact
hinders
> it with unnecessary text IMO).

sorry i left out the justification for the request. i mentioned it for two
reasons (which i think still apply): this surrounding code documents most
members of the class, even ones that one could argue are obvious. chromium style
says "Try to follow any per-file conventions you notice when modifying existing
code." so that's the first reason. the second is that i've recently noticed that
the main style guide has this to say about member variables: "Each class data
member (also called an instance variable or member variable) should have a
comment describing what it is used for." i've recently been putting
implementation details in the doc comments for member variables. for these two,
i might say something like:

  // A callback run during Client ID creation so the embedder can store the new
ID.

or something along those lines aimed at readers trying to understand how
MetricsStateManager makes use of this member. the doc comment for the type
itself is aimed at the consumer of MetricsStateManager, so there isn't
necessarily duplication between the two.

consider introducing a Delegate interface now that there are three callbacks
rather than just one. there have been discussions on chromium-dev suggesting to
use a callback in place of a single-method delegate interface, which leads me to
believe that the groupthink suggests that a delegate interface is preferred over
multiple callbacks. this would avoid adding two new members that i'm asking you
to document, leaving only one! :-)

[gab, 2014-07-10 21:45:13]

Managed to get rid of metrics_common; putting ClientInfo directly inside
components/metrics.

I'm not actually sure why that didn't work when I first tried what I remember as
being exactly this... I guess I'd somehow messed up my gyp change locally and
led myself to believe this wouldn't work..!

Thanks for pushing back and making me realize this could just work without an
extra component :-)!

FYI, I'll augment MetricsStateManager to test the new code shortly, but PTAL at
the latest design in the meantime.


Reply to grt inline below.


FYI, diffing vs patch set 9 is probably what makes the most sense to skip the
noise from metrics_common.


Thanks!
Gab


On 2014/07/10 14:03:33, grt wrote:
>
https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
> File components/metrics/metrics_state_manager.h (right):
> 
>
https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...
> components/metrics/metrics_state_manager.h:129: const SaveClientInfoCallback
> save_client_info_;
> On 2014/07/09 19:16:59, gab wrote:
> > On 2014/07/08 14:48:01, grt wrote:
> > > add doc comments
> > 
> > I find that in the case of typedef'ed callbacks, a comment here would either
> be
> > very similar to the comment on the ifdef type; or it would be useless (e.g.,
> "An
> > instance of a SaveClientInfoCallback"...).
> > 
> > I put no comments here as I don't think it helps readability (it in fact
> hinders
> > it with unnecessary text IMO).
> 
> sorry i left out the justification for the request. i mentioned it for two
> reasons (which i think still apply): this surrounding code documents most
> members of the class, even ones that one could argue are obvious. chromium
style
> says "Try to follow any per-file conventions you notice when modifying
existing
> code." so that's the first reason. 

True, but in this case the only existing callback didn't have an accompanying
comment (I added one now!).

> the second is that i've recently noticed that
> the main style guide has this to say about member variables: "Each class data
> member (also called an instance variable or member variable) should have a
> comment describing what it is used for." i've recently been putting
> implementation details in the doc comments for member variables. for these
two,
> i might say something like:
> 
>   // A callback run during Client ID creation so the embedder can store the
new
> ID.
> 
> or something along those lines aimed at readers trying to understand how
> MetricsStateManager makes use of this member. the doc comment for the type
> itself is aimed at the consumer of MetricsStateManager, so there isn't
> necessarily duplication between the two.
> 

This argument makes sense to me. I'd always thought of the header as documenting
the API, but I like adding comments on the members to help people reading the
impl for the first time get around.

Done :)!


> consider introducing a Delegate interface now that there are three callbacks
> rather than just one. there have been discussions on chromium-dev suggesting
to
> use a callback in place of a single-method delegate interface, which leads me
to
> believe that the groupthink suggests that a delegate interface is preferred
over
> multiple callbacks. this would avoid adding two new members that i'm asking
you
> to document, leaving only one! :-)

Actually, MetricsServiceClient (and its impl ChromeMetricsServiceClient) is this
delegate.

I was originally going to augment it to provide what I need, but it turns out
that ChromeMetricsServiceClient already holds a pointer to the
MetricsStateManager and thus passing a pointer to the MetricsServiceClient into
the MetricsStateManager creates a circular dependency :-(.

Therefore I went with this callback approach. I guess we could add a second
delegate, but I'm not convinced it's worth it (the individual callbacks allow
more precise comments on the member variables after all ;-)!).

[Ilya Sherman, 2014-07-11 04:45:41]

Thanks!

https://codereview.chromium.org/372473004/diff/280001/chrome/browser/google/g...
File chrome/browser/google/google_update_settings_posix.cc (right):

https://codereview.chromium.org/372473004/diff/280001/chrome/browser/google/g...
chrome/browser/google/google_update_settings_posix.cc:70: // TODO(gab):
Implement |installation_date| storing/loading on POSIX.
nit: Please update this comment.

https://codereview.chromium.org/372473004/diff/280001/chrome/browser/google/g...
chrome/browser/google/google_update_settings_posix.cc:83: // TODO(gab):
Implement |installation_date| storing/loading on POSIX.
nit: Please update this comment.

https://codereview.chromium.org/372473004/diff/280001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/372473004/diff/280001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:300:
scoped_ptr<metrics::ClientInfo> client_info(new metrics::ClientInfo);
nit: Can this be moved down to just above line 308?

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
File components/metrics/client_info.h (right):

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
components/metrics/client_info.h:18: public:
nit: Structs are public by default.

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
components/metrics/client_info.h:22: std::string client_id;
nit: Please leave a blank line between adjacent documented fields.

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
components/metrics/client_info.h:25: // The date on which reporting was enabled:
represented as an epoch time in
nit: "on which reporting was enabled" -> "on which metrics reporting was
enabled"

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
components/metrics/client_info.h:30: DISALLOW_COPY_AND_ASSIGN(ClientInfo);
What's wrong with copy or assignment for this class?  Structs typically don't
include this restriction, since they're just simple data containers.

https://codereview.chromium.org/372473004/diff/280001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/280001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:276:
local_state_->ClearPref(prefs::kMetricsResetIds);
This should clear the backup as well.

https://codereview.chromium.org/372473004/diff/280001/components/metrics/metr...
File components/metrics/metrics_state_manager.h (right):

https://codereview.chromium.org/372473004/diff/280001/components/metrics/metr...
components/metrics/metrics_state_manager.h:107: void BackupCurrentClientInfo();
nit: "Backup" -> "BackUp"

https://codereview.chromium.org/372473004/diff/280001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/372473004/diff/280001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32531: +<histogram
name="UMA.ClientIdBackupRecoveredWithAge" units="hours">
These units are not currently accurate -- UMA_HISTOGRAM_CUSTOM_TIMES always
emits values in milliseconds.

[gab, 2014-07-11 20:33:41]

Thanks, comments addressed and added tests to
metrics_state_manager_unittests.cc.

Cheers!
Gab

https://codereview.chromium.org/372473004/diff/280001/chrome/browser/google/g...
File chrome/browser/google/google_update_settings_posix.cc (right):

https://codereview.chromium.org/372473004/diff/280001/chrome/browser/google/g...
chrome/browser/google/google_update_settings_posix.cc:70: // TODO(gab):
Implement |installation_date| storing/loading on POSIX.
On 2014/07/11 04:45:40, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

https://codereview.chromium.org/372473004/diff/280001/chrome/browser/google/g...
chrome/browser/google/google_update_settings_posix.cc:83: // TODO(gab):
Implement |installation_date| storing/loading on POSIX.
On 2014/07/11 04:45:40, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

https://codereview.chromium.org/372473004/diff/280001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/372473004/diff/280001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:300:
scoped_ptr<metrics::ClientInfo> client_info(new metrics::ClientInfo);
On 2014/07/11 04:45:41, Ilya Sherman wrote:
> nit: Can this be moved down to just above line 308?

Done.

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
File components/metrics/client_info.h (right):

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
components/metrics/client_info.h:18: public:
On 2014/07/11 04:45:41, Ilya Sherman wrote:
> nit: Structs are public by default.

Yes, but since I had a private section below I thought it was weird to leave it
implicitly public here...

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
components/metrics/client_info.h:22: std::string client_id;
On 2014/07/11 04:45:41, Ilya Sherman wrote:
> nit: Please leave a blank line between adjacent documented fields.

Done.

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
components/metrics/client_info.h:25: // The date on which reporting was enabled:
represented as an epoch time in
On 2014/07/11 04:45:41, Ilya Sherman wrote:
> nit: "on which reporting was enabled" -> "on which metrics reporting was
> enabled"

Done.

https://codereview.chromium.org/372473004/diff/280001/components/metrics/clie...
components/metrics/client_info.h:30: DISALLOW_COPY_AND_ASSIGN(ClientInfo);
On 2014/07/11 04:45:41, Ilya Sherman wrote:
> What's wrong with copy or assignment for this class?  Structs typically don't
> include this restriction, since they're just simple data containers.

I think the same argument to put DISALLOW_COPY_AND_ASSIGN on classes (to avoid
unintended copies of multi-member classes) applies on structs which are no
different from classes in that regard.

If I was to remove this I think it would also make sense to not make efforts to
pass ClientInfo by scoped_ptr, but rather just pass it by value, but IMO
avoiding the copies is nice (and this makes it explicit).

The style guide says to either specify an explicit copy constructor or use this
macro:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...

And it also says to use struct in data-only cases:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...

So IMO struct + DISALLOW_COPY_AND_ASSIGN is correct here.

https://codereview.chromium.org/372473004/diff/280001/components/metrics/metr...
File components/metrics/metrics_state_manager.h (right):

https://codereview.chromium.org/372473004/diff/280001/components/metrics/metr...
components/metrics/metrics_state_manager.h:107: void BackupCurrentClientInfo();
On 2014/07/11 04:45:41, Ilya Sherman wrote:
> nit: "Backup" -> "BackUp"

Done.

https://codereview.chromium.org/372473004/diff/280001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/372473004/diff/280001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32531: +<histogram
name="UMA.ClientIdBackupRecoveredWithAge" units="hours">
On 2014/07/11 04:45:41, Ilya Sherman wrote:
> These units are not currently accurate -- UMA_HISTOGRAM_CUSTOM_TIMES always
> emits values in milliseconds.

I see, I looked at other instances using "hours" as units and they use
UMA_HISTOGRAM_COUNTS_10000 (giving us 416 days) which works for me, switched to
that.

(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...)

[Ilya Sherman, 2014-07-11 23:25:00]

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:308:
local_state_->ClearPref(prefs::kMetricsResetIds);
This should clear the backup as well.  (Looks like this comment got lost from
the previous set.)

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager_unittest.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:46: 
nit: Spurious blank lines.

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:85: return
backup_copy.Pass();
Wouldn't this be easier if copying the struct were allowed? ;)

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:220: scoped_ptr<ClientInfo>
tmp_client_info;
nit: Perhaps s/tmp/previous/?

[gab, 2014-07-14 19:17:15]

Thanks, PTAL!

Cheers,
Gab

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:308:
local_state_->ClearPref(prefs::kMetricsResetIds);
On 2014/07/11 23:25:00, Ilya Sherman wrote:
> This should clear the backup as well.  (Looks like this comment got lost from
> the previous set.)

Done (and sorry I missed that previously, definitely the first time I noticed
this comment..!).

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager_unittest.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:46: 
On 2014/07/11 23:25:00, Ilya Sherman wrote:
> nit: Spurious blank lines.

Done.

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:85: return
backup_copy.Pass();
On 2014/07/11 23:25:00, Ilya Sherman wrote:
> Wouldn't this be easier if copying the struct were allowed? ;)

Yes, except that by [1] this means I would need an explicit copy constructor.

And I don't like adding extra complexity in the implementation only for the sake
of tests when it's not required :-)!

[1]
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:220: scoped_ptr<ClientInfo>
tmp_client_info;
On 2014/07/11 23:25:00, Ilya Sherman wrote:
> nit: Perhaps s/tmp/previous/?

Done.

[grt (UTC plus 2), 2014-07-14 19:40:21]

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager_unittest.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:85: return
backup_copy.Pass();
On 2014/07/14 19:17:14, gab wrote:
> On 2014/07/11 23:25:00, Ilya Sherman wrote:
> > Wouldn't this be easier if copying the struct were allowed? ;)
> 
> Yes, except that by [1] this means I would need an explicit copy constructor.
> 
> And I don't like adding extra complexity in the implementation only for the
sake
> of tests when it's not required :-)!
> 
> [1]
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...

Is this true for structs? I thought structs were permitted to use the implicit
copy ctor.

[gab, 2014-07-14 19:47:21]

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager_unittest.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:85: return
backup_copy.Pass();
On 2014/07/14 19:40:21, grt wrote:
> On 2014/07/14 19:17:14, gab wrote:
> > On 2014/07/11 23:25:00, Ilya Sherman wrote:
> > > Wouldn't this be easier if copying the struct were allowed? ;)
> > 
> > Yes, except that by [1] this means I would need an explicit copy
constructor.
> > 
> > And I don't like adding extra complexity in the implementation only for the
> sake
> > of tests when it's not required :-)!
> > 
> > [1]
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...
> 
> Is this true for structs? I thought structs were permitted to use the implicit
> copy ctor.

I think they are, but I don't see a good reason to have it in this case.

(and I also don't see why it's okay for structs, not for classes... IMO the same
reasons that apply to classes directly apply to structs... the story will be
different with C++11 moves, but for now I'd rather move this by pointer than by
value)

[Ilya Sherman, 2014-07-15 00:54:15]

LGTM, thanks!

https://codereview.chromium.org/372473004/diff/340001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/340001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:310:
store_client_info_.Run(ClientInfo());
Optional nit: Perhaps add a comment like "// Clear the backup as well."

[gab, 2014-07-15 02:02:54]

Thanks, done and added an extra test :-).

@cpu for chrome/app stamp
@thestig for chrome/common stamp

Thanks!
Gab

https://codereview.chromium.org/372473004/diff/340001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/340001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:310:
store_client_info_.Run(ClientInfo());
On 2014/07/15 00:54:14, Ilya Sherman wrote:
> Optional nit: Perhaps add a comment like "// Clear the backup as well."

Done.

[Lei Zhang, 2014-07-15 02:05:00]

lgtm

[Alexei Svitkine (slow), 2014-07-15 12:53:26]

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_services_manager.cc:79:
base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
Just to confirm, are these two OK to run on UI thread?

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:300: 
Nit: Remove empty line.

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:312: &installation_date_str)) {
Nit: Align.

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:318:
&reporting_enbaled_date_date_str)) {
Nit: Align.

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:328:
WriteGoogleUpdateStrKey(google_update::kRegMetricsId,
Is it okay to not check for errors for these? If so, add a comment.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
File components/metrics/client_info.h (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
components/metrics/client_info.h:19: ClientInfo();
Nit: Add a destructor decl here and define it out of line.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:92: const base::Time
now(base::Time::Now());
Nit: =

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:100:
client_info_backup->installation_date ?
Nit: != 0, since it's an int. Same below.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:240:
DCHECK(client_info->client_id.find('-') == std::string::npos);
Nit: DCHECK_EQ

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:253: DCHECK(client_id_it ==
client_info->client_id.end());
Nit: DCHECK_EQ

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:263: 
Nit: Remove empty line.

[grt (UTC plus 2), 2014-07-15 16:04:27]

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager_unittest.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:85: return
backup_copy.Pass();
On 2014/07/14 19:47:21, gab wrote:
> On 2014/07/14 19:40:21, grt wrote:
> > On 2014/07/14 19:17:14, gab wrote:
> > > On 2014/07/11 23:25:00, Ilya Sherman wrote:
> > > > Wouldn't this be easier if copying the struct were allowed? ;)
> > > 
> > > Yes, except that by [1] this means I would need an explicit copy
> constructor.
> > > 
> > > And I don't like adding extra complexity in the implementation only for
the
> > sake
> > > of tests when it's not required :-)!
> > > 
> > > [1]
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...
> > 
> > Is this true for structs? I thought structs were permitted to use the
implicit
> > copy ctor.
> 
> I think they are, but I don't see a good reason to have it in this case.
> 
> (and I also don't see why it's okay for structs, not for classes... IMO the
same
> reasons that apply to classes directly apply to structs... the story will be
> different with C++11 moves, but for now I'd rather move this by pointer than
by
> value)

The reason we disallow copy and assign by default for classes (exceptions are
allowed) is that it can be hard to tell just by looking at a class whether or
not it's horribly inefficient to treat it as a value type. In the case of
something so simple as this struct (a string and a pair of 64-bit ints?), it's
easy to see by inspection that treating it as a value type is not horribly
inefficient. I don't think the rule should be a blind: okay to copy structs, not
okay to copy classes, since it's easy to make a struct that is inefficient to
copy. It's perfectly okay to create a struct and document it as being safe to
treat as a value, though. (Note: just continuing the discussion. I'm not asking
you to change anything here.)

[gab, 2014-07-15 20:50:06]

Thanks grt/asvitkine PTAL!

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager_unittest.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:85: return
backup_copy.Pass();
On 2014/07/15 16:04:27, grt wrote:
> On 2014/07/14 19:47:21, gab wrote:
> > On 2014/07/14 19:40:21, grt wrote:
> > > On 2014/07/14 19:17:14, gab wrote:
> > > > On 2014/07/11 23:25:00, Ilya Sherman wrote:
> > > > > Wouldn't this be easier if copying the struct were allowed? ;)
> > > > 
> > > > Yes, except that by [1] this means I would need an explicit copy
> > constructor.
> > > > 
> > > > And I don't like adding extra complexity in the implementation only for
> the
> > > sake
> > > > of tests when it's not required :-)!
> > > > 
> > > > [1]
> > > >
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...
> > > 
> > > Is this true for structs? I thought structs were permitted to use the
> implicit
> > > copy ctor.
> > 
> > I think they are, but I don't see a good reason to have it in this case.
> > 
> > (and I also don't see why it's okay for structs, not for classes... IMO the
> same
> > reasons that apply to classes directly apply to structs... the story will be
> > different with C++11 moves, but for now I'd rather move this by pointer than
> by
> > value)
> 
> The reason we disallow copy and assign by default for classes (exceptions are
> allowed) is that it can be hard to tell just by looking at a class whether or
> not it's horribly inefficient to treat it as a value type. In the case of
> something so simple as this struct (a string and a pair of 64-bit ints?), it's
> easy to see by inspection that treating it as a value type is not horribly
> inefficient. I don't think the rule should be a blind: okay to copy structs,
not
> okay to copy classes, since it's easy to make a struct that is inefficient to
> copy. It's perfectly okay to create a struct and document it as being safe to
> treat as a value, though. (Note: just continuing the discussion. I'm not
asking
> you to change anything here.)

I see, makes sense, if you insist I'm happy to remove DISALLOW_COPY_AND_ASSIGN,
but then I'd make it explicitly passed by value everywhere (i.e. no scoped_ptr
passing), I don't think that allowing copies yet not using them is desired.

I'm willing to go either way, let me know if you feel it should be changed.

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_services_manager.cc:79:
base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Just to confirm, are these two OK to run on UI thread?

On Windows yes (registry access is not considered IO).

On Posix (google_update_settings_posix.cc), a file is used, but this was already
the case before this CL and it uses base::WriteFile which has ThreadRestrictions
checks (so I've been guessing it was okay...).

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:300: 
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: Remove empty line.

Done.

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:312: &installation_date_str)) {
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:318:
&reporting_enbaled_date_date_str)) {
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/372473004/diff/360001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:328:
WriteGoogleUpdateStrKey(google_update::kRegMetricsId,
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Is it okay to not check for errors for these? If so, add a comment.

The method used to return bool, but no callers would handle the error (as there
isn't much to be done about it).

StoreMetricsClientInfo is thus effectively a best effort at saving a backup,
added a comment to this effect.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
File components/metrics/client_info.h (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
components/metrics/client_info.h:19: ClientInfo();
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: Add a destructor decl here and define it out of line.

Chromium style guide explicitly allows inline destructors when the class is made
strictly of simple members (i.e., POD types or types which are themselves only
made of POD types and have an inline constructor -- I remember that std::string
was explicitly allowed in this scenario):
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo....

In fact clang_compile bots complain when you forget the out-of-line destructor
and your class is considered "complex" (and they don't complain on this CL so it
must be okay IMO).

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:92: const base::Time
now(base::Time::Now());
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: =

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

explicitly allows both.

In general I prefer to use () to init objects and = to init POD types/refs.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:100:
client_info_backup->installation_date ?
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: != 0, since it's an int. Same below.

0 is explicitly false in C++; I don't think there is a need to be more explicit
here, ints are first-class citizen in conditionals, there is no implicit
conversion to bool required to evaluate this statement (in fact !=0 would
probably generate more assembly).

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:240:
DCHECK(client_info->client_id.find('-') == std::string::npos);
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: DCHECK_EQ

DCHECK_EQ is useful when the output of "actual"/"expected" is both:
 1) Useful
 2) Printable

In this case DCHECK_EQ won't even compile as the macro wouldn't know how to
"print" an iterator on failure.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:253: DCHECK(client_id_it ==
client_info->client_id.end());
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: DCHECK_EQ

Nope, same as above.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:263: 
On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> Nit: Remove empty line.

Done.

[grt (UTC plus 2), 2014-07-16 13:52:31]

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
File components/metrics/metrics_state_manager_unittest.cc (right):

https://codereview.chromium.org/372473004/diff/320001/components/metrics/metr...
components/metrics/metrics_state_manager_unittest.cc:85: return
backup_copy.Pass();
On 2014/07/15 20:50:05, gab wrote:
> On 2014/07/15 16:04:27, grt wrote:
> > On 2014/07/14 19:47:21, gab wrote:
> > > On 2014/07/14 19:40:21, grt wrote:
> > > > On 2014/07/14 19:17:14, gab wrote:
> > > > > On 2014/07/11 23:25:00, Ilya Sherman wrote:
> > > > > > Wouldn't this be easier if copying the struct were allowed? ;)
> > > > > 
> > > > > Yes, except that by [1] this means I would need an explicit copy
> > > constructor.
> > > > > 
> > > > > And I don't like adding extra complexity in the implementation only
for
> > the
> > > > sake
> > > > > of tests when it's not required :-)!
> > > > > 
> > > > > [1]
> > > > >
> > > >
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...
> > > > 
> > > > Is this true for structs? I thought structs were permitted to use the
> > implicit
> > > > copy ctor.
> > > 
> > > I think they are, but I don't see a good reason to have it in this case.
> > > 
> > > (and I also don't see why it's okay for structs, not for classes... IMO
the
> > same
> > > reasons that apply to classes directly apply to structs... the story will
be
> > > different with C++11 moves, but for now I'd rather move this by pointer
than
> > by
> > > value)
> > 
> > The reason we disallow copy and assign by default for classes (exceptions
are
> > allowed) is that it can be hard to tell just by looking at a class whether
or
> > not it's horribly inefficient to treat it as a value type. In the case of
> > something so simple as this struct (a string and a pair of 64-bit ints?),
it's
> > easy to see by inspection that treating it as a value type is not horribly
> > inefficient. I don't think the rule should be a blind: okay to copy structs,
> not
> > okay to copy classes, since it's easy to make a struct that is inefficient
to
> > copy. It's perfectly okay to create a struct and document it as being safe
to
> > treat as a value, though. (Note: just continuing the discussion. I'm not
> asking
> > you to change anything here.)
> 
> I see, makes sense, if you insist I'm happy to remove
DISALLOW_COPY_AND_ASSIGN,
> but then I'd make it explicitly passed by value everywhere (i.e. no scoped_ptr
> passing), I don't think that allowing copies yet not using them is desired.
> 
> I'm willing to go either way, let me know if you feel it should be changed.

I don't feel strongly that you must change it in this CL. I haven't looked at
how it's used, but if you think it simplifies the code, then go for it. Your
call.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
File components/metrics/client_info.h (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
components/metrics/client_info.h:19: ClientInfo();
On 2014/07/15 20:50:06, gab wrote:
> On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > Nit: Add a destructor decl here and define it out of line.
> 
> Chromium style guide explicitly allows inline destructors when the class is
made
> strictly of simple members (i.e., POD types or types which are themselves only
> made of POD types and have an inline constructor -- I remember that
std::string
> was explicitly allowed in this scenario):
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo....

The style guide allows it for trivial dtors because the compiler can optimize
away the dtor (for the most part). std::string does not have a trivial dtor (it
may need to free allocated memory, so it or one of its constituents must have a
non-implicit dtor), so this struct doesn't have a trivial dtor. Although
Chromium treats std::string as a value type, I don't remember seeing any
discussion that you can use the implicit dtor if your type is
POD-except-for-a-string.

[Alexei Svitkine (slow), 2014-07-16 14:39:56]

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:92: const base::Time
now(base::Time::Now());
On 2014/07/15 20:50:06, gab wrote:
> On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > Nit: =
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...
> 
> explicitly allows both.
> 
> In general I prefer to use () to init objects and = to init POD types/refs.

base::Time is essentially a POD type, since it just wraps an int64. Anyway, I
know both are allowed, so in those cases the guideline is to be consistent with
surrounding code and/or follow the preference of the reviewer/owner (who will be
maintaining this code afterwards). In this case, my preference as owner/review
is to use =.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:100:
client_info_backup->installation_date ?
On 2014/07/15 20:50:06, gab wrote:
> On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > Nit: != 0, since it's an int. Same below.
> 
> 0 is explicitly false in C++; I don't think there is a need to be more
explicit
> here, ints are first-class citizen in conditionals, there is no implicit
> conversion to bool required to evaluate this statement (in fact !=0 would
> probably generate more assembly).

I am aware how C++ works, my argument is not about correctness but about
readability.

It's better to use != 0 checks for int types, since this avoids possible
confusion about what's being checked (e.g. whether its a NULL ptr check or an
integer check). That's why I (and many other Chromium reviewers) prefer that for
int types, that non-zero check is explicit.

(Also, your assertion that != 0 will generate worse machine code is unfounded.
All modern compilers are smart enough to generate the same code for both cases.)

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:240:
DCHECK(client_info->client_id.find('-') == std::string::npos);
On 2014/07/15 20:50:06, gab wrote:
> On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > Nit: DCHECK_EQ
> 
> DCHECK_EQ is useful when the output of "actual"/"expected" is both:
>  1) Useful
>  2) Printable
> 
> In this case DCHECK_EQ won't even compile as the macro wouldn't know how to
> "print" an iterator on failure.

Fair enough.

[Alexei Svitkine (slow), 2014-07-16 14:43:58]

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_services_manager.cc:79:
base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
On 2014/07/15 20:50:05, gab wrote:
> On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > Just to confirm, are these two OK to run on UI thread?
> 
> On Windows yes (registry access is not considered IO).
> 
> On Posix (google_update_settings_posix.cc), a file is used, but this was
already
> the case before this CL and it uses base::WriteFile which has
ThreadRestrictions
> checks (so I've been guessing it was okay...).

Hmm, I think the POSIX code is problematic. I'm sure it will result in
AssertIOAllowed() triggering in a debug build. But it looks like only
StoreMetricsClientInfo() does IO, but LoadMetricsClientInfo doesn't.

In which case, I think we can fix this by making the POSIX impl of
StoreMetricsClientInfo() post a task to the blocking pool where IO is allowed.

[erikwright (departed), 2014-07-16 15:44:23]

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
File components/metrics/client_info.h (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
components/metrics/client_info.h:19: ClientInfo();
On 2014/07/16 13:52:31, grt wrote:
> On 2014/07/15 20:50:06, gab wrote:
> > On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > > Nit: Add a destructor decl here and define it out of line.
> > 
> > Chromium style guide explicitly allows inline destructors when the class is
> made
> > strictly of simple members (i.e., POD types or types which are themselves
only
> > made of POD types and have an inline constructor -- I remember that
> std::string
> > was explicitly allowed in this scenario):
> >
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo....
> 
> The style guide allows it for trivial dtors because the compiler can optimize
> away the dtor (for the most part). std::string does not have a trivial dtor
(it
> may need to free allocated memory, so it or one of its constituents must have
a
> non-implicit dtor), so this struct doesn't have a trivial dtor. Although
> Chromium treats std::string as a value type, I don't remember seeing any
> discussion that you can use the implicit dtor if your type is
> POD-except-for-a-string.

I infer from the fact that the clang style rules allow this inline destructor
that some objective analysis has been done to say that it is reasonable/right to
inline destructors when the only non-POD is a string.

But I don't know of any documentation to that effect.

So I would follow the letter of the style-guide (which dictates an out-of-line
DTOR here) unless I could find the rationale behind the clang rule (probably by
pinging the author) and then update the styleguide to reflect it.

[gab, 2014-07-16 15:51:43]

Nits applied, PTAL.

Cheers,
Gab

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_services_manager.cc:79:
base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
On 2014/07/16 14:43:58, Alexei Svitkine wrote:
> On 2014/07/15 20:50:05, gab wrote:
> > On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > > Just to confirm, are these two OK to run on UI thread?
> > 
> > On Windows yes (registry access is not considered IO).
> > 
> > On Posix (google_update_settings_posix.cc), a file is used, but this was
> already
> > the case before this CL and it uses base::WriteFile which has
> ThreadRestrictions
> > checks (so I've been guessing it was okay...).
> 
> Hmm, I think the POSIX code is problematic. I'm sure it will result in
> AssertIOAllowed() triggering in a debug build. But it looks like only
> StoreMetricsClientInfo() does IO, but LoadMetricsClientInfo doesn't.
> 
> In which case, I think we can fix this by making the POSIX impl of
> StoreMetricsClientInfo() post a task to the blocking pool where IO is allowed.

Right, but GoogleUpdateSettings::StoreMetricsClientInfo() was already called
from ChromeMetricsServiceClient::SetMetricsClientId() before this CL.

I'm happy to fix this in my follow-up for POSIX (see this CL's description), but
for now I don't think this CL makes this worse on POSIX and I'd rather avoid
touching the POSIX impl in this CL.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
File components/metrics/client_info.h (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/clie...
components/metrics/client_info.h:19: ClientInfo();
On 2014/07/16 15:44:22, erikwright wrote:
> On 2014/07/16 13:52:31, grt wrote:
> > On 2014/07/15 20:50:06, gab wrote:
> > > On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > > > Nit: Add a destructor decl here and define it out of line.
> > > 
> > > Chromium style guide explicitly allows inline destructors when the class
is
> > made
> > > strictly of simple members (i.e., POD types or types which are themselves
> only
> > > made of POD types and have an inline constructor -- I remember that
> > std::string
> > > was explicitly allowed in this scenario):
> > >
> >
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo....
> > 
> > The style guide allows it for trivial dtors because the compiler can
optimize
> > away the dtor (for the most part). std::string does not have a trivial dtor
> (it
> > may need to free allocated memory, so it or one of its constituents must
have
> a
> > non-implicit dtor), so this struct doesn't have a trivial dtor. Although
> > Chromium treats std::string as a value type, I don't remember seeing any
> > discussion that you can use the implicit dtor if your type is
> > POD-except-for-a-string.
> 
> I infer from the fact that the clang style rules allow this inline destructor
> that some objective analysis has been done to say that it is reasonable/right
to
> inline destructors when the only non-POD is a string.
> 
> But I don't know of any documentation to that effect.
> 
> So I would follow the letter of the style-guide (which dictates an out-of-line
> DTOR here) unless I could find the rationale behind the clang rule (probably
by
> pinging the author) and then update the styleguide to reflect it.

SG, thanks, I'll just do it to close this argument.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:92: const base::Time
now(base::Time::Now());
On 2014/07/16 14:39:56, Alexei Svitkine wrote:
> On 2014/07/15 20:50:06, gab wrote:
> > On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > > Nit: =
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...
> > 
> > explicitly allows both.
> > 
> > In general I prefer to use () to init objects and = to init POD types/refs.
> 
> base::Time is essentially a POD type, since it just wraps an int64. Anyway, I
> know both are allowed, so in those cases the guideline is to be consistent
with
> surrounding code and/or follow the preference of the reviewer/owner (who will
be
> maintaining this code afterwards). In this case, my preference as owner/review
> is to use =.

As you wish, done.

https://codereview.chromium.org/372473004/diff/360001/components/metrics/metr...
components/metrics/metrics_state_manager.cc:100:
client_info_backup->installation_date ?
On 2014/07/16 14:39:56, Alexei Svitkine wrote:
> On 2014/07/15 20:50:06, gab wrote:
> > On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > > Nit: != 0, since it's an int. Same below.
> > 
> > 0 is explicitly false in C++; I don't think there is a need to be more
> explicit
> > here, ints are first-class citizen in conditionals, there is no implicit
> > conversion to bool required to evaluate this statement (in fact !=0 would
> > probably generate more assembly).
> 
> I am aware how C++ works, my argument is not about correctness but about
> readability.
> 
> It's better to use != 0 checks for int types, since this avoids possible
> confusion about what's being checked (e.g. whether its a NULL ptr check or an
> integer check). That's why I (and many other Chromium reviewers) prefer that
for
> int types, that non-zero check is explicit.
> 
> (Also, your assertion that != 0 will generate worse machine code is unfounded.
> All modern compilers are smart enough to generate the same code for both
cases.)

Apologies if it felt like I was putting your C++ knowledge in doubt; I was only
trying to justify that it doesn't matter... I have never seen such pushbacks
from other reviewers, but I'm happy to do as you wish here.

https://codereview.chromium.org/372473004/diff/400001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (left):

https://codereview.chromium.org/372473004/diff/400001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:170:
GoogleUpdateSettings::StoreMetricsClientId(client_id);
(here)

[Alexei Svitkine (slow), 2014-07-16 19:18:22]

LGTM, thanks!

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/372473004/diff/360001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_services_manager.cc:79:
base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
On 2014/07/16 15:51:43, gab wrote:
> On 2014/07/16 14:43:58, Alexei Svitkine wrote:
> > On 2014/07/15 20:50:05, gab wrote:
> > > On 2014/07/15 12:53:25, Alexei Svitkine wrote:
> > > > Just to confirm, are these two OK to run on UI thread?
> > > 
> > > On Windows yes (registry access is not considered IO).
> > > 
> > > On Posix (google_update_settings_posix.cc), a file is used, but this was
> > already
> > > the case before this CL and it uses base::WriteFile which has
> > ThreadRestrictions
> > > checks (so I've been guessing it was okay...).
> > 
> > Hmm, I think the POSIX code is problematic. I'm sure it will result in
> > AssertIOAllowed() triggering in a debug build. But it looks like only
> > StoreMetricsClientInfo() does IO, but LoadMetricsClientInfo doesn't.
> > 
> > In which case, I think we can fix this by making the POSIX impl of
> > StoreMetricsClientInfo() post a task to the blocking pool where IO is
allowed.
> 
> Right, but GoogleUpdateSettings::StoreMetricsClientInfo() was already called
> from ChromeMetricsServiceClient::SetMetricsClientId() before this CL.
> 
> I'm happy to fix this in my follow-up for POSIX (see this CL's description),
but
> for now I don't think this CL makes this worse on POSIX and I'd rather avoid
> touching the POSIX impl in this CL.

SGTM, but please fix in a follow-up CL.

[gab, 2014-07-16 20:36:54]

@grt/asvitkine/isherman : see below

Thanks!
Gab

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:188:
'<(DEPTH)/components/components.gyp:metrics',
This dependency causes link errors between x86 vs x64 targets on Windows:

metrics.lib(metrics.persisted_logs.obj) : fatalerror LNK1112: module machine
type 'X86' conflicts with target machine type 'x64'

I don't really understand why (persistent_logs.cc uses base/base64.h...?!)...

The only solution I can think of is to introduce the metrics_common target as I
originally was going to do (which would only contain client_info.[cc|h] for
now).

WDYT?

[Ilya Sherman, 2014-07-16 20:40:43]

+Justin for help with 64-bit Win build

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:188:
'<(DEPTH)/components/components.gyp:metrics',
On 2014/07/16 20:36:54, gab wrote:
> This dependency causes link errors between x86 vs x64 targets on Windows:
> 
> metrics.lib(metrics.persisted_logs.obj) : fatalerror LNK1112: module machine
> type 'X86' conflicts with target machine type 'x64'
> 
> I don't really understand why (persistent_logs.cc uses base/base64.h...?!)...
> 
> The only solution I can think of is to introduce the metrics_common target as
I
> originally was going to do (which would only contain client_info.[cc|h] for
> now).
> 
> WDYT?

Justin, could you help us understand this error message in the 64-bit Windows
build?  (FWIW, I'm pretty sure base/base64.h is irrelevant.)

[grt (UTC plus 2), 2014-07-16 22:18:29]

take a look at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_exe.....
my guess is you need to jump through hoops to make a *_win64 target in the
metrics component. just a hunch.

https://codereview.chromium.org/372473004/diff/460001/components/metrics/clie...
File components/metrics/client_info.h (right):

https://codereview.chromium.org/372473004/diff/460001/components/metrics/clie...
components/metrics/client_info.h:26: int64 installation_date;
i'm sure this is unrelated, but new code should include stdint.h and use int64_t
as per the comment in basictypes.h

[jschuh, 2014-07-16 23:40:19]

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:188:
'<(DEPTH)/components/components.gyp:metrics',
On 2014/07/16 20:40:42, Ilya Sherman wrote:
> On 2014/07/16 20:36:54, gab wrote:
> > This dependency causes link errors between x86 vs x64 targets on Windows:
> > 
> > metrics.lib(metrics.persisted_logs.obj) : fatalerror LNK1112: module machine
> > type 'X86' conflicts with target machine type 'x64'
> > 
> > I don't really understand why (persistent_logs.cc uses
base/base64.h...?!)...
> > 
> > The only solution I can think of is to introduce the metrics_common target
as
> I
> > originally was going to do (which would only contain client_info.[cc|h] for
> > now).
> > 
> > WDYT?
> 
> Justin, could you help us understand this error message in the 64-bit Windows
> build?  (FWIW, I'm pretty sure base/base64.h is irrelevant.)

I'm stumped. The error message means that you're grabbing a 32-bit lib in the
64-bit build. It doesn't look like you're building the 32-bit target, so maybe
you're not building the 64-bit target and end up picking up a 32-bit default?

[cpu_(ooo_6.6-7.5), 2014-07-17 01:36:06]

client_util.cc and deps lgtm

[gab, 2014-07-17 21:51:31]

@asvitkine/isherman: looks like we need some support of metrics_win64 target
(see similar such targets over chrome, e.g. [1])...

Do you want that to be all of "metrics" or a "metrics_common" target containing
only client_info.[h|cc] for now?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common_cons...

[gab, 2014-07-17 21:51:49]

On 2014/07/17 21:51:31, gab wrote:
> @asvitkine/isherman: looks like we need some support of metrics_win64 target

s/support/sort

> (see similar such targets over chrome, e.g. [1])...
> 
> Do you want that to be all of "metrics" or a "metrics_common" target
containing
> only client_info.[h|cc] for now?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common_cons...

[Ilya Sherman, 2014-07-17 23:00:04]

On 2014/07/17 21:51:31, gab wrote:
> @asvitkine/isherman: looks like we need some sort of metrics_win64 target
> (see similar such targets over chrome, e.g. [1])...
> 
> Do you want that to be all of "metrics" or a "metrics_common" target
containing
> only client_info.[h|cc] for now?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common_cons...

How do we currently build a 64-bit Chrome build with metrics enabled? 
Presumably it needs the same degree of support?  I'd rather make the full
component build properly on 64-bit Windows rather than splitting off a mini
component for this struct.

[grt (UTC plus 2), 2014-07-18 01:14:49]

i just took another look at this for grins. see below.

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:188:
'<(DEPTH)/components/components.gyp:metrics',
On 2014/07/16 20:36:54, gab wrote:
> This dependency causes link errors between x86 vs x64 targets on Windows:

is this dependency even needed? installer_util_nacl_win64 doesn't build the
sources that would need the metrics component, does it
(google_update_settings.{cc,h})? what happens if you remove this line?

[gab, 2014-07-18 02:41:07]

@Ilya: note that this is not a 64bit build issue, it's an issue with building
nacl64 in 32bit builds..

Fixing this would essentially require having a metrics_win64 target mostly
identical to metrics but that depends on base_win64 etc. instead.

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:188:
'<(DEPTH)/components/components.gyp:metrics',
On 2014/07/18 01:14:49, grt wrote:
> On 2014/07/16 20:36:54, gab wrote:
> > This dependency causes link errors between x86 vs x64 targets on Windows:
> 
> is this dependency even needed? installer_util_nacl_win64 doesn't build the
> sources that would need the metrics component, does it
> (google_update_settings.{cc,h})? what happens if you remove this line?

It does, google_update_settings.cc is in target_defaults (i.e. all targets in
this file..)

[grt (UTC plus 2), 2014-07-18 13:53:41]

On 2014/07/18 02:41:07, gab wrote:
> @Ilya: note that this is not a 64bit build issue, it's an issue with building
> nacl64 in 32bit builds..
> 
> Fixing this would essentially require having a metrics_win64 target mostly
> identical to metrics but that depends on base_win64 etc. instead.
> 
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> File chrome/chrome_installer_util.gypi (right):
> 
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> chrome/chrome_installer_util.gypi:188:
> '<(DEPTH)/components/components.gyp:metrics',
> On 2014/07/18 01:14:49, grt wrote:
> > On 2014/07/16 20:36:54, gab wrote:
> > > This dependency causes link errors between x86 vs x64 targets on Windows:
> > 
> > is this dependency even needed? installer_util_nacl_win64 doesn't build the
> > sources that would need the metrics component, does it
> > (google_update_settings.{cc,h})? what happens if you remove this line?
> 
> It does, google_update_settings.cc is in target_defaults (i.e. all targets in
> this file..)

Duh. This is what I get for attempting a code review too late at night.
Apologies for the noise.

Looks like adding metrics_common and metrics_common_win64 targets would work. It
seems simpler to add a metrics_win64 target that only includes the client_info
files. Would that work?

[Alexei Svitkine (slow), 2014-07-18 14:14:20]

On 2014/07/18 13:53:41, grt wrote:
> On 2014/07/18 02:41:07, gab wrote:
> > @Ilya: note that this is not a 64bit build issue, it's an issue with
building
> > nacl64 in 32bit builds..
> > 
> > Fixing this would essentially require having a metrics_win64 target mostly
> > identical to metrics but that depends on base_win64 etc. instead.
> > 
> >
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> > File chrome/chrome_installer_util.gypi (right):
> > 
> >
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> > chrome/chrome_installer_util.gypi:188:
> > '<(DEPTH)/components/components.gyp:metrics',
> > On 2014/07/18 01:14:49, grt wrote:
> > > On 2014/07/16 20:36:54, gab wrote:
> > > > This dependency causes link errors between x86 vs x64 targets on
Windows:
> > > 
> > > is this dependency even needed? installer_util_nacl_win64 doesn't build
the
> > > sources that would need the metrics component, does it
> > > (google_update_settings.{cc,h})? what happens if you remove this line?
> > 
> > It does, google_update_settings.cc is in target_defaults (i.e. all targets
in
> > this file..)
> 
> Duh. This is what I get for attempting a code review too late at night.
> Apologies for the noise.
> 
> Looks like adding metrics_common and metrics_common_win64 targets would work.
It
> seems simpler to add a metrics_win64 target that only includes the client_info
> files. Would that work?

Does nacl64.exe need this to depend on client_info? Or could the bit that
depends on client_info be moved to a separate library that nacl64 doesn't depend
on?

[grt (UTC plus 2), 2014-07-18 14:22:40]

On 2014/07/18 14:14:20, Alexei Svitkine wrote:
> On 2014/07/18 13:53:41, grt wrote:
> > On 2014/07/18 02:41:07, gab wrote:
> > > @Ilya: note that this is not a 64bit build issue, it's an issue with
> building
> > > nacl64 in 32bit builds..
> > > 
> > > Fixing this would essentially require having a metrics_win64 target mostly
> > > identical to metrics but that depends on base_win64 etc. instead.
> > > 
> > >
> >
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> > > File chrome/chrome_installer_util.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> > > chrome/chrome_installer_util.gypi:188:
> > > '<(DEPTH)/components/components.gyp:metrics',
> > > On 2014/07/18 01:14:49, grt wrote:
> > > > On 2014/07/16 20:36:54, gab wrote:
> > > > > This dependency causes link errors between x86 vs x64 targets on
> Windows:
> > > > 
> > > > is this dependency even needed? installer_util_nacl_win64 doesn't build
> the
> > > > sources that would need the metrics component, does it
> > > > (google_update_settings.{cc,h})? what happens if you remove this line?
> > > 
> > > It does, google_update_settings.cc is in target_defaults (i.e. all targets
> in
> > > this file..)
> > 
> > Duh. This is what I get for attempting a code review too late at night.
> > Apologies for the noise.
> > 
> > Looks like adding metrics_common and metrics_common_win64 targets would
work.
> It
> > seems simpler to add a metrics_win64 target that only includes the
client_info
> > files. Would that work?
> 
> Does nacl64.exe need this to depend on client_info? Or could the bit that
> depends on client_info be moved to a separate library that nacl64 doesn't
depend
> on?

I had presumed it was since I hope nacl64.exe connects to breakpad for crash
reporting. It would need to get the client_id from the registry for this, no? If
not, then by all means break the dependency. To be honest, I haven't looked into
whether or not nacl64 talks to breakpad, but I assume it does since it depends
on breakpad_win64.

Another option is to add client_info.{cc,h} to the chrome_nacl_win64 target
itself. It looks like a few other sources are pulled from here and there to get
around exactly the sort of problem you're running into here. That seems like a
hack, but it's a pre-existing one.

[jschuh, 2014-07-18 15:19:08]

On 2014/07/18 14:22:40, grt wrote:
> On 2014/07/18 14:14:20, Alexei Svitkine wrote:
> > On 2014/07/18 13:53:41, grt wrote:
> > > On 2014/07/18 02:41:07, gab wrote:
> > > > @Ilya: note that this is not a 64bit build issue, it's an issue with
> > building
> > > > nacl64 in 32bit builds..
> > > > 
> > > > Fixing this would essentially require having a metrics_win64 target
mostly
> > > > identical to metrics but that depends on base_win64 etc. instead.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> > > > File chrome/chrome_installer_util.gypi (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> > > > chrome/chrome_installer_util.gypi:188:
> > > > '<(DEPTH)/components/components.gyp:metrics',
> > > > On 2014/07/18 01:14:49, grt wrote:
> > > > > On 2014/07/16 20:36:54, gab wrote:
> > > > > > This dependency causes link errors between x86 vs x64 targets on
> > Windows:
> > > > > 
> > > > > is this dependency even needed? installer_util_nacl_win64 doesn't
build
> > the
> > > > > sources that would need the metrics component, does it
> > > > > (google_update_settings.{cc,h})? what happens if you remove this line?
> > > > 
> > > > It does, google_update_settings.cc is in target_defaults (i.e. all
targets
> > in
> > > > this file..)
> > > 
> > > Duh. This is what I get for attempting a code review too late at night.
> > > Apologies for the noise.
> > > 
> > > Looks like adding metrics_common and metrics_common_win64 targets would
> work.
> > It
> > > seems simpler to add a metrics_win64 target that only includes the
> client_info
> > > files. Would that work?
> > 
> > Does nacl64.exe need this to depend on client_info? Or could the bit that
> > depends on client_info be moved to a separate library that nacl64 doesn't
> depend
> > on?
> 
> I had presumed it was since I hope nacl64.exe connects to breakpad for crash
> reporting. It would need to get the client_id from the registry for this, no?
If
> not, then by all means break the dependency. To be honest, I haven't looked
into
> whether or not nacl64 talks to breakpad, but I assume it does since it depends
> on breakpad_win64.
> 
> Another option is to add client_info.{cc,h} to the chrome_nacl_win64 target
> itself. It looks like a few other sources are pulled from here and there to
get
> around exactly the sort of problem you're running into here. That seems like a
> hack, but it's a pre-existing one.

Now I'm very confused. We shouldn't be building nacl64.exe at all on Win64.
The *64 files are for the 32-bit builds only.

[gab, 2014-07-18 15:35:53]

On 2014/07/18 15:19:08, Justin Schuh wrote:
> On 2014/07/18 14:22:40, grt wrote:
> > On 2014/07/18 14:14:20, Alexei Svitkine wrote:
> > > On 2014/07/18 13:53:41, grt wrote:
> > > > On 2014/07/18 02:41:07, gab wrote:
> > > > > @Ilya: note that this is not a 64bit build issue, it's an issue with
> > > building
> > > > > nacl64 in 32bit builds..
> > > > > 
> > > > > Fixing this would essentially require having a metrics_win64 target
> mostly
> > > > > identical to metrics but that depends on base_win64 etc. instead.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> > > > > File chrome/chrome_installer_util.gypi (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/372473004/diff/460001/chrome/chrome_installer...
> > > > > chrome/chrome_installer_util.gypi:188:
> > > > > '<(DEPTH)/components/components.gyp:metrics',
> > > > > On 2014/07/18 01:14:49, grt wrote:
> > > > > > On 2014/07/16 20:36:54, gab wrote:
> > > > > > > This dependency causes link errors between x86 vs x64 targets on
> > > Windows:
> > > > > > 
> > > > > > is this dependency even needed? installer_util_nacl_win64 doesn't
> build
> > > the
> > > > > > sources that would need the metrics component, does it
> > > > > > (google_update_settings.{cc,h})? what happens if you remove this
line?
> > > > > 
> > > > > It does, google_update_settings.cc is in target_defaults (i.e. all
> targets
> > > in
> > > > > this file..)
> > > > 
> > > > Duh. This is what I get for attempting a code review too late at night.
> > > > Apologies for the noise.
> > > > 
> > > > Looks like adding metrics_common and metrics_common_win64 targets would
> > work.
> > > It
> > > > seems simpler to add a metrics_win64 target that only includes the
> > client_info
> > > > files. Would that work?
> > > 
> > > Does nacl64.exe need this to depend on client_info? Or could the bit that
> > > depends on client_info be moved to a separate library that nacl64 doesn't
> > depend
> > > on?
> > 
> > I had presumed it was since I hope nacl64.exe connects to breakpad for crash
> > reporting. It would need to get the client_id from the registry for this,
no?
> If
> > not, then by all means break the dependency. To be honest, I haven't looked
> into
> > whether or not nacl64 talks to breakpad, but I assume it does since it
depends
> > on breakpad_win64.
> > 
> > Another option is to add client_info.{cc,h} to the chrome_nacl_win64 target
> > itself. It looks like a few other sources are pulled from here and there to
> get
> > around exactly the sort of problem you're running into here. That seems like
a
> > hack, but it's a pre-existing one.
> 
> Now I'm very confused. We shouldn't be building nacl64.exe at all on Win64.
> The *64 files are for the 32-bit builds only.

Yes, it's the nacl64 target that's having trouble in 32-bit bots (sorry for the
confusion!)

[gab, 2014-07-18 19:45:37]

Seems (from local testing) that hardcoding client_info.cc has a source for
installer_util_nacl_win64 fixes it.

This is not a pretty solution, but has grt pointed out, there is precedent for
it to workaround this exact problem with nacl64 on 32 bit builds.

In practice nacl64 doesn't need ClientInfo, but it needs GoogleUpdateSettings
which in turn needs ClientInfo for parts of its API. The parts of the API that
nacl64 uses don't need ClientInfo, but splitting GoogleUpdateSettings in
multiple independent pieces is not a small task (and one I'd rather not
undertake just to work around this limitation...).

PTAL.

Thanks!
Gab

[gab, 2014-07-21 21:30:13]

On 2014/07/18 19:45:37, gab wrote:
> Seems (from local testing) that hardcoding client_info.cc has a source for
> installer_util_nacl_win64 fixes it.
> 
> This is not a pretty solution, but has grt pointed out, there is precedent for
> it to workaround this exact problem with nacl64 on 32 bit builds.
> 
> In practice nacl64 doesn't need ClientInfo, but it needs GoogleUpdateSettings
> which in turn needs ClientInfo for parts of its API. The parts of the API that
> nacl64 uses don't need ClientInfo, but splitting GoogleUpdateSettings in
> multiple independent pieces is not a small task (and one I'd rather not
> undertake just to work around this limitation...).
> 
> PTAL.

ping ^^^

> 
> Thanks!
> Gab

[Ilya Sherman, 2014-07-21 21:49:45]

LGTM, though I think the relevant OWNER lives in another castle.

[gab, 2014-07-21 21:59:03]

On 2014/07/21 21:49:45, Ilya Sherman wrote:
> LGTM, though I think the relevant OWNER lives in another castle.

grt@ is the relevant owner and it was one of his suggestions :-)

grt@ please take a final look from your castle ;-)!

[grt (UTC plus 2), 2014-07-21 22:27:32]

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:193: # Include |client_info.cc| directly here
to avoid having to create a
hmm. i'm wondering if it's better to keep this hack alongside the others in the
chrome_nacl_win64 target of chrome_exe.gypi. are any of the other sources for
that target pulled for dependents of chrome_nacl_win64 rather than for the
target itself?

[gab, 2014-07-22 16:18:38]

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:193: # Include |client_info.cc| directly here
to avoid having to create a
On 2014/07/21 22:27:32, grt wrote:
> hmm. i'm wondering if it's better to keep this hack alongside the others in
the
> chrome_nacl_win64 target of chrome_exe.gypi. are any of the other sources for
> that target pulled for dependents of chrome_nacl_win64 rather than for the
> target itself?

Looking at the sources for chrome_nacl_win64 target, these all seem like direct
dependencies of the exe itself (e.g., breakpad); the chrome_initial target has
some of the same sources hardcoded.

In this case I think keeping client_info.cc as part of installer_util_nacl_win64
makes sense as it makes it clear what those are included for (otherwise it's
obscure what makes it such that chrome_nacl_win64 ends up needing those files).

[grt (UTC plus 2), 2014-07-22 16:47:12]

sounds good to me. lgtm w/ nits.

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:194: # metrics_win64 target solely for this
purpose...
nit: ... -> .

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:197: 
nit: remove newline

[gab, 2014-07-22 19:09:40]

Thanks, firing!

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
File chrome/chrome_installer_util.gypi (right):

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:194: # metrics_win64 target solely for this
purpose...
On 2014/07/22 16:47:12, grt wrote:
> nit: ... -> .

Done.

https://codereview.chromium.org/372473004/diff/540001/chrome/chrome_installer...
chrome/chrome_installer_util.gypi:197: 
On 2014/07/22 16:47:11, grt wrote:
> nit: remove newline

Done.

[gab, 2014-07-22 19:09:47]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-07-22 19:11:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/372473004/560001

[commit-bot: I haz the power, 2014-07-22 22:05:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-22 23:37:04]

Change committed as 284805