[webkit-reviews] review granted: [Bug 233288] Drop ServiceWorkerClientIdentifier / DocumentIdentifier and use ScriptExecutionContextIdentifier instead : [Attachment 444596] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 18 03:56:06 PST 2021


Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 233288: Drop ServiceWorkerClientIdentifier / DocumentIdentifier and use
ScriptExecutionContextIdentifier instead
https://bugs.webkit.org/show_bug.cgi?id=233288

Attachment 444596: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 444596
  --> https://bugs.webkit.org/attachment.cgi?id=444596
Patch

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

> Source/WebCore/dom/Document.cpp:8728
> +	  
m_serviceWorkerConnection->unregisterServiceWorkerClient(contextIdentifier());

This change is not needed and goes against a comment saying this function is
deprecated.

> Source/WebCore/dom/ScriptExecutionContext.cpp:120
> +    allScriptExecutionContextsMap().add(m_contextIdentifier,
const_cast<ScriptExecutionContext*>(this));

No const_cast here wanted or needed.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:774
> +	   request.setClientIdentifierIfNeeded(document->contextIdentifier());

Change not needed, as above. Unless I am missing something?

> Source/WebCore/platform/ProcessQualified.h:109
> +    encoder << m_object;

I’ve been wondering why we don’t write these kinds of functions as one-liners.

> Source/WebCore/platform/ProcessQualified.h:117
> +    decoder >> object;

We really need a better idiom for writing these structure decoders. I think we
could find a way to cut down on repetitive code somehow.

> Source/WebCore/platform/ProcessQualified.h:124
> +    return ProcessQualified { *object, *processIdentifier };

Could use two sets of braces instead of naming the type here.

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:28
> +#include "ProcessQualified.h"

Can this be a forward declaration instead of an include?

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:29
>  #include <wtf/ObjectIdentifier.h>

Wondering the same.

> Source/WebCore/testing/Internals.cpp:2762
> +    return
Document::allDocumentsMap().contains(ScriptExecutionContextIdentifier {
makeObjectIdentifier<ScriptExecutionContextIdentifierType>(documentIdentifier),
Process::identifier() });

Can this be written any more tersely without so many type names?

> Source/WebCore/workers/service/ServiceWorker.cpp:124
> +	   sourceIdentifier = context.contextIdentifier();

Why contextIdentifier instead of just identifier?

> Source/WebCore/workers/service/ServiceWorkerClientData.cpp:73
> +	   context.contextIdentifier(),

Why contextIdentifier instead of just identifier?

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:63
> +	   if (!identifier || !*identifier)

Not sure why we mix *identifier with identifier.value() here. Choose one I say.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:633
> +    return scriptExecutionContext()->contextIdentifier();

Why contextIdentifier instead of just identifier?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:177
> +    auto addResult =
m_clientsUsingRegistration.ensure(clientIdentifier.processIdentifier(), [] {

This could use add instead of ensure. An empty HashSet is just a null pointer,
near zero cost to construct.


More information about the webkit-reviews mailing list