https://w3c.github.io/ServiceWorker/#clients-get
Created attachment 327930 [details] Patch
Created attachment 327941 [details] Patch
Created attachment 327988 [details] Patch
Created attachment 327989 [details] Patch
Created attachment 327990 [details] Patch
Created attachment 328004 [details] Patch
Comment on attachment 328004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328004&action=review > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:61 > + if (!identifier) We should probably check that the identifier is not 0 either. > Source/WebCore/workers/service/ServiceWorkerClients.cpp:32 > +#include "JSServiceWorkerWindowClient.h" I always find it fishy when we include bindings files in WebCore implementation objects. > Source/WebCore/workers/service/ServiceWorkerClients.cpp:73 > +static inline ExceptionOr<std::optional<ServiceWorkerClientData>> isolatedExceptionOrClientData(const ExceptionOr<std::optional<ServiceWorkerClientData>>& value) I think the logic belongs in ExceptionOr.h. Something like: #include <wtf/CrossThreadCopier.h> template<typename T> struct CrossThreadCopierBase<false, false, ExceptionOr<T>> { typedef ExceptionOr<T> Type; static Type copy(const Type& source) { if (value.hasException()) return value.exception().isolatedCopy(); return CrossThreadCopier<T>::copy(value.releaseReturnValue()); } }; Then you can use crossThreadCopy() with your type. > Source/WebCore/workers/service/ServiceWorkerClients.h:43 > +class ServiceWorkerClients : public ThreadSafeRefCounted<ServiceWorkerClients>, public ActiveDOMObject { Why is this needed? Do we really ref/deref ServiceWorkerClients from the main thread? It does not look to me that you are (or that you should). > Source/WebCore/workers/service/server/SWServer.cpp:308 > +std::optional<ServiceWorkerClientData> SWServer::clientFromId(const ClientOrigin& origin, ServiceWorkerClientIdentifier clientIdentifier) ById? > Source/WebCore/workers/service/server/SWServer.cpp:310 > + auto clients = m_clients.get(origin); Why are you copying the vector here? Shouldn't it be auto&? > Source/WebCore/workers/service/server/SWServerWorker.h:84 > + const ClientOrigin& origin() const; I think it weird that a method called SWServerWorker::origin() returns a ClientOrigin type. This is nothing to do with clients per say.
Comment on attachment 328004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328004&action=review > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:54 > +inline std::optional<ServiceWorkerClientIdentifier> ServiceWorkerClientIdentifier::fromString(const String& string) Should probably take a StringView in parameter. > Source/WebCore/workers/service/ServiceWorkerClients.cpp:53 > +void ServiceWorkerClients::getCompleted(ServiceWorkerClientIdentifier identifier, ExceptionOr<std::optional<ServiceWorkerClientData>>&& clientData, DeferredPromise& promiseReference) I find "getCompleted" naming a bit confusing. something like didFinishGetRequest() would be much clearer to me.
(In reply to Chris Dumez from comment #7) > Comment on attachment 328004 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328004&action=review > > > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:61 > > + if (!identifier) > > We should probably check that the identifier is not 0 either. I am not sure, since the default identifier is 0-0. I can add an additional check in ServiceWorkerClients to check for that and return early but I am not sure this is worth it. > > Source/WebCore/workers/service/ServiceWorkerClients.cpp:32 > > +#include "JSServiceWorkerWindowClient.h" > > I always find it fishy when we include bindings files in WebCore > implementation objects. We need toJS() for resolving promises. > > Source/WebCore/workers/service/ServiceWorkerClients.cpp:73 > > +static inline ExceptionOr<std::optional<ServiceWorkerClientData>> isolatedExceptionOrClientData(const ExceptionOr<std::optional<ServiceWorkerClientData>>& value) > > I think the logic belongs in ExceptionOr.h. Something like: > #include <wtf/CrossThreadCopier.h> > template<typename T> struct CrossThreadCopierBase<false, false, > ExceptionOr<T>> { > typedef ExceptionOr<T> Type; > static Type copy(const Type& source) > { > if (value.hasException()) > return value.exception().isolatedCopy(); > return CrossThreadCopier<T>::copy(value.releaseReturnValue()); > } > }; > > Then you can use crossThreadCopy() with your type. Will give it a try and will see whether optional is making things more difficult or not. > > Source/WebCore/workers/service/ServiceWorkerClients.h:43 > > +class ServiceWorkerClients : public ThreadSafeRefCounted<ServiceWorkerClients>, public ActiveDOMObject { > > Why is this needed? Do we really ref/deref ServiceWorkerClients from the > main thread? It does not look to me that you are (or that you should). I will rework the patch and remove this change by eliminating any possibility for it to be derefed on the main thread. > > Source/WebCore/workers/service/server/SWServer.cpp:308 > > +std::optional<ServiceWorkerClientData> SWServer::clientFromId(const ClientOrigin& origin, ServiceWorkerClientIdentifier clientIdentifier) > > ById? I prefer FromId since a query is being done. > > Source/WebCore/workers/service/server/SWServer.cpp:310 > > + auto clients = m_clients.get(origin); > > Why are you copying the vector here? Shouldn't it be auto&? Will change it. > > Source/WebCore/workers/service/server/SWServerWorker.h:84 > > + const ClientOrigin& origin() const; > > I think it weird that a method called SWServerWorker::origin() returns a > ClientOrigin type. This is nothing to do with clients per say. SWServerWorker origin is the origin of the client it was spawned from. Maybe ClientOrigin is not a very good name though since it has more to do with ITP than service worker clients.
Comment on attachment 328004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328004&action=review >>> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:61 >>> + if (!identifier) >> >> We should probably check that the identifier is not 0 either. > > I am not sure, since the default identifier is 0-0. > I can add an additional check in ServiceWorkerClients to check for that and return early but I am not sure this is worth it. If JavaScript gives you 0-0 and you let it though. Then our code will handle to handle invalid identifiers (0 is invalid). Things like looking up the identifier in a HashMap would likely crash (because 0 is the magic empty value is such HashMap).
Comment on attachment 328004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328004&action=review >>> Source/WebCore/workers/service/ServiceWorkerClients.cpp:73 >>> +static inline ExceptionOr<std::optional<ServiceWorkerClientData>> isolatedExceptionOrClientData(const ExceptionOr<std::optional<ServiceWorkerClientData>>& value) >> >> I think the logic belongs in ExceptionOr.h. Something like: >> #include <wtf/CrossThreadCopier.h> >> template<typename T> struct CrossThreadCopierBase<false, false, ExceptionOr<T>> { >> typedef ExceptionOr<T> Type; >> static Type copy(const Type& source) >> { >> if (value.hasException()) >> return value.exception().isolatedCopy(); >> return CrossThreadCopier<T>::copy(value.releaseReturnValue()); >> } >> }; >> >> Then you can use crossThreadCopy() with your type. > > Will give it a try and will see whether optional is making things more difficult or not. I added support for using crossThreadCopy() with std::optional earlier today :) >>> Source/WebCore/workers/service/server/SWServer.cpp:308 >>> +std::optional<ServiceWorkerClientData> SWServer::clientFromId(const ClientOrigin& origin, ServiceWorkerClientIdentifier clientIdentifier) >> >> ById? > > I prefer FromId since a query is being done. This kind of operation is usually a getById though. See for e.g. getElementById(), not getElementFromId(). >>> Source/WebCore/workers/service/server/SWServerWorker.h:84 >>> + const ClientOrigin& origin() const; >> >> I think it weird that a method called SWServerWorker::origin() returns a ClientOrigin type. This is nothing to do with clients per say. > > SWServerWorker origin is the origin of the client it was spawned from. > Maybe ClientOrigin is not a very good name though since it has more to do with ITP than service worker clients. Then call it clientOrigin() ?
Created attachment 328102 [details] Patch
GTK is red.
Created attachment 328112 [details] Patch
(In reply to youenn fablet from comment #14) > Created attachment 328112 [details] > Patch build failure log is not clear at all. Let's retry once, it will probably fail but who knows.
> build failure log is not clear at all. Let's retry once, it will probably > fail but who knows. Was a flaky build issue apparently.
Comment on attachment 328112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328112&action=review > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:45 > + bool isValid() const { return serverConnectionIdentifier.toUInt64() && contextIdentifier.toUInt64(); } I do not think this is a good. We just never construct invalid identifiers. > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:63 > + if (!identifier) I think we should also return std::nullopt if *identifier is 0. > Source/WebCore/workers/service/ServiceWorkerClients.cpp:110 > + if (state->m_clients) How can clients ever be null here. If postTaskToServiceWorker managed to post the task, then the ServiceWorker exists, which means we have a ServiceWorkerGlobalScope, which means clients is alive. > Source/WebCore/workers/service/ServiceWorkerClients.cpp:136 > + request.state->m_clients = nullptr; It looks fragile that we have to do this. > Source/WebCore/workers/service/ServiceWorkerClients.h:74 > + class RequestState : public ThreadSafeRefCounted<RequestState> { Does this really need to be ThreadSafeRefCounted<RequestState>? Why cannot we pass an identifier to the lambda and then look up the state from m_pendingRequests when we're back on the service worker thread? > Source/WebCore/workers/service/ServiceWorkerClients.h:76 > + RequestState(ServiceWorkerClients*, DeferredPromise*, ServiceWorkerIdentifier); We should not have a public constructor for a RefCounted class. The constructor should be private and there should be a public create() factory function. > Source/WebCore/workers/service/ServiceWorkerClients.h:78 > + ServiceWorkerClients* m_clients; Having public m_* members is not a nice pattern or common practice. > Source/WebCore/workers/service/ServiceWorkerClients.h:80 > + ServiceWorkerIdentifier m_identifier; This name is too generic and confusing because we are also dealing with client identifiers. We should probably call it serviceWorkerIdentifier.
Created attachment 328138 [details] Patch
> View in context: > https://bugs.webkit.org/attachment.cgi?id=328112&action=review > > > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:45 > > + bool isValid() const { return serverConnectionIdentifier.toUInt64() && contextIdentifier.toUInt64(); } > > I do not think this is a good. We just never construct invalid identifiers. Default constructed identifier is 0-0. I would guess IPC identifiers could end up be invalid as well. > > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:63 > > + if (!identifier) > > I think we should also return std::nullopt if *identifier is 0. OK > > Source/WebCore/workers/service/ServiceWorkerClients.cpp:110 > > + if (state->m_clients) > > How can clients ever be null here. If postTaskToServiceWorker managed to > post the task, then the ServiceWorker exists, which means we have a > ServiceWorkerGlobalScope, which means clients is alive. I was thinking that the scope could stop owning the clients, which is not the case. There is no reason that clients should be an ActiveDOMObject either. Patch updated accordingly
Comment on attachment 328138 [details] Patch r=me if the bots happy.
Created attachment 328161 [details] Rebasing
Comment on attachment 328161 [details] Rebasing Clearing flags on attachment: 328161 Committed r225427: <https://trac.webkit.org/changeset/225427>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35808294>