Unify UUID/GUID generation code across base and blink

Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - returns mostly-correct Ver4 GUID string but in uppercase
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*

Committed: https://crrev.com/4ad1f556b877a4570c75349f41753cea587f4723
Cr-Commit-Position: refs/heads/master@{#390042}

[kinuko, 2016-04-21 14:06:55]

Description was changed from

==========
Unify UUID/GUID generation code

We have three different implementations in Chromium and they all return
slightly different UUID/GUID string.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't confirm RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

to

==========
Unify UUID/GUID generation code across base and blink

We have three different implementations in Chromium and they all return
slightly different UUID/GUID string.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't confirm RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

[kinuko, 2016-04-21 14:07:02]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 14:07:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906723003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906723003/1

[commit-bot: I haz the power, 2016-04-21 14:11:09]

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

[commit-bot: I haz the power, 2016-04-21 14:11:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2016-04-21 14:24:52]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 14:25:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906723003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906723003/20001

[kinuko, 2016-04-21 14:30:19]

Description was changed from

==========
Unify UUID/GUID generation code across base and blink

We have three different implementations in Chromium and they all return
slightly different UUID/GUID string.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't confirm RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

to

==========
Unify UUID/GUID generation code across base and blink

We have three different implementations in Chromium and they all return
slightly different UUID/GUID string.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't conform with
RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

[kinuko, 2016-04-21 14:30:25]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[kinuko, 2016-04-21 14:30:30]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-04-21 14:30:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906723003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906723003/40001

[commit-bot: I haz the power, 2016-04-21 15:06:01]

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

[commit-bot: I haz the power, 2016-04-21 15:06:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2016-04-21 15:25:53]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 15:26:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906723003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906723003/60001

[commit-bot: I haz the power, 2016-04-21 18:20:24]

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

[commit-bot: I haz the power, 2016-04-21 18:20:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kinuko, 2016-04-22 03:37:05]

kinuko@chromium.org changed reviewers:
+ johnme@chromium.org, mark@chromium.org

[kinuko, 2016-04-22 03:37:42]

Description was changed from

==========
Unify UUID/GUID generation code across base and blink

We have three different implementations in Chromium and they all return
slightly different UUID/GUID string.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't conform with
RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

to

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't conform with
RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

[kinuko, 2016-04-22 03:38:17]

Description was changed from

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't conform with
RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

to

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't conform with
RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

[kinuko, 2016-04-22 03:40:42]

Could you review?

mark@ for //base changes
johnme@ for //chrome/browser/push_messaging change (it's a bit unfortunate)

I think they should be unified but we've been using/accepting different formats
for years and this might break things.

[johnme, 2016-04-22 12:02:27]

push_messaging lgtm (with nit) - thanks!

https://codereview.chromium.org/1906723003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (right):

https://codereview.chromium.org/1906723003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:69: // Always use
the uppercased GUID to to make the case mangling handling
Nit: Keeping these uppercase makes sense, but the primary reason is just for
consistency, so we don't end up with a mix of cases on the server. The following
comment might be clearer:

  // Use uppercase GUID for consistency with GUIDs Push has already sent to GCM.
  // Also allows detecting case mangling; see code commented "crbug.com/461867".

[Mark Mentovai, 2016-04-22 13:01:46]

LGTM in base.

In the CL description, you said that CoCreateGuid doesn’t conform to the RFC. Is
that really the case?

https://codereview.chromium.org/1906723003/diff/60001/base/guid.cc
File base/guid.cc (right):

https://codereview.chromium.org/1906723003/diff/60001/base/guid.cc#newcode71
base/guid.cc:71: return StringPrintf("%08x-%04x-%04x-%01X%03x-%012llx",
There’s a capital X in here that should be lowercase.

https://codereview.chromium.org/1906723003/diff/60001/base/guid.cc#newcode75
base/guid.cc:75: static_cast<unsigned int>(bytes[1] >> 60),
I don’t see the reason to have bytes[1] >> 60 and (bytes[1] >> 48) & 0x0fff be
separated.

[kinuko, 2016-04-25 05:59:39]

Description was changed from

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service, which doesn't conform with
RFC
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

to

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

[kinuko, 2016-04-25 14:14:01]

Description was changed from

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - using Windows system service
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

to

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - returns mostly-correct Ver4 GUID string but in uppercase
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

[kinuko, 2016-04-25 14:22:37]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 14:22:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906723003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906723003/100001

[kinuko, 2016-04-25 14:24:37]

Thanks!

> In the CL description, you said that CoCreateGuid doesn’t conform to the RFC.
Is
> that really the case?

To be more precise CoCreateGuid looks ok but StringFromGUID2 seems to return
upper-case string.

https://codereview.chromium.org/1906723003/diff/60001/base/guid.cc
File base/guid.cc (right):

https://codereview.chromium.org/1906723003/diff/60001/base/guid.cc#newcode71
base/guid.cc:71: return StringPrintf("%08x-%04x-%04x-%01X%03x-%012llx",
On 2016/04/22 13:01:46, Mark Mentovai wrote:
> There’s a capital X in here that should be lowercase.

Done.

https://codereview.chromium.org/1906723003/diff/60001/base/guid.cc#newcode75
base/guid.cc:75: static_cast<unsigned int>(bytes[1] >> 60),
On 2016/04/22 13:01:46, Mark Mentovai wrote:
> I don’t see the reason to have bytes[1] >> 60 and (bytes[1] >> 48) & 0x0fff be
> separated.

Oops, you're right, done.

https://codereview.chromium.org/1906723003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (right):

https://codereview.chromium.org/1906723003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:69: // Always use
the uppercased GUID to to make the case mangling handling
On 2016/04/22 12:02:27, johnme wrote:
> Nit: Keeping these uppercase makes sense, but the primary reason is just for
> consistency, so we don't end up with a mix of cases on the server. The
following
> comment might be clearer:
> 
>   // Use uppercase GUID for consistency with GUIDs Push has already sent to
GCM.
>   // Also allows detecting case mangling; see code commented
"crbug.com/461867".

Done.

[commit-bot: I haz the power, 2016-04-25 14:29:30]

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

[commit-bot: I haz the power, 2016-04-25 14:29:31]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[kinuko, 2016-04-26 05:54:39]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-26 05:54:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906723003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906723003/120001

[commit-bot: I haz the power, 2016-04-26 07:44:51]

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

[commit-bot: I haz the power, 2016-04-26 07:44:52]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-04-26 12:57:10]

kinuko@chromium.org changed reviewers:
+ tkent@chromium.org

[kinuko, 2016-04-26 12:58:55]

Kent-san, can you review platform changes?

[tkent, 2016-04-26 13:24:33]

tkent@chromium.org changed reviewers:
+ esprehn@chromium.org

[tkent, 2016-04-26 13:24:34]

New base dependency should be reviewed by esprehn.

https://codereview.chromium.org/1906723003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UUID.cpp (right):

https://codereview.chromium.org/1906723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UUID.cpp:20: ASSERT(uuid.is8Bit());
ASSERT -> DCHECK

[kinuko, 2016-04-27 08:47:06]

esprehn@- could you review platform/DEPS change (and others files you care
about)?

(We could possibly move these files into some subdirectories to limit the DEPS
change, while I don't have a good idea about where this should live)

https://codereview.chromium.org/1906723003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UUID.cpp (right):

https://codereview.chromium.org/1906723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UUID.cpp:20: ASSERT(uuid.is8Bit());
On 2016/04/26 13:24:34, tkent wrote:
> ASSERT -> DCHECK

Actually we don't need this ASSERT, so I removed it.

[kinuko, 2016-04-27 08:47:21]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 08:47:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906723003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906723003/160001

[esprehn, 2016-04-27 08:55:49]

lgtm

[esprehn, 2016-04-27 09:00:59]

https://codereview.chromium.org/1906723003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UUID.cpp (right):

https://codereview.chromium.org/1906723003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UUID.cpp:23: return
base::IsValidGUIDOutputString(utf8.asStringPiece());
Can this take a StringPiece so we can avoid the copy?

[kinuko, 2016-04-27 09:21:20]

Thanks!

https://codereview.chromium.org/1906723003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UUID.cpp (right):

https://codereview.chromium.org/1906723003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UUID.cpp:23: return
base::IsValidGUIDOutputString(utf8.asStringPiece());
On 2016/04/27 09:00:59, esprehn wrote:
> Can this take a StringPiece so we can avoid the copy?

It does now =)

[kinuko, 2016-04-27 10:24:29]

The CQ bit was unchecked by kinuko@chromium.org

[kinuko, 2016-04-27 10:24:40]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2016-04-27 10:24:40]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1906723003/#ps160001
(title: "remove ASSERT, use StringPiece")

[commit-bot: I haz the power, 2016-04-27 10:24:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906723003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906723003/160001

[commit-bot: I haz the power, 2016-04-27 11:00:33]

Description was changed from

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - returns mostly-correct Ver4 GUID string but in uppercase
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

to

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - returns mostly-correct Ver4 GUID string but in uppercase
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

[commit-bot: I haz the power, 2016-04-27 11:00:35]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-04-27 11:02:03]

Description was changed from

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - returns mostly-correct Ver4 GUID string but in uppercase
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*
==========

to

==========
Unify UUID/GUID generation code across base and blink

We have at least three different UUID/GUID implementations in Chromium
and they all use slightly different UUID/GUID format criteria.

From code health (and consistent behavior) pov it'd be nice
to unify them.

- base/guid_posix.cc - returns mostly-correct Ver4 GUID string but in uppercase
- base/guid_win.cc - returns mostly-correct Ver4 GUID string but in uppercase
- third_party/WebKit/Source/platform/UUID.cpp - returns correct Ver4 GUID

This patch tries to unify them into one location in base/guid.cc.

BUG=561879
TEST=base_unittests:GUID*
TEST=blink_platform_unittests:UUID*

Committed: https://crrev.com/4ad1f556b877a4570c75349f41753cea587f4723
Cr-Commit-Position: refs/heads/master@{#390042}
==========

[commit-bot: I haz the power, 2016-04-27 11:02:05]

Patchset 8 (id:??) landed as
https://crrev.com/4ad1f556b877a4570c75349f41753cea587f4723
Cr-Commit-Position: refs/heads/master@{#390042}