[webkit-reviews] review granted: [Bug 235070] Make ServiceWorkerClient.id a UUID instead of a string derived from a ScriptExecutionContextIdentifier : [Attachment 449161] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 14 09:24:21 PST 2022


Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 235070: Make ServiceWorkerClient.id a UUID instead of a string derived from
a ScriptExecutionContextIdentifier
https://bugs.webkit.org/show_bug.cgi?id=235070

Attachment 449161: Patch

https://bugs.webkit.org/attachment.cgi?id=449161&action=review




--- Comment #17 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 449161
  --> https://bugs.webkit.org/attachment.cgi?id=449161
Patch

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

r=me with some simplification ideas. I think the patch adds a bit more
complexity than we really need.

> Source/WebCore/platform/ProcessQualified.h:72
> +    const T& objectHash() const

Shouldn't be needed, see comment below.

> Source/WebCore/platform/ProcessQualified.h:134
> +    add(hasher, processQualified.objectHash());

I don't think we should need to add this objectHash(), we should probably just
add this to UUID.h:
inline void add(Hasher& hasher, const UUID& uuid)
{
    add(hasher, uuid());
}

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:70
> +    unsigned objectHash() const

Shouldn't be needed, see comment above.

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:97
> +    String toDebugString() const { return
makeString(m_processIdentifier.toUInt64(), '-', m_object.toString()); }

I personally don't think it is worth the extra mental complexity to have 2
different kinds of strings, just to print the process identifier in some cases.
I think having only a single string (that is the "visible" string) would be
better.

> Source/WebCore/testing/Internals.idl:936
> +    DOMString serviceWorkerClientInternalIdentifier(Document document);

Why do we need to expose "internal" identifiers to the "Web" (I understand it
is just tests). Why cannot tests rely on regular client identifiers (UUIDs)?


More information about the webkit-reviews mailing list