Summary: | Drop ServiceWorkerClientIdentifier / DocumentIdentifier and use ScriptExecutionContextIdentifier instead | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, annulen, benjamin, cmarcelo, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, hta, japhet, jer.noble, kangil.han, philipj, ryuan.choi, sam, sergio, tommyw, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 232436, 233323 | ||||||||||
Attachments: |
|
Description
Chris Dumez
2021-11-17 15:57:15 PST
Created attachment 444596 [details]
Patch
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. (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. Created attachment 444678 [details]
Patch
Comment on attachment 444678 [details]
Patch
Generated tests failure looks real
(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. (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. (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. My mistake 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”. Created attachment 444688 [details]
Patch
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]. |