[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