Refactor SetClientID such that metrics rather than crash backs up the client id in Google Update set

Refactor SetClientID such that metrics rather than crash backs up the client id
in Google Update settings.

Consequentially, the backed up client_id now keeps its dashes and crash_keys
strips them at runtime rather than when backing it up
(https://codereview.chromium.org/372473004/ will add support for stripped
client_id backups for some time).

Also rename a lot of methods involved in setting the client id; having all of
them named "SetClientID" makes this series of calls very hard to follow!

BUG=391338
TBR=thestig

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

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

Ilya PTAL at this in the context of https://codereview.chromium.org/372473004/

[Ilya Sherman, 2014-07-08 03:21:38]

https://codereview.chromium.org/365133005/diff/40001/chrome/app/chrome_breakp...
File chrome/app/chrome_breakpad_client.cc (right):

https://codereview.chromium.org/365133005/diff/40001/chrome/app/chrome_breakp...
chrome/app/chrome_breakpad_client.cc:83: void
ChromeBreakpadClient::InitClientID(const std::string& client_id) {
nit: Should the variable be renamed to include "guid" in the name as well?

https://codereview.chromium.org/365133005/diff/40001/chrome/app/client_util.cc
File chrome/app/client_util.cc (right):

https://codereview.chromium.org/365133005/diff/40001/chrome/app/client_util.c...
chrome/app/client_util.cc:79:
GoogleUpdateSettings::RetrieveMetricsID(&metrics_id);
Optional nit: IMO "Get" and "Retrieve" are synonyms, but "Get" is shorter and
more commonly used.  Hence, I'd keep the previous name.

https://codereview.chromium.org/365133005/diff/40001/chrome/common/child_proc...
File chrome/common/child_process_logging_win.cc (right):

https://codereview.chromium.org/365133005/diff/40001/chrome/common/child_proc...
chrome/common/child_process_logging_win.cc:72: std::string client_id;
nit: Should the variable name be updated here to include "guid" in the name?

https://codereview.chromium.org/365133005/diff/40001/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (left):

https://codereview.chromium.org/365133005/diff/40001/chrome/common/crash_keys...
chrome/common/crash_keys.cc:236: GoogleUpdateSettings::SetMetricsId(guid);
Why is this line safe to remove?  Are you sure that it's not needed via the call
from chrome_breakpad_client.cc?

(I realize that this is the main point of the CL, but I'm not super familiar
with the code, so I don't know the exact reasons why this was here.)

https://codereview.chromium.org/365133005/diff/40001/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/365133005/diff/40001/chrome/common/crash_keys...
chrome/common/crash_keys.cc:229: // Remove all instance of '-' char from the
GUID. So BCD-WXY becomes BCDWXY.
nit: "instance" -> "instances"

[grt (UTC plus 2), 2014-07-08 14:18:43]

cl description nits. i humbly suggest the following which makes the first line
more succinct (without losing important info, i think), resolves it's -> its and
its -> it's confusion in the second line, and makes a call -> calls swap in the
third.

Refactor SetClientID such that metrics rather than crash backs up the client id
in Google Update settings.

Consequentially, the backed up client_id now keeps its dashes and crash_keys
strips them at runtime rather than when backing it up
(https://codereview.chromium.org/372473004/ will add support for stripped
client_id backups for some time).

Also rename a lot of methods involved in setting the client id; having all of
them named "SetClientID" makes this series of calls very hard to follow!

[grt (UTC plus 2), 2014-07-08 14:34:57]

On 2014/07/08 14:18:43, grt wrote:
> Consequentially, the backed up client_id now keeps its dashes and crash_keys
> strips them at runtime rather than when backing it up
> (https://codereview.chromium.org/372473004/ will add support for stripped
> client_id backups for some time).

On further thought, I don't like having rietveld issue links in CL descriptions
since I'm not aware of there being any guarantee that they act like permalinks.
The crbug issue (which is permanent, to my knowledge) will contain the history
of related changes, so I don't know that there's a need for much beyond a
description of what this particular change accomplishes.

[grt (UTC plus 2), 2014-07-08 14:39:40]

Based on a high-level look at the change, I have some general naming commentary:
- I think using new verbs to disambiguate makes the code harder rather than
easier to understand. If two interfaces have the same SetFoo method, consider
SetBarFoo and SetBazFoo where Bar and Baz are specific to the interface (e.g.,
SetCrashClientId).
- Consider Load/Store for the registry functions rather than Retrieve/Save.

I haven't read through the change in detail, but would be happy to do so if
you'd like more eyes.

Cheers

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

https://codereview.chromium.org/365133005/diff/40001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.h:91: static bool
RetrieveMetricsID(std::string* metrics_id);
Why is this called "MetricsID" here and "ClientID" elsewhere? Are they not the
same thing?

[gab, 2014-07-08 17:46:02]

On 2014/07/08 14:34:57, grt wrote:
> On 2014/07/08 14:18:43, grt wrote:
> > Consequentially, the backed up client_id now keeps its dashes and crash_keys
> > strips them at runtime rather than when backing it up
> > (https://codereview.chromium.org/372473004/ will add support for stripped
> > client_id backups for some time).
> 
> On further thought, I don't like having rietveld issue links in CL
descriptions
> since I'm not aware of there being any guarantee that they act like
permalinks.
> The crbug issue (which is permanent, to my knowledge) will contain the history
> of related changes, so I don't know that there's a need for much beyond a
> description of what this particular change accomplishes.

I disagree, links between CLs help a lot at review time (providing extra context
now and later). 
I have also never stumbled upon a codereview link that was not longer valid (and
I've seen very old ones) -- the commits on crrev.com in fact link back to the CL
on codereview.chromium.org.

Thanks for the better CL description though :-).

I'll also try to get better method names under way.

[grt (UTC plus 2), 2014-07-08 18:32:58]

On 2014/07/08 17:46:02, gab wrote:
> On 2014/07/08 14:34:57, grt wrote:
> > On 2014/07/08 14:18:43, grt wrote:
> > > Consequentially, the backed up client_id now keeps its dashes and
crash_keys
> > > strips them at runtime rather than when backing it up
> > > (https://codereview.chromium.org/372473004/ will add support for stripped
> > > client_id backups for some time).
> > 
> > On further thought, I don't like having rietveld issue links in CL
> descriptions
> > since I'm not aware of there being any guarantee that they act like
> permalinks.
> > The crbug issue (which is permanent, to my knowledge) will contain the
history
> > of related changes, so I don't know that there's a need for much beyond a
> > description of what this particular change accomplishes.
> 
> I disagree, links between CLs help a lot at review time (providing extra
context
> now and later). 

Agreed. The code review messages are a better place for this at-review-time info
than the CL description itself.

> I have also never stumbled upon a codereview link that was not longer valid
(and
> I've seen very old ones) -- the commits on http://crrev.com in fact link back
to the CL
> on http://codereview.chromium.org.

"It worked in the past" isn't the same as "it will work in the future." I
contend that anyone doing forensics in changelist history where they truly need
this information will find the timeline expressed in the crbug issue most
useful. Adding links to related code reviews in the CL description adds
overhead: it is one more thing to keep up to date if the related change is split
up, handed off to another developer, etc. And if these things happen after this
change lands, it's not possible to update the links in the CL description. Yes,
you could add a note to that other issue directing anyone to the new real issue,
but this again adds more book-keeping. The crbug issue will always have the
truth.

[gab, 2014-07-08 18:41:32]

On 2014/07/08 18:32:58, grt wrote:
> On 2014/07/08 17:46:02, gab wrote:
> > On 2014/07/08 14:34:57, grt wrote:
> > > On 2014/07/08 14:18:43, grt wrote:
> > > > Consequentially, the backed up client_id now keeps its dashes and
> crash_keys
> > > > strips them at runtime rather than when backing it up
> > > > (https://codereview.chromium.org/372473004/ will add support for
stripped
> > > > client_id backups for some time).
> > > 
> > > On further thought, I don't like having rietveld issue links in CL
> > descriptions
> > > since I'm not aware of there being any guarantee that they act like
> > permalinks.
> > > The crbug issue (which is permanent, to my knowledge) will contain the
> history
> > > of related changes, so I don't know that there's a need for much beyond a
> > > description of what this particular change accomplishes.
> > 
> > I disagree, links between CLs help a lot at review time (providing extra
> context
> > now and later). 
> 
> Agreed. The code review messages are a better place for this at-review-time
info
> than the CL description itself.


I used to do that, but I've switched to putting everything a reviewer should
know in the CL description. Otherwise if I mention important details in the
first request for review, when I later add another reviewer I either have to
repeat that or assume he'll look at review history...

Also, people looking at old CLs only read CL description, rarely do they look at
review history.


> 
> > I have also never stumbled upon a codereview link that was not longer valid
> (and
> > I've seen very old ones) -- the commits on http://crrev.com in fact link
back
> to the CL
> > on http://codereview.chromium.org.
> 
> "It worked in the past" isn't the same as "it will work in the future."

I think this would be a serious breakage to the Chromium workflow.

> I contend that anyone doing forensics in changelist history where they truly
need
> this information will find the timeline expressed in the crbug issue most
> useful. Adding links to related code reviews in the CL description adds
> overhead: it is one more thing to keep up to date if the related change is
split
> up, handed off to another developer, etc. And if these things happen after
this
> change lands, it's not possible to update the links in the CL description.
Yes,
> you could add a note to that other issue directing anyone to the new real
issue,
> but this again adds more book-keeping. The crbug issue will always have the
> truth.

You're right that there is is *some* pitfalls, but I IMO the gains outweigh
those and I will continue this practice.

This also doesn't prevent someone to later follow the timeline in crbug as an
alternative either.

[gab, 2014-07-08 18:42:23]

Renamed many things as suggested by Ilya/grt.

PTAL.

Thanks!
Gab

https://codereview.chromium.org/365133005/diff/40001/chrome/app/chrome_breakp...
File chrome/app/chrome_breakpad_client.cc (right):

https://codereview.chromium.org/365133005/diff/40001/chrome/app/chrome_breakp...
chrome/app/chrome_breakpad_client.cc:83: void
ChromeBreakpadClient::InitClientID(const std::string& client_id) {
On 2014/07/08 03:21:38, Ilya Sherman wrote:
> nit: Should the variable be renamed to include "guid" in the name as well?

Done.

https://codereview.chromium.org/365133005/diff/40001/chrome/app/client_util.cc
File chrome/app/client_util.cc (right):

https://codereview.chromium.org/365133005/diff/40001/chrome/app/client_util.c...
chrome/app/client_util.cc:79:
GoogleUpdateSettings::RetrieveMetricsID(&metrics_id);
On 2014/07/08 03:21:38, Ilya Sherman wrote:
> Optional nit: IMO "Get" and "Retrieve" are synonyms, but "Get" is shorter and
> more commonly used.  Hence, I'd keep the previous name.

Went with Load/Store as suggested by grt @
https://codereview.chromium.org/372473004/diff/80001/components/metrics/metri...

I find this makes it more explicit that it's external storage, not memory
storage.

https://codereview.chromium.org/365133005/diff/40001/chrome/common/child_proc...
File chrome/common/child_process_logging_win.cc (right):

https://codereview.chromium.org/365133005/diff/40001/chrome/common/child_proc...
chrome/common/child_process_logging_win.cc:72: std::string client_id;
On 2014/07/08 03:21:38, Ilya Sherman wrote:
> nit: Should the variable name be updated here to include "guid" in the name?

Done.

https://codereview.chromium.org/365133005/diff/40001/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (left):

https://codereview.chromium.org/365133005/diff/40001/chrome/common/crash_keys...
chrome/common/crash_keys.cc:236: GoogleUpdateSettings::SetMetricsId(guid);
On 2014/07/08 03:21:38, Ilya Sherman wrote:
> Why is this line safe to remove?  Are you sure that it's not needed via the
call
> from chrome_breakpad_client.cc?
> 
> (I realize that this is the main point of the CL, but I'm not super familiar
> with the code, so I don't know the exact reasons why this was here.)

No it's not needed from the other call and yes this code is confusing (hence why
I'm trying to clean it up a bit first before putting another CL on top of it!).

The other call is only invoked from breakpad_linux.cc [1] which takes the value
from the command-line which was previously set using the value in
GoogleUpdateSettings [2] (so as long as that's set it doesn't matter where it
was set from...).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/components/breakpa...
(breakpad_mac.mm does the same thing)
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

https://codereview.chromium.org/365133005/diff/40001/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/365133005/diff/40001/chrome/common/crash_keys...
chrome/common/crash_keys.cc:229: // Remove all instance of '-' char from the
GUID. So BCD-WXY becomes BCDWXY.
On 2014/07/08 03:21:38, Ilya Sherman wrote:
> nit: "instance" -> "instances"

Done.

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

https://codereview.chromium.org/365133005/diff/40001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.h:91: static bool
RetrieveMetricsID(std::string* metrics_id);
On 2014/07/08 14:39:40, grt wrote:
> Why is this called "MetricsID" here and "ClientID" elsewhere? Are they not the
> same thing?

Went with "ClientId" everywhere (or do you think it should be "ClientID"? the
code was inconsistent with this too)

[Ilya Sherman, 2014-07-08 23:41:37]

Thanks, LGTM other than the fact that it looks like your other precursor CL bled
into this one.

https://codereview.chromium.org/365133005/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/365133005/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:150:
registry->RegisterInt64Pref(metrics::prefs::kInstallDate, 0);
Looks like your other change got mixed into this one?

[gab, 2014-07-08 23:56:44]

https://codereview.chromium.org/365133005/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/365133005/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:150:
registry->RegisterInt64Pref(metrics::prefs::kInstallDate, 0);
On 2014/07/08 23:41:37, Ilya Sherman wrote:
> Looks like your other change got mixed into this one?

It's based on top of it locally, hence why you see it in the unchanged text of
the diff, but they can land in parallel (I think the code they touch doesn't
overlap enough to cause a conflict).

Or do you see something I'm still missing?

[Ilya Sherman, 2014-07-09 00:00:14]

https://codereview.chromium.org/365133005/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/365133005/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:150:
registry->RegisterInt64Pref(metrics::prefs::kInstallDate, 0);
On 2014/07/08 23:56:44, gab wrote:
> On 2014/07/08 23:41:37, Ilya Sherman wrote:
> > Looks like your other change got mixed into this one?
> 
> It's based on top of it locally, hence why you see it in the unchanged text of
> the diff, but they can land in parallel (I think the code they touch doesn't
> overlap enough to cause a conflict).
> 
> Or do you see something I'm still missing?

Sorry, I was looking at the side-by-side view, which was showing me lies.

[gab, 2014-07-09 00:02:57]

Thanks,

@grt: Please double-check GoogleUpdateSettings
@cpu: OWNERS for chrome/app and components/breakpad
@rsesek: OWNERS for chrome/common/crash_keys*

[gab, 2014-07-09 00:05:16]

On 2014/07/09 00:02:57, gab wrote:
> Thanks,
> 
> @grt: Please double-check GoogleUpdateSettings
> @cpu: OWNERS for chrome/app and components/breakpad

@cpu: Please also look at chrome/common/child_process_logging_win.cc despite not
being a specific owner for it (@thestig please stamp it for owners)

> @rsesek: OWNERS for chrome/common/crash_keys*

[Robert Sesek, 2014-07-09 00:47:58]

lgtm

[grt (UTC plus 2), 2014-07-09 12:59:59]

nice cleanup! lgtm with a few nits.

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

https://codereview.chromium.org/365133005/diff/120001/chrome/browser/google/g...
chrome/browser/google/google_update_settings_posix.cc:33: if
(!base::DirectoryExists(consent_file.DirName()))
i see that this was moved here from SetMetricsId, but why is it needed? does
ReadFileToString not return false if the dir isn't there?

https://codereview.chromium.org/365133005/diff/120001/chrome/common/child_pro...
File chrome/common/child_process_logging_win.cc (right):

https://codereview.chromium.org/365133005/diff/120001/chrome/common/child_pro...
chrome/common/child_process_logging_win.cc:68: // This would be handled by
BreakpadClient::SetBreakpadClientID(), but because
SetBreakpadClientID -> SetBreakpadClientIdFromGUID

https://codereview.chromium.org/365133005/diff/120001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/365133005/diff/120001/chrome/common/crash_key...
chrome/common/crash_keys.cc:58: const char kClientID[] = "guid";
kClientId for consistency?

https://codereview.chromium.org/365133005/diff/120001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/365133005/diff/120001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.h:89: // Returns the metrics
client-id backed up in the registry. If none found,
nit: "client-id" doesn't seem to be used elsewhere. please use whichever of
"client id", "Client ID", "ClientID", etc... is most prevalent.

[gab, 2014-07-09 14:42:39]

Thanks, done.

> @cpu: OWNERS for chrome/app and components/breakpad
> @cpu: Please also look at chrome/common/child_process_logging_win.cc despite
not
being a specific owner for it (@thestig please stamp it for owners)

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

https://codereview.chromium.org/365133005/diff/120001/chrome/browser/google/g...
chrome/browser/google/google_update_settings_posix.cc:33: if
(!base::DirectoryExists(consent_file.DirName()))
On 2014/07/09 12:59:59, grt wrote:
> i see that this was moved here from SetMetricsId, but why is it needed? does
> ReadFileToString not return false if the dir isn't there?

I think it should (and the impl makes it look like it should also) but the
method comment in file_util.h isn't explicit about this.

I want this change to be a true no-op; so I'm just moving this to avoid having
to get DIR_USER_DATA in GoogleUpdateSettings::SetMetricsId() as that's overkill.

It can probably be removed altogether, but I'd rather not do it in this change.

https://codereview.chromium.org/365133005/diff/120001/chrome/common/child_pro...
File chrome/common/child_process_logging_win.cc (right):

https://codereview.chromium.org/365133005/diff/120001/chrome/common/child_pro...
chrome/common/child_process_logging_win.cc:68: // This would be handled by
BreakpadClient::SetBreakpadClientID(), but because
On 2014/07/09 12:59:59, grt wrote:
> SetBreakpadClientID -> SetBreakpadClientIdFromGUID

Done.

https://codereview.chromium.org/365133005/diff/120001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/365133005/diff/120001/chrome/common/crash_key...
chrome/common/crash_keys.cc:58: const char kClientID[] = "guid";
On 2014/07/09 12:59:59, grt wrote:
> kClientId for consistency?

Done.

https://codereview.chromium.org/365133005/diff/120001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/365133005/diff/120001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.h:89: // Returns the metrics
client-id backed up in the registry. If none found,
On 2014/07/09 12:59:59, grt wrote:
> nit: "client-id" doesn't seem to be used elsewhere. please use whichever of
> "client id", "Client ID", "ClientID", etc... is most prevalent.

Done.

[cpu_(ooo_6.6-7.5), 2014-07-09 17:56:02]

lgtm

[gab, 2014-07-09 17:58:16]

On 2014/07/09 17:56:02, cpu wrote:
> lgtm

Thanks, TBR thestig for chrome/common/child_process_logging_win.cc side-effects
(as it has no official owner above chrome/* :( ).

[gab, 2014-07-09 17:58:24]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-07-09 18:00:27]

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

[Lei Zhang, 2014-07-09 18:34:58]

child_process_logging_win.cc  lgtm

[commit-bot: I haz the power, 2014-07-09 18:53:29]

Change committed as 282093