Bug 65157 – Simplify createCanonicalUUIDString implementation.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Justin Schuh]

COM calls are not allowed in the Chromium sandbox. Patch forthcoming.

[Justin Schuh]

Created attachment 102057 [details]
Patch

[Michael Nordman]

Comment on attachment 102057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102057&action=review

> Source/WebCore/platform/UUID.cpp:64
> +#elif (PLATFORM(CHROMIUM) || OS(DARWIN)) && !USE(CF)

Is there a reason not to use this UUID generator in all CHROMIUM cases? Just curious.

> Source/WebCore/platform/UUID.cpp:82
> +    builder.append("\n");

I'm surprised to see the trailing newline here. Do the other impls result in strings with a trailing newline?

> Source/WebCore/platform/UUID.cpp:115
>      ASSERT(canonicalUuidStr[uuidVersionIdentifierIndex] == uuidVersionRequired);

All of the other impls make this assertion...
ASSERT(canonicalUuidStr[uuidVersionIdentifierIndex] == uuidVersionRequired);
Does this difference matter?

[Justin Schuh]

Comment on attachment 102057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102057&action=review

>> Source/WebCore/platform/UUID.cpp:64
>> +#elif (PLATFORM(CHROMIUM) || OS(DARWIN)) && !USE(CF)
> 
> Is there a reason not to use this UUID generator in all CHROMIUM cases? Just curious.

I'm not sure and couldn't get a firm answer, so I chose to leave Mac Chrome the way it is.

>> Source/WebCore/platform/UUID.cpp:82
>> +    builder.append("\n");
> 
> I'm surprised to see the trailing newline here. Do the other impls result in strings with a trailing newline?

Good catch. I assume that's for compatibility with /proc/sys/kernel/random/uuid (which has a newline). It's not an issue because the UUID gets encoded; however I can macro around that to keep the Windows version consistent.

>> Source/WebCore/platform/UUID.cpp:115
>>      ASSERT(canonicalUuidStr[uuidVersionIdentifierIndex] == uuidVersionRequired);
> 
> All of the other impls make this assertion...
> ASSERT(canonicalUuidStr[uuidVersionIdentifierIndex] == uuidVersionRequired);
> Does this difference matter?

The code that manually generates the UUID explicitly sets this field, so there's no need to check for it.

[Justin Schuh]

Created attachment 102297 [details]
Patch

I talked to Adam and he said all the listed ports support OS_RANDOMNESS. So, he suggested just switching all of them over to a single implementation. (Also, I verified the newline was pointless, and actually clipped in the Linux implementation.)

[Justin Schuh]

Created attachment 102298 [details]
Patch

Should probably use a different title and changelog entry now.

[Adam Barth]

Comment on attachment 102298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102298&action=review

I've added some folks to the CC line to make sure they see this patch go by.  I can't think of any reason not to use the cryptographically random implementation now that we have it.

> Source/WebCore/platform/UUID.cpp:45
> +    WTF::cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(randomData), sizeof(randomData));

The WTF:: part isn't needed.

[Justin Schuh]

Committed r91963: <http://trac.webkit.org/changeset/91963>