Moving GUID generation from base to chrome/browser/guid*

Moving GUID generation from base to chrome/browser/guid*

Moves GUID generation into chrome/browser/guid*.  GUID generation is used only within chrome/browser.  So am moving it there.

BUG=58813
TEST=GUIDTest.GUIDGeneratesAllZeroes, GUIDTest.GUIDGeneratesCorrectly, GUIDTest.GUIDCorrectlyFormatted, MetricsServiceTest.ClientIdCorrectlyFormatted

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

[brettw, 2010-10-14 04:54:41]

LGTM, thanks a lot for following up!

[Paweł Hajdan Jr., 2010-10-14 07:05:57]

Drive-by with test comments.

http://codereview.chromium.org/3800003/diff/1/8
File chrome/browser/guid_unittest.cc (right):

http://codereview.chromium.org/3800003/diff/1/8#newcode14
chrome/browser/guid_unittest.cc:14: extern std::string
RandomBytesToGUIDString(const uint64 bytes[2]);
Please don't do that. Find some other way to expose that function for tests.

http://codereview.chromium.org/3800003/diff/1/9
File chrome/browser/guid_win.cc (right):

http://codereview.chromium.org/3800003/diff/1/9#newcode24
chrome/browser/guid_win.cc:24: DCHECK(SUCCEEDED(guid_result));
Can we change the function signature to bool GenerateGUID(std::string* guid) so
we can return false here instead of DCHECK?

[Mark Mentovai, 2010-10-14 11:09:27]

LGTM

[dhollowa, 2010-10-14 14:37:27]

http://codereview.chromium.org/3800003/diff/1/8
File chrome/browser/guid_unittest.cc (right):

http://codereview.chromium.org/3800003/diff/1/8#newcode14
chrome/browser/guid_unittest.cc:14: extern std::string
RandomBytesToGUIDString(const uint64 bytes[2]);
On 2010/10/14 07:05:57, Paweł Hajdan Jr. wrote:
> Please don't do that. Find some other way to expose that function for tests.

What do you propose?

http://codereview.chromium.org/3800003/diff/1/9
File chrome/browser/guid_win.cc (right):

http://codereview.chromium.org/3800003/diff/1/9#newcode24
chrome/browser/guid_win.cc:24: DCHECK(SUCCEEDED(guid_result));
On 2010/10/14 07:05:57, Paweł Hajdan Jr. wrote:
> Can we change the function signature to bool GenerateGUID(std::string* guid)
so
> we can return false here instead of DCHECK?

I'd prefer not.  All the caller woud do is DCHECK the result anyway.

[brettw, 2010-10-14 15:05:43]

http://codereview.chromium.org/3800003/diff/1/8
File chrome/browser/guid_unittest.cc (right):

http://codereview.chromium.org/3800003/diff/1/8#newcode14
chrome/browser/guid_unittest.cc:14: extern std::string
RandomBytesToGUIDString(const uint64 bytes[2]);
I think it's fine to just add this as a regular function in the guid.h header
file you made. Maybe call it RandomDataTo... since I found it weird that it's
called "Bytes" but doesn't take byte values as input.

[dhollowa, 2010-10-14 16:15:06]

http://codereview.chromium.org/3800003/diff/1/8
File chrome/browser/guid_unittest.cc (right):

http://codereview.chromium.org/3800003/diff/1/8#newcode14
chrome/browser/guid_unittest.cc:14: extern std::string
RandomBytesToGUIDString(const uint64 bytes[2]);
On 2010/10/14 15:05:43, brettw wrote:
> I think it's fine to just add this as a regular function in the guid.h header
> file you made. Maybe call it RandomDataTo... since I found it weird that it's
> called "Bytes" but doesn't take byte values as input.

Done.

[brettw, 2010-10-14 16:24:15]

LGTM

http://codereview.chromium.org/3800003/diff/10001/11005
File chrome/browser/guid.h (right):

http://codereview.chromium.org/3800003/diff/10001/11005#newcode18
chrome/browser/guid.h:18: #if defined(OS_POSIX)
You'll need to include "build/build_config.h" in this file to make sure this
ifdef works.

http://codereview.chromium.org/3800003/diff/10001/11008
File chrome/browser/guid_win.cc (right):

http://codereview.chromium.org/3800003/diff/10001/11008#newcode24
chrome/browser/guid_win.cc:24: DCHECK(SUCCEEDED(guid_result));
Re: Pawel's comment about returning false. I prefer to just have a string
returned, but I agree it's nagging on me a bit that this function could fail in
mysterious cases we don't understand. I'd say we should check this and return an
empty string in this case, and document that it will return an empty string on
failure in the header.

[dhollowa, 2010-10-14 16:48:03]

http://codereview.chromium.org/3800003/diff/10001/11005
File chrome/browser/guid.h (right):

http://codereview.chromium.org/3800003/diff/10001/11005#newcode18
chrome/browser/guid.h:18: #if defined(OS_POSIX)
On 2010/10/14 16:24:15, brettw wrote:
> You'll need to include "build/build_config.h" in this file to make sure this
> ifdef works.

Done.

http://codereview.chromium.org/3800003/diff/10001/11008
File chrome/browser/guid_win.cc (right):

http://codereview.chromium.org/3800003/diff/10001/11008#newcode24
chrome/browser/guid_win.cc:24: DCHECK(SUCCEEDED(guid_result));
On 2010/10/14 16:24:15, brettw wrote:
> Re: Pawel's comment about returning false. I prefer to just have a string
> returned, but I agree it's nagging on me a bit that this function could fail
in
> mysterious cases we don't understand. I'd say we should check this and return
an
> empty string in this case, and document that it will return an empty string on
> failure in the header.

Agreed.  Done.

[Paweł Hajdan Jr., 2010-10-14 17:21:02]

Code I commented in the drive-by LGTM.

http://codereview.chromium.org/3800003/diff/8002/25005
File chrome/browser/guid.h (right):

http://codereview.chromium.org/3800003/diff/8002/25005#newcode20
chrome/browser/guid.h:20: #if defined(OS_POSIX)
nit: I think you could also add && defined(UNIT_TEST). This way the function
would only be exposed for the test code. Up to you.

[dhollowa, 2010-10-14 17:58:24]

http://codereview.chromium.org/3800003/diff/8002/25005
File chrome/browser/guid.h (right):

http://codereview.chromium.org/3800003/diff/8002/25005#newcode20
chrome/browser/guid.h:20: #if defined(OS_POSIX)
On 2010/10/14 17:21:02, Paweł Hajdan Jr. wrote:
> nit: I think you could also add && defined(UNIT_TEST). This way the function
> would only be exposed for the test code. Up to you.

This is tantalizing, but gets ugly because the declaration needs to exist for
non-unit test code as well.  I think the current way is cleaner.