content/common/service_worker/service_worker_event_dispatcher.mojom - Issue 2763453002: [ServiceWorker] Convert ping-pong IPC into mojo interface ServiceWorkerEventDispatcher

Unified Diff: content/common/service_worker/service_worker_event_dispatcher.mojom

Issue 2763453002: [ServiceWorker] Convert ping-pong IPC into mojo interface ServiceWorkerEventDispatcher (Closed)

Patch Set: Created 3 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« content/browser/service_worker/service_worker_version.cc ('K') | « content/browser/service_worker/service_worker_version.cc ('k') | content/common/service_worker/service_worker_messages.h » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: content/common/service_worker/service_worker_event_dispatcher.mojom

diff --git a/content/common/service_worker/service_worker_event_dispatcher.mojom b/content/common/service_worker/service_worker_event_dispatcher.mojom

index e47c085b9d9a8c9588cff6af96e73398ec7e75e2..d974106b7845bd332bcad659f35fb27eb618a6cb 100644

--- a/content/common/service_worker/service_worker_event_dispatcher.mojom

+++ b/content/common/service_worker/service_worker_event_dispatcher.mojom

@@ -73,4 +73,5 @@ interface ServiceWorkerEventDispatcher {

DispatchExtendableMessageEvent(ExtendableMessageEvent event)

=> (blink.mojom.ServiceWorkerEventStatus status,

mojo.common.mojom.Time dispatch_event_time);

+ Ping() => ();

falken 2017/03/21 06:54:04 Ping() doesn't really belong here, because it's no

shimazu 2017/03/21 07:59:27 My understanding is that Ping() is a method for pi

falken 2017/03/21 14:00:45 Hm, I think of Ping() as pinging the service worke

shimazu 2017/03/22 01:32:29 I totally agree with that. ServiceWorkerEventDispt

falken 2017/03/22 02:15:56 OK, if it's the only exception I think we don't ne

On 2017/03/22 01:32:29, shimazu wrote: > On 2017/03/21 14:00:45, falken wrote: > > On 2017/03/21 07:59:27, shimazu wrote: > > > On 2017/03/21 06:54:04, falken wrote: > > > > Ping() doesn't really belong here, because it's not an event. shimazu@ do > > you > > > > have plans about making new interfaces for all the messages, or perhaps > > > changing > > > > this interface name? > > > > > > My understanding is that Ping() is a method for pinging to the > > SWEventDispatcher > > > itself. I feel it's okay as is even though Ping is not an event, but I also > > > agree that Ping() might be confusing because it's not an event. > > > How about changing the method name like CheckIfResponsible() to clarify > that? > > > In that case, I think it's better to add a blank line before this method. > > > > > > > Hm, I think of Ping() as pinging the service worker itself. I'm not sure > > ServiceWorkerEventDispatcher is the right name if it's just the interface for > > IPCs sent from the browser to the service worker. Are all the existing legacy > > IPC messages going to be added to ServiceWorkerEventDispatcher? How many of > them > > are not about dispatching events? If there are many, maybe > > ServiceWorkerEventDispatcher should be renamed something like > > ServiceWorkerDispatcher. > > I totally agree with that. ServiceWorkerEventDisptacher is the interface > representing the context of a service worker used from the browser process. > > However, Ping() will be the only method except for Dispatch*Event > ServiceWorkerEventDispatcher will have. > // Link to the design doc > https://docs.google.com/document/d/1xW-NXKmqKu7pQ0hTNz9rpdGXgNaItyonOySVigXdq... > > ServiceWorkerEventDisptacher is possible to be considered as a counterpart of > ServiceWorkerGlobalScopeHost in the desgin doc, but I feel > ServiceWorkerEventDispatcher is clearer than "ServiceWorkerGlobalScope" or > "ServiceWorkerDispatcher" (but both have been already taken) because the > interface will have only Dispatch*Event except for Ping. > > What do you think?

OK, if it's the only exception I think we don't need to worry about it. I'd like a comment above Ping() explaining what it is though, something like: // Pings the service worker to check if it is responsive. If the callback is not called within a certain period of time, the browser will terminate the worker. Unlike the other functions in this interface, Ping() does not dispatch an event. I slightly prefer Ping() over CheckIfResponsive() to align with the ServiceWorkerVersion::Ping() naming.

};