Bug 233288 – Drop ServiceWorkerClientIdentifier / DocumentIdentifier and use ScriptExecutionContextIdentifier instead

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Drop ServiceWorkerClientIdentifier / DocumentIdentifier and use ScriptExecutionContextIdentifier instead.

We need to generic identifier type to identify script execution contexts globally. The proposal is to augment ScriptExecutionContextIdentifier with a process identifier to make it global, and then get rid of ServiceWorkerClientIdentifier & DocumentIdentifier.
This is useful for Web Lock API support as we need a clientId that is consistent between the Web Lock API and the service worker API (tested by imported/w3c/web-platform-tests/web-locks//clientids.tentative.https.html).

[Chris Dumez]

Created attachment 444596 [details]
Patch

[Darin Adler]

Comment on attachment 444596 [details]
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.

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 444596 [details]
> 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.

Yes, that's my bad. I updated identifier() to be identical to contextIdentifier() last minute and failed to update the rest of my patch accordingly.

[Chris Dumez]

Created attachment 444678 [details]
Patch

[Darin Adler]

Comment on attachment 444678 [details]
Patch

Generated tests failure looks real

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 444678 [details]
> Patch
> 
> Generated tests failure looks real

The new output looks like this though:
```
diff --git a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
index 630a865b13a6..52e3c7638390 100644
--- a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
+++ b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
@@ -127,6 +127,9 @@
 #endif
 #include "TestWithCVPixelBufferMessages.h"
 #if USE(AVFOUNDATION)
+#include <WebCore/CVUtilities.h>
+#endif
+#if USE(AVFOUNDATION)
 #include <wtf/RetainPtr.h>
 #endif
 
diff --git a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
index ee7defb81297..56496ddba5ea 100644
--- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
+++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
@@ -32,6 +32,9 @@
 #include "HandleMessage.h"
 #include "TestWithCVPixelBufferMessages.h"
 #if USE(AVFOUNDATION)
+#include <WebCore/CVUtilities.h>
+#endif
+#if USE(AVFOUNDATION)
 #include <wtf/RetainPtr.h>
 #endif
 
diff --git a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
index 1675bea59439..326500eb0a0e 100644
--- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
+++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
@@ -28,6 +28,9 @@
 #include "Connection.h"
 #include "MessageNames.h"
 #include "TestWithCVPixelBufferMessagesReplies.h"
+#if PLATFORM(COCOA)
+#include <WebCore/CVUtilities.h>
+#endif
 #include <wtf/Forward.h>
 #include <wtf/RetainPtr.h>
 #include <wtf/ThreadSafeRefCounted.h>
```

Not sure how it relates to my patch yet.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Darin Adler from comment #5)
> > Comment on attachment 444678 [details]
> > Patch
> > 
> > Generated tests failure looks real
> 
> The new output looks like this though:
> ```
> diff --git
> a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> index 630a865b13a6..52e3c7638390 100644
> --- a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> +++ b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> @@ -127,6 +127,9 @@
>  #endif
>  #include "TestWithCVPixelBufferMessages.h"
>  #if USE(AVFOUNDATION)
> +#include <WebCore/CVUtilities.h>
> +#endif
> +#if USE(AVFOUNDATION)
>  #include <wtf/RetainPtr.h>
>  #endif
>  
> diff --git
> a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.
> cpp
> b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> index ee7defb81297..56496ddba5ea 100644
> ---
> a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> +++
> b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> @@ -32,6 +32,9 @@
>  #include "HandleMessage.h"
>  #include "TestWithCVPixelBufferMessages.h"
>  #if USE(AVFOUNDATION)
> +#include <WebCore/CVUtilities.h>
> +#endif
> +#if USE(AVFOUNDATION)
>  #include <wtf/RetainPtr.h>
>  #endif
>  
> diff --git
> a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> index 1675bea59439..326500eb0a0e 100644
> --- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> +++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> @@ -28,6 +28,9 @@
>  #include "Connection.h"
>  #include "MessageNames.h"
>  #include "TestWithCVPixelBufferMessagesReplies.h"
> +#if PLATFORM(COCOA)
> +#include <WebCore/CVUtilities.h>
> +#endif
>  #include <wtf/Forward.h>
>  #include <wtf/RetainPtr.h>
>  #include <wtf/ThreadSafeRefCounted.h>
> ```
> 
> Not sure how it relates to my patch yet.

I think it was more likely caused by https://bugs.webkit.org/show_bug.cgi?id=231945.

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Darin Adler from comment #5)
> > > Comment on attachment 444678 [details]
> > > Patch
> > > 
> > > Generated tests failure looks real
> > 
> > The new output looks like this though:
> > ```
> > diff --git
> > a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> > b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> > index 630a865b13a6..52e3c7638390 100644
> > --- a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> > +++ b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> > @@ -127,6 +127,9 @@
> >  #endif
> >  #include "TestWithCVPixelBufferMessages.h"
> >  #if USE(AVFOUNDATION)
> > +#include <WebCore/CVUtilities.h>
> > +#endif
> > +#if USE(AVFOUNDATION)
> >  #include <wtf/RetainPtr.h>
> >  #endif
> >  
> > diff --git
> > a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.
> > cpp
> > b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> > index ee7defb81297..56496ddba5ea 100644
> > ---
> > a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> > +++
> > b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> > @@ -32,6 +32,9 @@
> >  #include "HandleMessage.h"
> >  #include "TestWithCVPixelBufferMessages.h"
> >  #if USE(AVFOUNDATION)
> > +#include <WebCore/CVUtilities.h>
> > +#endif
> > +#if USE(AVFOUNDATION)
> >  #include <wtf/RetainPtr.h>
> >  #endif
> >  
> > diff --git
> > a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> > b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> > index 1675bea59439..326500eb0a0e 100644
> > --- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> > +++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> > @@ -28,6 +28,9 @@
> >  #include "Connection.h"
> >  #include "MessageNames.h"
> >  #include "TestWithCVPixelBufferMessagesReplies.h"
> > +#if PLATFORM(COCOA)
> > +#include <WebCore/CVUtilities.h>
> > +#endif
> >  #include <wtf/Forward.h>
> >  #include <wtf/RetainPtr.h>
> >  #include <wtf/ThreadSafeRefCounted.h>
> > ```
> > 
> > Not sure how it relates to my patch yet.
> 
> I think it was more likely caused by
> https://bugs.webkit.org/show_bug.cgi?id=231945.

Youenn has a patch in-flight to do the rebaseline already.

[Darin Adler]

My mistake

[Darin Adler]

Comment on attachment 444678 [details]
Patch

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

Post landing feedback

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:29
> +template <typename> class ObjectIdentifier;

Ideally this should be done with Forward.h so we don’t have to write WTF prefixes. If it wasn’t for the WTF prefixes thing I guess we could do individual forward declarations like this. But would hate for Forward.h include costs to undercut the savings from using forward declarations to cut down number of headers included to speed up compiles. Maybe we should eventually abandon the “using WTF::String” thing and write the WTF prefixes everywhere.

Would like to nail down the idiom so we aren’t making different choices each time.

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:34
> +template <typename> class ProcessQualified;

I slightly prefer the formatting without the space after “template”.

[Chris Dumez]

Created attachment 444688 [details]
Patch

[EWS]

Committed r286012 (244405@main): <https://commits.webkit.org/244405@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444688 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85557507>