Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25008006:
Flush out initial [un]registerServiceWorker roundtrip (Closed)
|
Created:
7 years, 2 months ago by alecflett Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFlush out initial [un]registerServiceWorker roundtrip
This is a first cut at getting registerServiceWorker() to make
a full round trip from and back into blink. At the moment the browser process
just sends a dummy value back to the renderer over IPC.
BUG=285976
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231688
Patch Set 1 #
Total comments: 24
Patch Set 2 : Fix thread access #Patch Set 3 : Clean up view/frame access #Patch Set 4 : Fix workerid #
Total comments: 21
Patch Set 5 : Update to reflect new Blink API #Patch Set 6 : Update to ToT #
Total comments: 4
Patch Set 7 : Now with security checks #Patch Set 8 : Additional comments for security #Patch Set 9 : Untangle message_filters #
Total comments: 4
Patch Set 10 : Fix request_id leak #
Total comments: 17
Patch Set 11 : Fix naming, other small nits #
Total comments: 14
Patch Set 12 : Fix nits #Patch Set 13 : Update to ToT #Patch Set 14 : update to ToT #
Total comments: 12
Patch Set 15 : Use int64 for workers, remove policy check #Patch Set 16 : Use int64 for workers, remove policy check #Patch Set 17 : Use int64 for workers, remove policy check #
Total comments: 12
Patch Set 18 : Address review comments #
Total comments: 26
Patch Set 19 : Address review comments #
Total comments: 12
Patch Set 20 : Address jam's nits #Patch Set 21 : Address jam's nits attempt 2 #Patch Set 22 : allow_unused #Patch Set 23 : Fix ALLOW_UNUSED again #Patch Set 24 : Fix ALLOW_UNUSED again #Patch Set 25 : Remove render_process_id_ for now #Messages
Total messages: 55 (0 generated)
This CL isn't 100% ready to land, I need feedback on a few things.
1) I've mostly followed the pattern from quota/filesystem/idb for message
filters and dispatchers on both the renderer and browser sides. I used quota's
pattern of mapping requests to threads, rather than piping a thread id through.
2) The "worker_id" is generated in the browser - the theory being that if two
renderers both try to register the same pattern/script combination that they
should both get the same worker_id. or if the script is already registered, then
again it will get the same worker_id.
3) This implementation just keeps a static counter in the browser process so as
to always create a new worker id. This is just temporary (I wrote "dont' check
in" but maybe it's fine here?)
4) I'm propagating a boolean through to the frontend ('registered') with the
expectation that the DOM object returned from the Promise will expose this... at
the moment the ServiceWorker DOM idl exposes no methods, but I'm certain that
this is going to be one of them - Alex and I will try to flush this out in the
spec.
5) I plumbed through from the RenderFrameImpl to the RenderViewImpl but honestly
I'm not sure if I need that or not - right now there's no way for the browser
process to know what domain this message comes from, and I think I need to get
that from view->document.
Thanks for sending this, I'm still on my way catching up but let me start with simple/naive comments/questions. (Hope you can just let me know if any of my comments are not relevant :)) https://codereview.chromium.org/25008006/diff/1/content/browser/service_worke... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/1/content/browser/service_worke... content/browser/service_worker/service_worker_dispatcher_host.cc:40: static int32 worker_id = 0; So the plan is we're replacing this with browser-side registry context (maybe attached to the browser context?), which maps pattern/script to a worker_id (and some more contextual info), and also will manage some browser-side cache/storage stuff... am I following? https://codereview.chromium.org/25008006/diff/1/content/child/child_thread.cc File content/child/child_thread.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/child_thread.cc... content/child/child_thread.cc:7: #include <string> is this needed? https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/service_worker_dispatcher.cc:108: WebServiceWorkerImpl* worker = new WebServiceWorkerImpl(worker_id, true); (we could think more about this later, but anyway) I think I prefer not to use boolean if possible.. https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/service_worker_dispatcher.h:57: IDMap<WebServiceWorkerImpl, IDMapOwnPointer> workers_; (oh, ok these are all owned here) https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/web_service_worker_proxy.h (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_proxy.h:20: Not directly related to this CL, but if you guys have a brief plan about what methods this class will be exposing can you share them? (If not yet it's totally fine too) https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/web_service_worker_registry_proxy.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_registry_proxy.cc:27: pattern, scriptUrl, callbacks); So we start with an assumption that we call this only on the main thread (while we also have some code which works on any threads)? (just asking) If that's the case we might be able to start with slightly simpler/less code? (But in the way that no much effort will be necessary when we're converting it to real multi-threaded)
https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/service_worker_dispatcher.cc:122: callbacks->onSuccess(worker); Why is a Worker passed as a result of unregistration? https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/service_worker_message_filter.h:33: RequestIdToThreadId request_id_map_; There's a leak here. Upon worker shutdown, pending requests that map to that thread should be deleted from this collection. In what ways is having this mapping better than having the threadid being sent round trip and not needing the mapping? With the mapping, we need locks and extra logic to maintain the collection. https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/web_service_worker_proxy.h (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_proxy.h:14: class WebServiceWorkerImpl The name of the class vs the name of the file is confusing. https://codereview.chromium.org/25008006/diff/1/content/common/service_worker... File content/common/service_worker_messages.h (right): https://codereview.chromium.org/25008006/diff/1/content/common/service_worker... content/common/service_worker_messages.h:30: int32 /* service_worker_id */) What does the second parameter mean here and in the next message? https://codereview.chromium.org/25008006/diff/1/content/renderer/render_frame... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/25008006/diff/1/content/renderer/render_frame... content/renderer/render_frame_impl.cc:221: // RenderViewImpl::FromWebView(frame->view()) ? I'd vote to make the WebServiceWorkerRegistry registry NOT frame specific and to just have the a single instance of that thing. Would that simplify things? Platform::serviceWorkerRegistry() sort of accessor.
Thanks for the feedback - I'll cleanup the leak and go over the threading stuff to make sure it's at least consistent within the scope of this patch. I'm working on a followup patch to this that flushes out some of the browser-side objects for managing registration (without any persistence of course) https://codereview.chromium.org/25008006/diff/1/content/browser/service_worke... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/1/content/browser/service_worke... content/browser/service_worker/service_worker_dispatcher_host.cc:40: static int32 worker_id = 0; On 2013/09/30 12:41:17, kinuko wrote: > So the plan is we're replacing this with browser-side registry context (maybe > attached to the browser context?), which maps pattern/script to a worker_id (and > some more contextual info), and also will manage some browser-side cache/storage > stuff... am I following? Yes exactly. I think either we need a global singleton (ServiceWorkerService?) or maybe just one-per-storage-partition, which is ServiceWorkerContext. https://codereview.chromium.org/25008006/diff/1/content/child/child_thread.cc File content/child/child_thread.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/child_thread.cc... content/child/child_thread.cc:7: #include <string> On 2013/09/30 12:41:17, kinuko wrote: > is this needed? Yes, IWYU (Include What You Use) says you should #include <string> if you use std::string at all. https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/service_worker_dispatcher.cc:108: WebServiceWorkerImpl* worker = new WebServiceWorkerImpl(worker_id, true); On 2013/09/30 12:41:17, kinuko wrote: > (we could think more about this later, but anyway) I think I prefer not to use > boolean if possible.. Yeah, I'm not totally excited about that either. I'm happy to wrap this in a struct so it has a name (and a place to put any additional context) and/or use an enum. Ultimately this same object will be found on navigator.serviceWorker, so this boolean could show up as a property like ".active" https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/service_worker_dispatcher.cc:122: callbacks->onSuccess(worker); On 2013/09/30 23:41:27, michaeln wrote: > Why is a Worker passed as a result of unregistration? It's unspecified right now but it seems the most consistent with the result of registerServiceWorker(). Basically we need the promise to return something and this seems like the best choice. Filed issue: https://github.com/slightlyoff/ServiceWorker/issues/90 https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/web_service_worker_proxy.h (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_proxy.h:14: class WebServiceWorkerImpl On 2013/09/30 23:41:27, michaeln wrote: > The name of the class vs the name of the file is confusing. I agree but I'm running out of names for things that mean "the implementation of the proxy for a service worker" :) I welcome other names, but this one at least seemed consistent with the equivalent Web* objects in the Blink API. https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_proxy.h:20: On 2013/09/30 12:41:17, kinuko wrote: > Not directly related to this CL, but if you guys have a brief plan about what > methods this class will be exposing can you share them? (If not yet it's totally > fine too) This needs to be flushed out on github, here's where it will show up: https://github.com/slightlyoff/ServiceWorker/blob/master/service_worker.ts#L71 (If the lines move, it's the class/interface that starts with 'interface SharedServiceWorker extends Worker') https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/web_service_worker_registry_proxy.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_registry_proxy.cc:27: pattern, scriptUrl, callbacks); On 2013/09/30 12:41:17, kinuko wrote: > So we start with an assumption that we call this only on the main thread (while > we also have some code which works on any threads)? (just asking) Well my intention was to support any main or worker thread by default when working in a child process - meaning you could call registerServiceWorker() from a worker. I didn't think I was assuming I was only on the main thread, maybe I'm misunderstanding ChildThread::current() here? Is there an assertion to make sure we're being called on the main thread or the worker thread, but not any other random threads in the renderer? Please let me know if there are additional things I should be protecting against. I'm also starting to investigate tests so that might be a good place to sanity check my assumptions as well. > > If that's the case we might be able to start with slightly simpler/less code? > (But in the way that no much effort will be necessary when we're converting it > to real multi-threaded) https://codereview.chromium.org/25008006/diff/1/content/renderer/render_frame... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/25008006/diff/1/content/renderer/render_frame... content/renderer/render_frame_impl.cc:221: // RenderViewImpl::FromWebView(frame->view()) ? On 2013/09/30 23:41:27, michaeln wrote: > I'd vote to make the WebServiceWorkerRegistry registry NOT frame specific and to > just have the a single instance of that thing. Would that simplify things? > > Platform::serviceWorkerRegistry() sort of accessor. I went this way after some consultation with abarth@. I'm not a fan of the single instance - I don't think it would simplify things as we'd have to keep our own map and looking up the current origin to find an object that would otherwise already be attached to the frame or view. I would personally like the notion of being per-origin but that concept doesn't really exist in the WebKit API... (though maybe it should) I'm expecting to use the frame as a way to get to the current security origin... I feel like this is analogous to the ApplicationCacheHost and related classes.
https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/service_worker_message_filter.h:33: RequestIdToThreadId request_id_map_; On 2013/09/30 23:41:27, michaeln wrote: > There's a leak here. Upon worker shutdown, pending requests that map to that > thread should be deleted from this collection. > > In what ways is having this mapping better than having the threadid being sent > round trip and not needing the mapping? With the mapping, we need locks and > extra logic to maintain the collection. I believe this part's designed after quota code, so let me write something about this... In short I don't really feel either way is particularly better than the other way. Sending thread_id to the browser process where the info is not needed at all feels unfortunate while keeping a locked map isn't great either. The former approach is simpler but taint wider code base. For quota case we already had browser-side code and we didn't want to change it, so we took the locked map approach. (Btw I'll need to fix the leak in quota code too)
https://codereview.chromium.org/25008006/diff/1/content/child/child_thread.cc File content/child/child_thread.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/child_thread.cc... content/child/child_thread.cc:7: #include <string> On 2013/10/01 00:17:04, alecflett wrote: > On 2013/09/30 12:41:17, kinuko wrote: > > is this needed? > > Yes, IWYU (Include What You Use) says you should #include <string> if you use > std::string at all. Oh, ok so this is not really for this change but to fix style in this file. https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/web_service_worker_proxy.h (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_proxy.h:14: class WebServiceWorkerImpl On 2013/10/01 00:17:04, alecflett wrote: > On 2013/09/30 23:41:27, michaeln wrote: > > The name of the class vs the name of the file is confusing. > > I agree but I'm running out of names for things that mean "the implementation of > the proxy for a service worker" :) > > I welcome other names, but this one at least seemed consistent with the > equivalent Web* objects in the Blink API. Doesn't just a webserviceworker_impl.{cc,h} work? https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_proxy.h:20: On 2013/10/01 00:17:04, alecflett wrote: > On 2013/09/30 12:41:17, kinuko wrote: > > Not directly related to this CL, but if you guys have a brief plan about what > > methods this class will be exposing can you share them? (If not yet it's > totally > > fine too) > > This needs to be flushed out on github, here's where it will show up: > > https://github.com/slightlyoff/ServiceWorker/blob/master/service_worker.ts#L71 > > (If the lines move, it's the class/interface that starts with 'interface > SharedServiceWorker extends Worker') Thanks! https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... File content/child/service_worker/web_service_worker_registry_proxy.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/... content/child/service_worker/web_service_worker_registry_proxy.cc:27: pattern, scriptUrl, callbacks); On 2013/10/01 00:17:04, alecflett wrote: > On 2013/09/30 12:41:17, kinuko wrote: > > So we start with an assumption that we call this only on the main thread > (while > > we also have some code which works on any threads)? (just asking) > > Well my intention was to support any main or worker thread by default when > working in a child process - meaning you could call registerServiceWorker() from > a worker. I didn't think I was assuming I was only on the main thread, maybe I'm > misunderstanding ChildThread::current() here? Is there an assertion to make sure > we're being called on the main thread or the worker thread, but not any other > random threads in the renderer? ChildThread::current() returns null on non-main thread, as noted in the header file comment (content/child/child_thread.h). If this could be called on an arbitrary threads I think we should rather call ServiceWorkerDispatcher::ThreadSpecificInstance() to get an instance. (If we're on the main thread we'll automatically get the same one as ChildThread::current()->service_worker_dispatcher()) > Please let me know if there are additional things I should be protecting > against. I'm also starting to investigate tests so that might be a good place to > sanity check my assumptions as well. > > > > > If that's the case we might be able to start with slightly simpler/less code? > > (But in the way that no much effort will be necessary when we're converting it > > to real multi-threaded) >
So I looked into some of the issues, and they're more closely tied than I
realized. I tried to switch over to using ThreadSpecificInstance since that does
seem like a good use of that function.
I also learned that ChildThread::current() does in fact return the current
thread as long as the current thread is a subclass of ChildThread (there are
actually quite a number of these) RenderThreadImpl and WorkerThread both follow
these, as do a number of other threads - this seems to be the intended use as
well.
I specifically needed a ThreadSafeSender in order to use ThreadSpecificInstance,
as well as the ServiceWorkerMessageFilter there.. which aren't really available
to me within the renderer frame/view.. but I can get there from the thread.
dispatcher = ServiceWorkerDispatcher::ThreadSpecificInstance(
ChildThread::current()->service_worker_message_filter(),
ChildThread::current()->thread_safe_sender());
Which has the advantage of lazily creating it if we're not on the right thread.
It's not pretty but there certainly is something attractive about the call to
ThreadSpecificInstance()) That said, if the dispatcher isn't already created,
we've probably done something wrong there anyway.
Now the real issue here is that thread_safe_sender isn't readily available in
the view, (that's why I grab it from the ChildThread::current() above) but it is
in the RenderWebKitPlatformSupport (which also gets it from
ChildThread::current())..
which leads back to michael's suggestion of using Platform rather than WebFrame.
The problem I have with that is that these messages really are coming from
rendered views or workers, and in particular they *return* that way too - in
order to route them appropriately we're going to have to do some funky tricks
with routing ids to get from the platform impl to the right view in the
callback.
So there are two possible resolutions here:
1) Use what's there going through WebFrame
2) Move this api to Platform, and use ThreadSpecificInstance
I'm leaning towards 1) but I'm going to ask for some other opinions as well.
On 2013/10/02 18:59:55, alecflett wrote: > So I looked into some of the issues, and they're more closely tied than I > realized. I tried to switch over to using ThreadSpecificInstance since that does > seem like a good use of that function. > > I also learned that ChildThread::current() does in fact return the current > thread as long as the current thread is a subclass of ChildThread (there are > actually quite a number of these) RenderThreadImpl and WorkerThread both follow > these, as do a number of other threads - this seems to be the intended use as > well. I think there's some confusion here- and also I might be mistaken what you meant when you said you'd like to support worker cases. WorkerThread is the main thread of SharedWorker, so if you're talking about supporting the API in SharedWorker yes ChildThread::current() returns non-null value, but in that case you also wouldn't need ThreadSpecificInstance. If you're on a worker thread for dedicated worker, though, the thread's not the main thread nor WorkerThread in chromium world either, and ChildThread::current() will return null unless we route the API onto the main thread in Blink. > I specifically needed a ThreadSafeSender in order to use ThreadSpecificInstance, > as well as the ServiceWorkerMessageFilter there.. which aren't really available > to me within the renderer frame/view.. but I can get there from the thread. > > dispatcher = ServiceWorkerDispatcher::ThreadSpecificInstance( > ChildThread::current()->service_worker_message_filter(), > ChildThread::current()->thread_safe_sender()); > > Which has the advantage of lazily creating it if we're not on the right thread. > It's not pretty but there certainly is something attractive about the call to > ThreadSpecificInstance()) That said, if the dispatcher isn't already created, > we've probably done something wrong there anyway. > > Now the real issue here is that thread_safe_sender isn't readily available in > the view, (that's why I grab it from the ChildThread::current() above) but it is > in the RenderWebKitPlatformSupport (which also gets it from > ChildThread::current()).. > > which leads back to michael's suggestion of using Platform rather than WebFrame. > The problem I have with that is that these messages really are coming from > rendered views or workers, and in particular they *return* that way too - in > order to route them appropriately we're going to have to do some funky tricks > with routing ids to get from the platform impl to the right view in the > callback. > > So there are two possible resolutions here: > 1) Use what's there going through WebFrame > 2) Move this api to Platform, and use ThreadSpecificInstance > > I'm leaning towards 1) but I'm going to ask for some other opinions as well. If the interface between Blink and content can be implemented without referring higher-level abstractions like ones defined in DOM in general doing so in Platform API would be much easier.
On 2013/10/03 01:42:59, kinuko wrote: > On 2013/10/02 18:59:55, alecflett wrote: > > So I looked into some of the issues, and they're more closely tied than I > > realized. I tried to switch over to using ThreadSpecificInstance since that > does > > seem like a good use of that function. > > > > I also learned that ChildThread::current() does in fact return the current > > thread as long as the current thread is a subclass of ChildThread (there are > > actually quite a number of these) RenderThreadImpl and WorkerThread both > follow > > these, as do a number of other threads - this seems to be the intended use as > > well. > > I think there's some confusion here- and also I might be mistaken what you meant > when you said you'd like to support worker cases. > > WorkerThread is the main thread of SharedWorker, so if you're talking about > supporting the API in SharedWorker yes ChildThread::current() returns non-null Michael helped explain this to me. So what I meant by "supporting workers" is that you'd be able to (eventually) call registerServiceWorker() from any kind of worker. After we talked a bit, we agreed that we could leave the ThreadSpecificInstance in there, but only support this on the "main" thread - in this case it means I'm calling ChildThread::current() only from inside RenderFr > If the interface between Blink and content can be implemented without referring > higher-level abstractions like ones defined in DOM in general doing so in > Platform API would be much easier. Well I think there are going to be two classes of Blink <-> content interaction - in the register case (this patch) we do ultimately go back to the DOM but the existing WebServiceWorker* classes shield the implementation from those details.
New patch updated... mind taking a look?
https://codereview.chromium.org/25008006/diff/23001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:40: static int32 NextWorkerId() { i think we usually prefer 'int' where int will do https://codereview.chromium.org/25008006/diff/23001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:50: // origin as the registering document. I'd vote to put in place scaffolding/structure that we believe will allow us to do what this comment suggests. As coded logic in this method body doesn't have any chance of doing that because it's got nothing to go on about where this call to register something is coming from. https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:73: message_filter_->RegisterRequestID(request_id, CurrentWorkerId()); Looks like two different dispatchers can generate the same request_id and which will collide in the filters map, request_id needs to be a global monotonically increasing value for this to work. https://codereview.chromium.org/25008006/diff/23001/content/renderer/render_f... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/25008006/diff/23001/content/renderer/render_f... content/renderer/render_frame_impl.h:56: virtual WebKit::WebServiceWorkerRegistry* serviceWorkerRegistry( I think I'd like to back up to the original blink CL and re review just what it is that we're doing and how. I think primary purpose of these initial CLs is to setup how it hangs together, but afaict, it just isn't hanging just yet. Let's change the webkitAPI in a few ways. 1) Let's make the returned object be that per-document thing and name it in some way that's better than 'registry'. 2) Put 'create' at the front of the method name to make it clear that it manufactures this object and passes ownership to blink. 3) Change the usages of this method in blink to have the same semantics in mind. Blink needs to provide storage for the return value and call delete on it at some point. As currently coded in blink + this CL, serviceWorkerRegistry() is invoked for each scriptable API call and the instance returned here is leaked.
https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:73: message_filter_->RegisterRequestID(request_id, CurrentWorkerId()); On 2013/10/04 01:26:07, michaeln wrote: > Looks like two different dispatchers can generate the same request_id and which > will collide in the filters map, request_id needs to be a global monotonically > increasing value for this to work. In case it's not obvious, i have a preference for sending the curworkerid in messages for the round trip :) Rational is its simpler and less error prone code, please look at the two bugs i've pointed out with the map maintenance stuff in this review as real world evidence of that (and bonus, no locks or extra maps).
https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:73: message_filter_->RegisterRequestID(request_id, CurrentWorkerId()); On 2013/10/04 01:51:52, michaeln wrote: > On 2013/10/04 01:26:07, michaeln wrote: > > Looks like two different dispatchers can generate the same request_id and > which > > will collide in the filters map, request_id needs to be a global monotonically > > increasing value for this to work. > > In case it's not obvious, i have a preference for sending the curworkerid in > messages for the round trip :) Rational is its simpler and less error prone > code, please look at the two bugs i've pointed out with the map maintenance > stuff in this review as real world evidence of that (and bonus, no locks or > extra maps). :( I have to agree that map stuff has some error-prone nature while it isolates thread-related details to one process. Either way works for me anyway, feel free to change this to the other pattern (or my preference is not to implement these curworkerid handlings until we really need it) https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:111: callbacks->onSuccess(worker); pending_callbacks_.Remove(request_id) here? Current SW blink API looks to pass several raw ptrs between module boundaries, it'd be nice to have some comments in the API header file to note if a given pointer needs to be freed by the callee or not. https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:27: explicit ServiceWorkerDispatcher(ThreadSafeSender* thread_safe_sender, nit: no need to be explicit https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:39: const GURL& scriptUrl, nit: script_url https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:57: IDMap<WebServiceWorkerImpl, IDMapOwnPointer> workers_; This one's not used? (Looks like we pass WebServiceWorkerImpl to blink and it's owned by Blink's ServiceWorker instance?) https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... File content/child/service_worker/service_worker_message_filter.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_message_filter.cc:63: request_id_map_.erase(it); This needs to be if (...) erase(it++); else ++it; (if we keep this code) https://codereview.chromium.org/25008006/diff/23001/content/content_child.gypi File content/content_child.gypi (right): https://codereview.chromium.org/25008006/diff/23001/content/content_child.gyp... content/content_child.gypi:74: 'child/service_worker/web_service_worker_registry_proxy.h', I think these *_proxy.{cc,h} files should be named *_impl.{cc,h} as far as they implement WebServiceWorkerImpl and WebServiceWorkerRegistryImpl? https://codereview.chromium.org/25008006/diff/23001/content/renderer/render_f... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/25008006/diff/23001/content/renderer/render_f... content/renderer/render_frame_impl.h:56: virtual WebKit::WebServiceWorkerRegistry* serviceWorkerRegistry( On 2013/10/04 01:26:07, michaeln wrote: > I think I'd like to back up to the original blink CL and re review just what it > is that we're doing and how. I think primary purpose of these initial CLs is to > setup how it hangs together, but afaict, it just isn't hanging just yet. > > Let's change the webkitAPI in a few ways. > > 1) Let's make the returned object be that per-document thing and name it in some > way that's better than 'registry'. > > 2) Put 'create' at the front of the method name to make it clear that it > manufactures this object and passes ownership to blink. > > 3) Change the usages of this method in blink to have the same semantics in mind. > Blink needs to provide storage for the return value and call delete on it at > some point. +1, except that I have not fully understood how WebSWRegistry is meant to be used. Does it really need to be implemented by chrome and kept in Blink? If what we need is a {un,}registration API and per-document registry for SWs it sounds like we can separate them.
https://codereview.chromium.org/25008006/diff/23001/content/content_child.gypi File content/content_child.gypi (right): https://codereview.chromium.org/25008006/diff/23001/content/content_child.gyp... content/content_child.gypi:74: 'child/service_worker/web_service_worker_registry_proxy.h', On 2013/10/04 14:55:48, kinuko wrote: > I think these *_proxy.{cc,h} files should be named *_impl.{cc,h} as far as they > implement WebServiceWorkerImpl and WebServiceWorkerRegistryImpl? +1, filename and classname should match one another
ok, I've updated this patch to reflect the review comments, and also to reflect the new Blink class names from https://codereview.chromium.org/26078002/ michaeln@ / kinuko@ PTAL? https://codereview.chromium.org/25008006/diff/23001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:40: static int32 NextWorkerId() { On 2013/10/04 01:26:07, michaeln wrote: > i think we usually prefer 'int' where int will do cpplint will warn/complain if you use native types - besides, these are being sent through messages, which need to have a fixed size. In any case this is just a temporary until I hook up the map in a followup CL. https://codereview.chromium.org/25008006/diff/23001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:50: // origin as the registering document. On 2013/10/04 01:26:07, michaeln wrote: > I'd vote to put in place scaffolding/structure that we believe will allow us to > do what this comment suggests. As coded logic in this method body doesn't have > any chance of doing that because it's got nothing to go on about where this call > to register something is coming from. OK https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:73: message_filter_->RegisterRequestID(request_id, CurrentWorkerId()); On 2013/10/04 01:26:07, michaeln wrote: > Looks like two different dispatchers can generate the same request_id and which > will collide in the filters map, request_id needs to be a global monotonically > increasing value for this to work. > oh wow yeah that means the quota api has this problem today? https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:73: message_filter_->RegisterRequestID(request_id, CurrentWorkerId()); On 2013/10/04 01:51:52, michaeln wrote: > On 2013/10/04 01:26:07, michaeln wrote: > > Looks like two different dispatchers can generate the same request_id and > which > > will collide in the filters map, request_id needs to be a global monotonically > > increasing value for this to work. > > In case it's not obvious, i have a preference for sending the curworkerid in > messages for the round trip :) Rational is its simpler and less error prone > code, please look at the two bugs i've pointed out with the map maintenance > stuff in this review as real world evidence of that (and bonus, no locks or > extra maps). Given the choice of using the global vs passing the extra value, I'll go with the extra value. I'll just rip out the map. https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:27: explicit ServiceWorkerDispatcher(ThreadSafeSender* thread_safe_sender, On 2013/10/04 14:55:48, kinuko wrote: > nit: no need to be explicit Done. https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:39: const GURL& scriptUrl, On 2013/10/04 14:55:48, kinuko wrote: > nit: script_url Done. https://codereview.chromium.org/25008006/diff/23001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:57: IDMap<WebServiceWorkerImpl, IDMapOwnPointer> workers_; On 2013/10/04 14:55:48, kinuko wrote: > This one's not used? (Looks like we pass WebServiceWorkerImpl to blink and it's > owned by Blink's ServiceWorker instance?) oops, yeah this just leftover from earlier iterations
we need to wait on the blink cl for this one.... https://codereview.chromium.org/25008006/diff/41001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/41001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:28: ServiceWorkerMessageFilter* message_filter); Is message_filter still needed? Might as well extract full value from our simplifications. https://codereview.chromium.org/25008006/diff/41001/content/renderer/render_f... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/25008006/diff/41001/content/renderer/render_f... content/renderer/render_frame_impl.h:56: virtual WebKit::WebServiceWorkerProvider* serviceWorkerProvider( s/b createServiceWorkerProvider(...)
Ok, I've updated the blink side to include a WebURL origin with each request, and updated this patch with the security checks that I'm aware we'll need right now. I'm using cookie access as a proxy for ServiceWorker access. We may also want settings/policy checks too, but I'll hold off on that for now since there are no service-worker specific settings right now. https://codereview.chromium.org/25008006/diff/41001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/41001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:28: ServiceWorkerMessageFilter* message_filter); On 2013/10/07 23:17:34, michaeln wrote: > Is message_filter still needed? Might as well extract full value from our > simplifications. nice catch, didn't even realize that. Fully untangled. https://codereview.chromium.org/25008006/diff/41001/content/renderer/render_f... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/25008006/diff/41001/content/renderer/render_f... content/renderer/render_frame_impl.h:56: virtual WebKit::WebServiceWorkerProvider* serviceWorkerProvider( On 2013/10/07 23:17:34, michaeln wrote: > s/b createServiceWorkerProvider(...) Hmm.. this was from an older value of the patch, not sure how it got in there.
(Some lighthearted comments while waiting for the other patch) https://codereview.chromium.org/25008006/diff/52001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/52001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:109: new WebServiceWorkerImpl(service_worker_id, true); Well... I guessed you could have just named this WebServiceWorkerProxy (and revert the impl/proxy file name changes)? It looks this interface is going to be used for communication between the actual Service Worker and if so the term proxy sounds good to me. (And WebFooProxy implementing WebFoo pattern sounds ok to me too) Wdyt- michaeln@ / alec@? https://codereview.chromium.org/25008006/diff/52001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:110: callbacks->onSuccess(worker); Should we do pending_callbacks_.Remove(request_id) to free up the callbacks here?
https://codereview.chromium.org/25008006/diff/52001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/52001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:109: new WebServiceWorkerImpl(service_worker_id, true); On 2013/10/08 02:30:19, kinuko wrote: > Well... I guessed you could have just named this WebServiceWorkerProxy (and > revert the impl/proxy file name changes)? It looks this interface is going to > be used for communication between the actual Service Worker and if so the term > proxy sounds good to me. (And WebFooProxy implementing WebFoo pattern sounds ok > to me too) Wdyt- michaeln@ / alec@? Well, it's more of an 'impl' - it's not proxying anything, just implementing a WebKit API interface. It does send its messages across IPC but not with the intention of hitting another WebServiceWorker on the other side. https://codereview.chromium.org/25008006/diff/52001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:110: callbacks->onSuccess(worker); On 2013/10/08 02:30:19, kinuko wrote: > Should we do pending_callbacks_.Remove(request_id) to free up the callbacks > here? oops! fixed.
https://codereview.chromium.org/25008006/diff/57001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/57001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:35: virtual void OnWorkerRunLoopStopped() OVERRIDE; nit: could this method be private https://codereview.chromium.org/25008006/diff/57001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:57: int32 request_id, here its called request_id... that's the name i like ;) https://codereview.chromium.org/25008006/diff/57001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:58: int32 worker_id); maybe expand this to service_worker_id so its less easily confused with a webcore worker thread id of some kind (although i think we'll have lots of confusion along those lines no matter what we do given our chosen name for the uniquely and persistently identified "ServiceWorker" thing). https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... File content/common/service_worker_messages.h (right): https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... content/common/service_worker_messages.h:18: int32 /* callback_id */, Lets use names consistently across msg structs and method sigs, in the DispatcherHost callback_id is referred to as registry_id. Without really caring too much about what name is used, i do care to see the same name used... having said that, i do think request id is the most appropiate name. https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... content/common/service_worker_messages.h:32: IPC_MESSAGE_CONTROL3(ServiceWorkerMsg_ServiceWorkerRegistered, Shouldn't these response messages convey success or failure in some form? It's clear the request is complete... but did it succeed? https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... content/common/service_worker_messages.h:35: int32 /* service_worker_id */) can this id be an int64 value to make the distinction between transient values and things that are persistent? this id will ultimately be written to disk. https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... content/common/service_worker_messages.h:41: int32 /* service_worker_id */) i still don't think we need service_worker_id here. suppose we unregister something that was not registered to start with. what id value goes here? https://codereview.chromium.org/25008006/diff/57001/content/renderer/render_f... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/25008006/diff/57001/content/renderer/render_f... content/renderer/render_frame_impl.cc:233: GURL(frame->document().securityOrigin().toString()), i think this will fail for some securityOrigin strings, we have some helper functions for this... i'll come back with a ptr to them
ok, addressed these as follows: https://codereview.chromium.org/25008006/diff/57001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/57001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:35: virtual void OnWorkerRunLoopStopped() OVERRIDE; On 2013/10/08 23:08:38, michaeln wrote: > nit: could this method be private Done. https://codereview.chromium.org/25008006/diff/57001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:57: int32 request_id, On 2013/10/08 23:08:38, michaeln wrote: > here its called request_id... that's the name i like ;) Sorry I'll try to make these a little more consistent. https://codereview.chromium.org/25008006/diff/57001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.h:58: int32 worker_id); On 2013/10/08 23:08:38, michaeln wrote: > maybe expand this to service_worker_id so its less easily confused with a > webcore worker thread id of some kind (although i think we'll have lots of > confusion along those lines no matter what we do given our chosen name for the > uniquely and persistently identified "ServiceWorker" thing). Done. https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... File content/common/service_worker_messages.h (right): https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... content/common/service_worker_messages.h:18: int32 /* callback_id */, On 2013/10/08 23:08:38, michaeln wrote: > Lets use names consistently across msg structs and method sigs, in the > DispatcherHost callback_id is referred to as registry_id. Without really caring > too much about what name is used, i do care to see the same name used... having > said that, i do think request id is the most appropiate name. oops, I'll clean up. https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... content/common/service_worker_messages.h:32: IPC_MESSAGE_CONTROL3(ServiceWorkerMsg_ServiceWorkerRegistered, On 2013/10/08 23:08:38, michaeln wrote: > Shouldn't these response messages convey success or failure in some form? It's > clear the request is complete... but did it succeed? My plan was to make a separate message for the error. I'd rather not call it "Registered" and then have it not have actually registered. This also makes the dispatching pretty clean. https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... content/common/service_worker_messages.h:35: int32 /* service_worker_id */) On 2013/10/08 23:08:38, michaeln wrote: > can this id be an int64 value to make the distinction between transient values > and things that are persistent? this id will ultimately be written to disk. oh, I wasn't imagining that these were persistent - I was imagining that we'd still be looking up the pattern/script in the db and dynamically assign an id. (Assuming we end up using leveldb to store this stuff, there's no reason to use a numeric key IMO) https://codereview.chromium.org/25008006/diff/57001/content/common/service_wo... content/common/service_worker_messages.h:41: int32 /* service_worker_id */) On 2013/10/08 23:08:38, michaeln wrote: > i still don't think we need service_worker_id here. suppose we unregister > something that was not registered to start with. what id value goes here? (Again, separate error message) https://codereview.chromium.org/25008006/diff/57001/content/renderer/render_f... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/25008006/diff/57001/content/renderer/render_f... content/renderer/render_frame_impl.cc:233: GURL(frame->document().securityOrigin().toString()), On 2013/10/08 23:08:38, michaeln wrote: > i think this will fail for some securityOrigin strings, we have some helper > functions for this... i'll come back with a ptr to them So I was following a pattern I found elsewhere (again, using Quota as inspiration) - happy to use the right pattern if this isn't it.
https://codereview.chromium.org/25008006/diff/57001/content/renderer/render_f... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/25008006/diff/57001/content/renderer/render_f... content/renderer/render_frame_impl.cc:233: GURL(frame->document().securityOrigin().toString()), On 2013/10/09 01:23:47, alecflett wrote: > On 2013/10/08 23:08:38, michaeln wrote: > > i think this will fail for some securityOrigin strings, we have some helper > > functions for this... i'll come back with a ptr to them > > So I was following a pattern I found elsewhere (again, using Quota as > inspiration) - happy to use the right pattern if this isn't it. Not sure michael is talking about the same thing, but common pattern is to check if securityOrigin.isUnique() is true or not and then get toString() for its URL only if it's true. toString() returns non-url-like string (e.g. "null") for unique origins. If we're going to have a method like allowServiceWorker() (in blink) I assume we'll do such check there (in conjunction with other necessary checks like against permissionClient etc) so we should be safe here (quota does this check in blink side).
(nits only) https://codereview.chromium.org/25008006/diff/63001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/63001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.h:32: int32 registry_id, nit: request_id https://codereview.chromium.org/25008006/diff/63001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.h:37: int32 registry_id, nit: request_id https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:105: // the proxy object doesn't have to be consistent unless we require nit: maybe 'proxy object' -> 'service worker object' if it's not a proxy? https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/service_worker_message_filter.cc (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/service_worker_message_filter.cc:42: } I think you'd want to do bool result = PickleIterator(msg).ReadInt(&thread_id); DCHECK(result); instead of consulting the map in the new code. https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/service_worker_message_filter.h:29: RequestIdToThreadId request_id_map_; They are no longer necessary https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/web_service_worker_provider_impl.cc (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/web_service_worker_provider_impl.cc:38: CHECK(origin == origin_) << "Requesting origin has changed since service " CHECK_EQ(origin_, origin) maybe better (friendlier log) https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/web_service_worker_provider_impl.h (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/web_service_worker_provider_impl.h:39: const WebKit::WebURL& scriptUrl, nit: scriptUrl -> script_url
all outstanding comments addressed. https://codereview.chromium.org/25008006/diff/63001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/63001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.h:32: int32 registry_id, On 2013/10/09 12:13:25, kinuko wrote: > nit: request_id Done. https://codereview.chromium.org/25008006/diff/63001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.h:37: int32 registry_id, On 2013/10/09 12:13:25, kinuko wrote: > nit: request_id Done. https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/service_worker_dispatcher.cc:105: // the proxy object doesn't have to be consistent unless we require On 2013/10/09 12:13:25, kinuko wrote: > nit: maybe 'proxy object' -> 'service worker object' if it's not a proxy? Done. (and clarified the language too) https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/service_worker_message_filter.cc (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/service_worker_message_filter.cc:42: } On 2013/10/09 12:13:25, kinuko wrote: > I think you'd want to do > > bool result = PickleIterator(msg).ReadInt(&thread_id); > DCHECK(result); > > instead of consulting the map in the new code. oops. Done. https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/service_worker_message_filter.h:29: RequestIdToThreadId request_id_map_; On 2013/10/09 12:13:25, kinuko wrote: > They are no longer necessary Done. https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/web_service_worker_provider_impl.cc (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/web_service_worker_provider_impl.cc:38: CHECK(origin == origin_) << "Requesting origin has changed since service " On 2013/10/09 12:13:25, kinuko wrote: > CHECK_EQ(origin_, origin) maybe better (friendlier log) Done. https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... File content/child/service_worker/web_service_worker_provider_impl.h (right): https://codereview.chromium.org/25008006/diff/63001/content/child/service_wor... content/child/service_worker/web_service_worker_provider_impl.h:39: const WebKit::WebURL& scriptUrl, On 2013/10/09 12:13:25, kinuko wrote: > nit: scriptUrl -> script_url Done.
Here's what I was thinking for the error messages: https://codereview.chromium.org/26631006/
https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.cc (left): https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:42: // TODO(alecflett): Enforce that script_url must have the same this todo seems important still https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:42: static int32 NextWorkerId() { I propose we use int64 bit IDs to identify registered workers and return the value that identifies the registration in response to registration calls. If the id we return here is not the id we store persistently, we'll have to maintain some mapping to go between the two and allocate the transient ids on the fly, which sounds like add complexity that we can avoid. https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:47: void ServiceWorkerDispatcherHost::OnRegisterServiceWorker( I think goals for this method at this time are primarily to provide completion, send some response, so the client side sees fullfilled or rejected promises and can make progress. Beyond that is just a TODO. https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:51: const GURL& script_url) { I think there's a requirement that 'scope' and 'script_url' be of the same origin. That would be a good check to codify at this time and to go thru an error return path. https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:57: if (!policy->CanAccessCookiesForOrigin(render_process_id_, scope)) I'd actually like to define a policy interface here and express in that interface what it is that we're asking. The impl details of how we those policy decisions are made can come later and the content settings aspects are ultimately a function of chome. I'm not suggesting that get done now. ServiceWorkerPolicy { virtual bool CanRegisterServiceWorker(...); virtual bool CanUnregisterServiceWorker(...); }; I'm not sure the CanAccessCookiesForOrigin() checks do what you're expecting them to do? They look to be of a different nature, not permission related, these checks are site isolation related and will generally return true unless some unusual conditions are met... // Only might return false if the very experimental // --enable-strict-site-isolation or --site-per-process flags are used. I think we could put a todo here for now about these kinds of checks. // TODO(xxx): deal with site isolation and ChildProcessSecurityPolicyImpl https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:78: thread_id, request_id, NextWorkerId())); I still don't understand the intent of returning the id of the now unregistered SW? https://codereview.chromium.org/25008006/diff/79001/content/child/service_wor... File content/child/service_worker/web_service_worker_provider_impl.h (right): https://codereview.chromium.org/25008006/diff/79001/content/child/service_wor... content/child/service_worker/web_service_worker_provider_impl.h:46: WebKit::WebServiceWorkerProviderClient* ALLOW_UNUSED client_; Should this be a scoped_ptr<WebKit::WebServiceWorkerProviderClient> now?
ok, issues addressed. PTAL? https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:42: static int32 NextWorkerId() { On 2013/10/16 02:03:42, michaeln wrote: > I propose we use int64 bit IDs to identify registered workers and return the > value that identifies the registration in response to registration calls. If the > id we return here is not the id we store persistently, we'll have to maintain > some mapping to go between the two and allocate the transient ids on the fly, > which sounds like add complexity that we can avoid. Done. https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:47: void ServiceWorkerDispatcherHost::OnRegisterServiceWorker( On 2013/10/16 02:03:42, michaeln wrote: > I think goals for this method at this time are primarily to provide completion, > send some response, so the client side sees fullfilled or rejected promises and > can make progress. Beyond that is just a TODO. Except for origin mismatch, so adding that now. https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:51: const GURL& script_url) { On 2013/10/16 02:03:42, michaeln wrote: > I think there's a requirement that 'scope' and 'script_url' be of the same > origin. That would be a good check to codify at this time and to go thru an > error return path. Done. https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:57: if (!policy->CanAccessCookiesForOrigin(render_process_id_, scope)) On 2013/10/16 02:03:42, michaeln wrote: > I'd actually like to define a policy interface here and express in that > interface what it is that we're asking. The impl details of how we those policy > decisions are made can come later and the content settings aspects are > ultimately a function of chome. I'm not suggesting that get done now. > > ServiceWorkerPolicy { > virtual bool CanRegisterServiceWorker(...); > virtual bool CanUnregisterServiceWorker(...); > }; > > > I'm not sure the CanAccessCookiesForOrigin() checks do what you're expecting > them to do? They look to be of a different nature, not permission related, these > checks are site isolation related and will generally return true unless some > unusual conditions are met... > // Only might return false if the very experimental > // --enable-strict-site-isolation or --site-per-process flags are used. > > I think we could put a todo here for now about these kinds of checks. > // TODO(xxx): deal with site isolation and ChildProcessSecurityPolicyImpl Done. Adding a TODO. https://codereview.chromium.org/25008006/diff/79001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.cc:78: thread_id, request_id, NextWorkerId())); On 2013/10/16 02:03:42, michaeln wrote: > I still don't understand the intent of returning the id of the now unregistered > SW? removed.
It looks the uploaded patches are screwed up. Can you try uploading them again?
On 2013/10/24 03:35:36, kinuko wrote: > It looks the uploaded patches are screwed up. Can you try uploading them again? OMG. I already uploaded them 2-3 times. Hopefully reitveld is feeling healthier now..
On 2013/10/24 18:24:50, alecflett wrote: > On 2013/10/24 03:35:36, kinuko wrote: > > It looks the uploaded patches are screwed up. Can you try uploading them > again? > > OMG. I already uploaded them 2-3 times. Hopefully reitveld is feeling healthier > now.. ok, looks like the changes stuck this time. Sorry about that!
https://codereview.chromium.org/25008006/diff/248001/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/248001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:52: // registration permission. stray comment is n/a anymore, at least not yet? https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:23: ServiceWorkerDispatcher* const kHasBeenDeleted = this wants to be either static or in an anon namespace to limit its scope https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:52: static int CurrentWorkerId() { please move this helper up to where the other non-instance helper stuff is https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:122: callbacks->onSuccess(nullptr); use NULL here https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... File content/child/service_worker/web_service_worker_impl.h (right): https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... content/child/service_worker/web_service_worker_impl.h:17: WebServiceWorkerImpl(int64 service_worker_id, bool registered) since registered is always 'true', can we remove this ctor argument and the data member? https://codereview.chromium.org/25008006/diff/248001/content/renderer/render_... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/25008006/diff/248001/content/renderer/render_... content/renderer/render_frame_impl.h:58: WebKit::WebServiceWorkerProviderClient*); In WebFrameClient.h, the default/stub impl of createServiceWorkerProvider might want to delete the client passed in. // May return null. Takes ownership of the client. { delete client; return 0; }
ok, comments addressed https://codereview.chromium.org/25008006/diff/248001/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/248001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:52: // registration permission. On 2013/10/24 21:57:39, michaeln wrote: > stray comment is n/a anymore, at least not yet? Done. https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:23: ServiceWorkerDispatcher* const kHasBeenDeleted = On 2013/10/24 21:57:39, michaeln wrote: > this wants to be either static or in an anon namespace to limit its scope Done. https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:52: static int CurrentWorkerId() { On 2013/10/24 21:57:39, michaeln wrote: > please move this helper up to where the other non-instance helper stuff is Done. https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:122: callbacks->onSuccess(nullptr); On 2013/10/24 21:57:39, michaeln wrote: > use NULL here Done. https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... File content/child/service_worker/web_service_worker_impl.h (right): https://codereview.chromium.org/25008006/diff/248001/content/child/service_wo... content/child/service_worker/web_service_worker_impl.h:17: WebServiceWorkerImpl(int64 service_worker_id, bool registered) On 2013/10/24 21:57:39, michaeln wrote: > since registered is always 'true', can we remove this ctor argument and the data > member? Done. https://codereview.chromium.org/25008006/diff/248001/content/renderer/render_... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/25008006/diff/248001/content/renderer/render_... content/renderer/render_frame_impl.h:58: WebKit::WebServiceWorkerProviderClient*); On 2013/10/24 21:57:39, michaeln wrote: > In WebFrameClient.h, the default/stub impl of createServiceWorkerProvider might > want to delete the client passed in. > > // May return null. Takes ownership of the client. > { delete client; return 0; } ah not a bad idea. https://codereview.chromium.org/42143002
lgtm pending greenbots and the small ctor change https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/web_service_worker_impl.h (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/web_service_worker_impl.h:17: WebServiceWorkerImpl(int64 service_worker_id) explicit keyword here
darin@ - this patch spreads across a large number of directories but most of what I need review for is fairly boilerplate. This is laying down the IPC stuff for service worker <-> navigator communication.
tsepez@ - mind reviewing our messages stuff? You can also take a look at the browser-side service_worker_dispatcher_host since that's where registration begins. This feature is off by default (on with a flag) and that's checked in the browser process.
LGTM after addressing the comment. Please don't remove your flag until the CPSP work is done. https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:51: // TODO(alecflett): add a ServiceWorker-specific policy query in Please file a follow-up bug for this and link it here.
lgtm + nits https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.h:30: nit: can we remove this line so that IPC handlers come right after this comment? https://codereview.chromium.org/25008006/diff/298002/content/child/child_thre... File content/child/child_thread.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/child/child_thre... content/child/child_thread.cc:266: channel_->RemoveFilter(service_worker_message_filter_.get()); nit: we usually remove filters in reverse of initialization order https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:20: namespace { nit: can you insert an empty line after this line https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:31: } nit: } // namespace https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:112: callbacks->onSuccess(worker); nit: I prefer arranging these lines like: callbacks->onSuccess(new WebServiceWorkerImpl(...)); or scoped_ptr<WebServiceWorkerImpl> worker( new WebServiceWorkerImpl(service_worker_id)); callbacks->onSuccess(worker.release()); https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_message_filter.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: Copyright 2013 https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_message_filter.h:13: namespace content { nit: please insert an empty line after namespace content { https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/web_service_worker_provider_impl.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/web_service_worker_provider_impl.cc:27: : thread_safe_sender_(thread_safe_sender), client_(client.release()) {} client.release() -> client.Pass() https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/web_service_worker_provider_impl.cc:33: const WebURL& scriptUrl, nit: scriptUrl -> script_url https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/web_service_worker_provider_impl.h (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/web_service_worker_provider_impl.h:24: namespace content { nit: empty line after namespace content { https://codereview.chromium.org/25008006/diff/298002/content/renderer/render_... File content/renderer/render_view_impl.cc (left): https://codereview.chromium.org/25008006/diff/298002/content/renderer/render_... content/renderer/render_view_impl.cc:117: #include "content/renderer/renderer_webapplicationcachehost_impl.h" Are these changes necessary/related to this CL?
On 2013/10/24 23:25:51, Tom Sepez wrote: > LGTM after addressing the comment. Please don't remove your flag until the CPSP > work is done. > > https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... > content/browser/service_worker/service_worker_dispatcher_host.cc:51: // > TODO(alecflett): add a ServiceWorker-specific policy query in > Please file a follow-up bug for this and link it here. Done. http://crbug.com/311631 for reference.
https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:51: // TODO(alecflett): add a ServiceWorker-specific policy query in On 2013/10/24 23:25:52, Tom Sepez wrote: > Please file a follow-up bug for this and link it here. Done. https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/298002/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.h:30: On 2013/10/25 15:28:00, kinuko wrote: > nit: can we remove this line so that IPC handlers come right after this comment? Done. https://codereview.chromium.org/25008006/diff/298002/content/child/child_thre... File content/child/child_thread.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/child/child_thre... content/child/child_thread.cc:266: channel_->RemoveFilter(service_worker_message_filter_.get()); On 2013/10/25 15:28:00, kinuko wrote: > nit: we usually remove filters in reverse of initialization order Done. https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:20: namespace { On 2013/10/25 15:28:00, kinuko wrote: > nit: can you insert an empty line after this line Done. https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.cc:112: callbacks->onSuccess(worker); On 2013/10/25 15:28:00, kinuko wrote: > nit: I prefer arranging these lines like: > > callbacks->onSuccess(new WebServiceWorkerImpl(...)); > > or > > scoped_ptr<WebServiceWorkerImpl> worker( > new WebServiceWorkerImpl(service_worker_id)); > callbacks->onSuccess(worker.release()); Done. https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_message_filter.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/10/25 15:28:00, kinuko wrote: > nit: Copyright 2013 Done. https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_message_filter.h:13: namespace content { On 2013/10/25 15:28:00, kinuko wrote: > nit: please insert an empty line after namespace content { Done. https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/service_worker_message_filter.h:13: namespace content { On 2013/10/25 15:28:00, kinuko wrote: > nit: please insert an empty line after namespace content { Done. https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/web_service_worker_impl.h (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/web_service_worker_impl.h:17: WebServiceWorkerImpl(int64 service_worker_id) On 2013/10/24 23:03:59, michaeln wrote: > explicit keyword here Done. https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/web_service_worker_provider_impl.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/web_service_worker_provider_impl.cc:27: : thread_safe_sender_(thread_safe_sender), client_(client.release()) {} On 2013/10/25 15:28:00, kinuko wrote: > client.release() -> client.Pass() Done. I'm now fully educated on Pass() vs release() :) https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/web_service_worker_provider_impl.cc:33: const WebURL& scriptUrl, On 2013/10/25 15:28:00, kinuko wrote: > nit: scriptUrl -> script_url Done. https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... File content/child/service_worker/web_service_worker_provider_impl.h (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_wo... content/child/service_worker/web_service_worker_provider_impl.h:24: namespace content { On 2013/10/25 15:28:00, kinuko wrote: > nit: empty line after namespace content { Done. https://codereview.chromium.org/25008006/diff/298002/content/renderer/render_... File content/renderer/render_view_impl.cc (left): https://codereview.chromium.org/25008006/diff/298002/content/renderer/render_... content/renderer/render_view_impl.cc:117: #include "content/renderer/renderer_webapplicationcachehost_impl.h" On 2013/10/25 15:28:00, kinuko wrote: > Are these changes necessary/related to this CL? oops no. Not sure I how I stumbled across these, I'll leave them out of here.
Over to jam. Mind taking a look, specifically for content/ hookups?
https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. why are all these new files in content/child instead of content/renderer? stuff is in content/child because it's used by content/renderer and content/utility, but in this case i thought this code is only going to be called from the renderer?
On 2013/10/28 17:05:50, jam wrote: > https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... > File content/child/service_worker/service_worker_dispatcher.h (right): > > https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... > content/child/service_worker/service_worker_dispatcher.h:1: // Copyright 2013 > The Chromium Authors. All rights reserved. > why are all these new files in content/child instead of content/renderer? stuff > is in content/child because it's used by content/renderer and content/utility, > but in this case i thought this code is only going to be called from the > renderer? this is to be shared with content/workers/, meaning you can call registerServiceWorker() *from* a shared worker, hence its presence in child/ The code that runs in the worker itself in the renderer hasn't yet been written, and yes it goes in content/renderer/. This is *just* the registration API.
lgtm On 2013/10/28 17:16:49, alecflett wrote: > On 2013/10/28 17:05:50, jam wrote: > > > https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... > > File content/child/service_worker/service_worker_dispatcher.h (right): > > > > > https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... > > content/child/service_worker/service_worker_dispatcher.h:1: // Copyright 2013 > > The Chromium Authors. All rights reserved. > > why are all these new files in content/child instead of content/renderer? > stuff > > is in content/child because it's used by content/renderer and content/utility, > > but in this case i thought this code is only going to be called from the > > renderer? > > this is to be shared with content/workers/, meaning you can call > registerServiceWorker() *from* a shared worker, hence its presence in child/ > > The code that runs in the worker itself in the renderer hasn't yet been written, > and yes it goes in content/renderer/. > > This is *just* the registration API. ah, got it. I didn't know that it would be called in workers as well https://codereview.chromium.org/25008006/diff/588001/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/588001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.h:37: int render_process_id_ ALLOW_UNUSED; nit: blank line above this why are you adding ALLOW_UNUSED? this isn't something that's widespread in the code https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.h:26: class ServiceWorkerMessageFilter; nit: order https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.h:29: class ServiceWorkerDispatcher : public webkit_glue::WorkerTaskRunner::Observer { nit: please add a small comment to each new class describing its purpose. also in other files https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.h:37: void RegisterServiceWorker( nit: document methods as well https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... File content/child/service_worker/service_worker_message_filter.cc (right): https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_message_filter.cc:43: ->OnMessageReceived(msg); nit: afaik the convention is to have the "->" on the previous line https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_message_filter.h:18: class CONTENT_EXPORT ServiceWorkerMessageFilter why the content_export?
https://codereview.chromium.org/25008006/diff/588001/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/588001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.h:37: int render_process_id_ ALLOW_UNUSED; On 2013/10/28 17:26:55, jam wrote: > nit: blank line above this > > why are you adding ALLOW_UNUSED? this isn't something that's widespread in the > code not right this moment, but in a future patch. An earlier iteration of this patch included a security check, but we're holding off on that until a little later (see the other TODO's in this patch) https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.h:26: class ServiceWorkerMessageFilter; On 2013/10/28 17:26:55, jam wrote: > nit: order Done. https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_dispatcher.h:37: void RegisterServiceWorker( On 2013/10/28 17:26:55, jam wrote: > nit: document methods as well Done. https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... File content/child/service_worker/service_worker_message_filter.cc (right): https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_message_filter.cc:43: ->OnMessageReceived(msg); On 2013/10/28 17:26:55, jam wrote: > nit: afaik the convention is to have the "->" on the previous line Sorry this passed through 'git cl format' which most of the time looks nice.. I think in this case it simply ran out of line.. (there's no whitespace, yay!) https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/588001/content/child/service_wo... content/child/service_worker/service_worker_message_filter.h:18: class CONTENT_EXPORT ServiceWorkerMessageFilter On 2013/10/28 17:26:55, jam wrote: > why the content_export? for tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/788001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
allow_unused
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/758002
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/1198001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/1348001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/1348001
Message was sent while issue was closed.
Change committed as 231688 |
