Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2746783002:
[ServiceWorker] Mojofy InstallEvent of Service Worker (Closed)
|
Created:
3 years, 9 months ago by xiaofengzhang Modified:
3 years, 7 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, viettrungluu+watch_chromium.org, qsr+mojo_chromium.org, mlamouri+watch-content_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, abarth-chromium, Aaron Boodman, kinuko+serviceworker, yzshen+watch_chromium.org, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, darin (slow to review), blink-worker-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ServiceWorker] Mojofy InstallEvent of Service Worker
This CL converts install event IPCs into mojo interface:
ServiceWorkerMsg_InstallEvent
ServiceWorkerHostMsg_InstallEventFinished
Also add an associated interface for the ForeignFetchRegistration,
so that this event be sent on the same message pipe as the service
worker event dispatcher interface, so that we can guarantee the
delivery order of foreign fetch register calls and event dispatcher
interface callbacks.
Also add the test cases for the associated interface.
BUG=629701
TEST=content_unittests
Review-Url: https://codereview.chromium.org/2746783002
Cr-Commit-Position: refs/heads/master@{#468544}
Committed: https://chromium.googlesource.com/chromium/src/+/aeae396182b48115df4170d4e6e61aea9b2e7416
Patch Set 1 #
Total comments: 13
Patch Set 2 : Rebase and address falken, leon and mek's comments #
Total comments: 8
Patch Set 3 : Rebase and address shimazu's comment #33 #
Total comments: 4
Patch Set 4 : Rebase #
Total comments: 36
Patch Set 5 : Address leon's comments #58 #Patch Set 6 : Address shimazu's comment #66 and leon's #69 #
Total comments: 13
Patch Set 7 : Address leon's comment #76 and fix test failure #Patch Set 8 : Rebase 2824193002 #Patch Set 9 : Fix the test failure #
Total comments: 28
Patch Set 10 : Address shimazu's comment #95 and horo's #105 #
Total comments: 1
Patch Set 11 : Just delete a useless line #Messages
Total messages: 134 (95 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
xiaofeng.zhang@intel.com changed reviewers: + falken@chromium.org, shimazu@chromium.org
Hi shimazu, falken there is a test failure: FATAL:service_worker_version.cc(1395)] Check failed: status() == INSTALLING That's because the time sequence between mojo and ipc. The registerForeignFetch should be done during the process of service worker install and before finish installation, but after InstallEvent mojofication, the time sequence can't be guaranteed. So do you have any plan about the mojofication of ServiceWorkerHostMsg_RegisterForeignFetchScope? or other solution for the test failure?
falken@chromium.org changed reviewers: + mek@chromium.org
+mek for foreign fetch. Overall this looks good. I don't have a good answer about the IPC and Mojo ordering breakage for ServiceWorkerHostMsg_RegisterForeignFetchScope. It sounds like you'll have to mojofy that too and guarantee the required ordering. https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.cc:394: callback.Run(SERVICE_WORKER_OK, true, base::Time::Now()); true /* has_fetch_handler */ https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:92: callback.Run(SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED, true, true /* has_fetch_handler */ https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:445: new_version()->FinishRequest(request_id, status == SERVICE_WORKER_OK, By the way, this status == OK bool is not actually used. This is a flaw in our API but it's not the fault of this patch. https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:581: // Aborts the all pending install event callbacks. Can you document why install event is different than the ones below. https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:584: !it.IsAtEnd(); it.Advance()) { I think you can use auto here https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:585: it.GetCurrentValue()->Run(SERVICE_WORKER_ERROR_ABORT, false, false /* has_fetch_hander */
On 2017/03/14 at 08:49:26, falken wrote: > +mek for foreign fetch. > > Overall this looks good. I don't have a good answer about the IPC and Mojo ordering breakage for > ServiceWorkerHostMsg_RegisterForeignFetchScope. It sounds like you'll have to mojofy that too and guarantee the required ordering. Yeah, for that to work correctly we'll have to somehow make sure the install event finished is handled after the register foreign fetch scopes. One way to do that might be to define a new mojom interface for the install event methods (currently just the one method), and pass that as an associated interface to DispatchInstallEvent(). (unrelated, something similar might also make sense for events such as FetchEvent that have multiple reply IPCs: one for the respondWith, and a separate for the end of all waitUntil promises). But there are probably many other equally valid ways to fix the ordering of these messages.
On 2017/03/14 17:35:28, Marijn Kruisselbrink wrote:
> On 2017/03/14 at 08:49:26, falken wrote:
> > +mek for foreign fetch.
> >
> > Overall this looks good. I don't have a good answer about the IPC and Mojo
> ordering breakage for
> > ServiceWorkerHostMsg_RegisterForeignFetchScope. It sounds like you'll have
to
> mojofy that too and guarantee the required ordering.
>
> Yeah, for that to work correctly we'll have to somehow make sure the install
> event finished is handled after the register foreign fetch scopes. One way to
do
> that might be to define a new mojom interface for the install event methods
> (currently just the one method), and pass that as an associated interface to
> DispatchInstallEvent(). (unrelated, something similar might also make sense
for
> events such as FetchEvent that have multiple reply IPCs: one for the
> respondWith, and a separate for the end of all waitUntil promises).
>
> But there are probably many other equally valid ways to fix the ordering of
> these messages.
Hi, mek@, to make sure would you please help to confirm whether my understanding
is correct as bellow? Thanks.
We create another interface
interface ForeignFetchRegistar {
Register(xxx, xxx);
}
and associate it with existing ServiceWorkerEventDispatcher,
then when we dispatch Install event we establish an associated connection of
ForeignFetchRegistar,
which could be used by renderer side to register foreign fetch scope.
On 2017/03/15 at 07:36:29, leon.han wrote:
> On 2017/03/14 17:35:28, Marijn Kruisselbrink wrote:
> > On 2017/03/14 at 08:49:26, falken wrote:
> > > +mek for foreign fetch.
> > >
> > > Overall this looks good. I don't have a good answer about the IPC and Mojo
> > ordering breakage for
> > > ServiceWorkerHostMsg_RegisterForeignFetchScope. It sounds like you'll have
to
> > mojofy that too and guarantee the required ordering.
> >
> > Yeah, for that to work correctly we'll have to somehow make sure the install
> > event finished is handled after the register foreign fetch scopes. One way
to do
> > that might be to define a new mojom interface for the install event methods
> > (currently just the one method), and pass that as an associated interface to
> > DispatchInstallEvent(). (unrelated, something similar might also make sense
for
> > events such as FetchEvent that have multiple reply IPCs: one for the
> > respondWith, and a separate for the end of all waitUntil promises).
> >
> > But there are probably many other equally valid ways to fix the ordering of
> > these messages.
>
> Hi, mek@, to make sure would you please help to confirm whether my
understanding is correct as bellow? Thanks.
>
> We create another interface
> interface ForeignFetchRegistar {
> Register(xxx, xxx);
> }
> and associate it with existing ServiceWorkerEventDispatcher,
> then when we dispatch Install event we establish an associated connection of
ForeignFetchRegistar,
> which could be used by renderer side to register foreign fetch scope.
Yeah, that's pretty much what I was thinking (modulo nitpicks around naming).
Something like:
interface InstallEventMethods {
Register(array<url.mojom.Url> sub_scopes, array<url.mojom.Origin> origins);
};
interface ServiceWorkerEventDispatcher {
DispatchInstallEvent(associated InstallEventMethods& methods)
=> (blink.mojom.ServiceWorkerEventStatus status,
bool has_fetch_handler,
mojo.common.mojom.Time dispatch_event_time);
// Existing stuff
};
Description was changed from ========== [ServiceWorker] Mojofy InstallEvent of Service Worker This CL converts install event IPCs into mojo interface: ServiceWorkerMsg_InstallEvent ServiceWorkerHostMsg_InstallEventFinished BUG=629701 TEST=content_unittests ========== to ========== [ServiceWorker] Mojofy InstallEvent of Service Worker This CL converts install event IPCs into mojo interface: ServiceWorkerMsg_InstallEvent ServiceWorkerHostMsg_InstallEventFinished Also add an associated interface for the ForeignFetchRegistration, so that this event be sent on the same message pipe as the service worker event dispatcher interface, so that we can guarantee the delivery order of foreign fetch register calls and event dispatcher interface callbacks. BUG=629701 TEST=content_unittests ==========
https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.cc:394: callback.Run(SERVICE_WORKER_OK, true, base::Time::Now()); On 2017/03/14 08:49:26, falken wrote: > true /* has_fetch_handler */ Done. https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:92: callback.Run(SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED, true, On 2017/03/14 08:49:26, falken wrote: > true /* has_fetch_handler */ Done. https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:445: new_version()->FinishRequest(request_id, status == SERVICE_WORKER_OK, On 2017/03/14 08:49:26, falken wrote: > By the way, this status == OK bool is not actually used. This is a flaw in our > API but it's not the fault of this patch. thanks, falken, so I should keep it in this patch, right? https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:581: // Aborts the all pending install event callbacks. On 2017/03/14 08:49:26, falken wrote: > Can you document why install event is different than the ones below. Done. https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:584: !it.IsAtEnd(); it.Advance()) { On 2017/03/14 08:49:26, falken wrote: > I think you can use auto here It seems can't. Because InstallEventCallbacksMap is IDMap class, and it doesn't has begin(), etc. https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:585: it.GetCurrentValue()->Run(SERVICE_WORKER_ERROR_ABORT, false, On 2017/03/14 08:49:26, falken wrote: > false /* has_fetch_hander */ Done. https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:585: it.GetCurrentValue()->Run(SERVICE_WORKER_ERROR_ABORT, false, On 2017/03/14 08:49:26, falken wrote: > false /* has_fetch_hander */ Done.
On 2017/04/01 02:27:21, xiaofengzhang wrote: > https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... > File content/browser/service_worker/embedded_worker_test_helper.cc (right): > > https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... > content/browser/service_worker/embedded_worker_test_helper.cc:394: > callback.Run(SERVICE_WORKER_OK, true, base::Time::Now()); > On 2017/03/14 08:49:26, falken wrote: > > true /* has_fetch_handler */ > > Done. > > https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_context_unittest.cc (right): > > https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_context_unittest.cc:92: > callback.Run(SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED, true, > On 2017/03/14 08:49:26, falken wrote: > > true /* has_fetch_handler */ > > Done. > > https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_register_job.cc (right): > > https://codereview.chromium.org/2746783002/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_register_job.cc:445: > new_version()->FinishRequest(request_id, status == SERVICE_WORKER_OK, > On 2017/03/14 08:49:26, falken wrote: > > By the way, this status == OK bool is not actually used. This is a flaw in our > > API but it's not the fault of this patch. > > thanks, falken, so I should keep it in this patch, right? > > https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... > File content/renderer/service_worker/service_worker_context_client.cc (right): > > https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... > content/renderer/service_worker/service_worker_context_client.cc:581: // Aborts > the all pending install event callbacks. > On 2017/03/14 08:49:26, falken wrote: > > Can you document why install event is different than the ones below. > > Done. > > https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... > content/renderer/service_worker/service_worker_context_client.cc:584: > !it.IsAtEnd(); it.Advance()) { > On 2017/03/14 08:49:26, falken wrote: > > I think you can use auto here > It seems can't. Because InstallEventCallbacksMap is IDMap class, and it doesn't > has begin(), etc. > > https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... > content/renderer/service_worker/service_worker_context_client.cc:585: > it.GetCurrentValue()->Run(SERVICE_WORKER_ERROR_ABORT, false, > On 2017/03/14 08:49:26, falken wrote: > > false /* has_fetch_hander */ > > Done. > > https://codereview.chromium.org/2746783002/diff/20001/content/renderer/servic... > content/renderer/service_worker/service_worker_context_client.cc:585: > it.GetCurrentValue()->Run(SERVICE_WORKER_ERROR_ABORT, false, > On 2017/03/14 08:49:26, falken wrote: > > false /* has_fetch_hander */ > > Done. @shimazu, could you help run the test? Leon is on vacation.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Sorry for late coming and thanks for working on this. Leon seems to set the dry-run last week, thanks. I originally thought foreignFetchScopes can be implemented as a method of a new interface which is the counterpart of SWContextClient (ServiceWorkerContextHost in the design doc), but having each methods as an interface seems good. https://codereview.chromium.org/2746783002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:437: DCHECK(!install_method_binding_.is_bound()); https://codereview.chromium.org/2746783002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:444: weak_factory_.GetWeakPtr(), request_id)); I think it's good to create a receiver class like the following in anonymous namespace and pass it to OnInstallFinished in order to make clear when the Binding<> is bound and closed. -- class InstallEventMethodsReceiver : mojom::... { public: std::unique_ptr<InstallEventMethods> CreateAndBind(mojom::InstallEventMethodAssociatedPtrInfo* ptr_info, ...); RegisterForeignFetchScopes(...) override; private: mojo::Binding<...> binding_; } https://codereview.chromium.org/2746783002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2746783002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_version.h:275: void RegisterForeignFetchScopes(const std::vector<GURL>& sub_scopes, Could you move this after RegisterRequestCallback and add a comment like "// Implements mojom::ServiceWorkerInstallEventMethods"? https://codereview.chromium.org/2746783002/diff/60001/content/common/service_... File content/common/service_worker/service_worker_event_dispatcher.mojom (right): https://codereview.chromium.org/2746783002/diff/60001/content/common/service_... content/common/service_worker/service_worker_event_dispatcher.mojom:119: interface InstallEventMethod { Could you rename it to ServiceWorkerInstallEventMethods? It's in the namespace of 'content.mojom', so we should have the 'ServiceWorker' prefix. Also could you add some comment to explain this interface like the following? -- // This interface is passed from the browser to the renderer with DispatchInstallEvent. The ordering between RegisterForeignFetchScopes and the callback of DispatchInstallEvent should be preserved. https://codereview.chromium.org/2746783002/diff/60001/content/common/service_... content/common/service_worker/service_worker_event_dispatcher.mojom:120: Register(array<url.mojom.Url> sub_scopes, array<url.mojom.Origin> origins); Could you rename it to RegisterForeignFetchScopes? https://codereview.chromium.org/2746783002/diff/60001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2746783002/diff/60001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:672: it.GetCurrentValue()->Run(SERVICE_WORKER_ERROR_ABORT, false, I think you can use AbortPendingEventCallbacks for the install event. https://codereview.chromium.org/2746783002/diff/60001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2746783002/diff/60001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.h:338: mojom::InstallEventMethodAssociatedPtr install_method_client_; I think you are trying to have the method interface for each event. In this case, each interface pointers passed through DispatchSomeEvent should be kept by each event instead of directly owned by SWContextClient. I mean it should be mapped to each event id. For implementation, you might add the event_id to the ctor of InstallEvent in blink to return the id back to SWContextClient when registerForeignFetchScopes is called.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
shimazu@ I'm very sorry for late coming. I will refine this patch a.s.a.p. base on all comments. https://codereview.chromium.org/2746783002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2746783002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_version.h:275: void RegisterForeignFetchScopes(const std::vector<GURL>& sub_scopes, On 2017/04/05 05:01:27, shimazu wrote: > Could you move this after RegisterRequestCallback and add a comment like "// > Implements mojom::ServiceWorkerInstallEventMethods"? Nice, thanks. But this function(RegisterForeignFetchScopes) is not the implementation of mojom::ServiceWorkerInstallEventMethods". I just change the name, and in my original patch, it was called in ServiceWorkerRegisterJob::Register.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Almost nits. And seems still have some red bots, let's investigate more. https://codereview.chromium.org/2746783002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/2746783002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.h:17: #include "content/common/service_worker/service_worker_event_dispatcher.mojom.h" We do not need this include here. https://codereview.chromium.org/2746783002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.h:19: #include "mojo/public/cpp/bindings/associated_binding.h" We do not need this include here. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:429: bool handled = true; After install event mojofication this function will become trivial, and some of its related functions will also become trivial. Hi, Shimazu-san, can we keep this as is in this CL and do thorough cleanup with a follow-up CL? https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:90: base::WrapUnique(new InstallEventMethodsReceiver(job)), Use base::MakeUnique instead. https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1141: const int event_id, No need to use 'const'. https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1152: install_methods_map_[event_id]->RegisterForeignFetchScopes(urls, url_origins); Need to DCHECK that install_methods_map_[event_id] exists and is bound. And, can we use 'std::vector<GURL>(sub_scopes.begin(), sub_scopes.end())' here directly instead of a local variable 'urls'? https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1306: int request_id = context_->install_event_callbacks.Add( Better to uniform the naming as 'event_id' with Line.1141? https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1311: mojom::ServiceWorkerInstallEventMethodsAssociatedPtr install_method_client_; install_method_client_ --> install_methods ? https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h (right): https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h:23: const int event_id, No need to use 'const' for int type parameter here and all other related places.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Thanks for moving it forward:) I guess this is still in update, but let me put several comments. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:429: bool handled = true; On 2017/04/23 03:33:10, leonhsl wrote: > After install event mojofication this function will become trivial, and some of > its related functions will also become trivial. > > Hi, Shimazu-san, can we keep this as is in this CL and do thorough cleanup with > a follow-up CL? It's great work! Let's remove no-op functions (inner_sink(), for example) and redundant members like |inner_sink_| in this patch. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_unittest.cc:276: EXPECT_EQ(1UL, helper_->dispatched_events()->size()); ASSERT_EQ to check if enough elements exist before accessing the element. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:82: class ServiceWorkerRegisterJob::InstallEventMethodsReceiver This is not necessary to be an inner class of ServcieWorkerRegisterJob. I think you can implement this in unnamed namespace with passing a raw pointer to the SWVersion in the constructor. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:89: mojo::MakeStrongAssociatedBinding( I feel managing its lifecycle in browser process is better to understand the behavior, so I prefer normal AssociatedBinding for this receiver. WDYT? https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:470: base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished, To clarify InstallEventMethodsReceiver is valid only between DispatchInstallEvent and OnInstallFinished, how about passing the receiver instance to OnInstallFinished and remove it in OnInstallFinished? ex) ... auto install_event_methods = base::MakeUnique<InstallEventMethodsReceiver>(...); new_version()->event_dispatcher()->DispatchInstallEvent( install_event_methods.CreateAndBindInterface(), base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished, weak_factory_.GetWeakPtr(), request_id, base::Passed(&install_event_methods))); } void ServiceWorkerRegisterJob::OnInstallFinished( int request_id, std::unique_ptr<InstallEventMethodsReceiver> instal_event_methods, ...) { install_event_methods.reset(); ... } https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.h:99: class InstallEventMethodsReceiver; This is not necessary to be a member of SWRegisterJob because no reference to it in this header. https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1309: auto it = install_methods_map_.find(request_id); DCHECK(install_methods_map_.find(request_id) == install_methods_map.end()); https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:357: InstallEventMethodsMap install_methods_map_; Do you have any concerns to put this in WorkerContextData? If it's possible to move this in WorkerContextData, you won't need to call install_methods_map_.reset() when aborting. https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h (right): https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h:38: const int event_id, Please remove this const, https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h:40: int event_id_; and let's add const here.
https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:90: base::WrapUnique(new InstallEventMethodsReceiver(job)), On 2017/04/23 03:33:10, leonhsl wrote: > Use base::MakeUnique instead. Done. https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1141: const int event_id, On 2017/04/23 03:33:10, leonhsl wrote: > No need to use 'const'. Done. https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1152: install_methods_map_[event_id]->RegisterForeignFetchScopes(urls, url_origins); On 2017/04/23 03:33:10, leonhsl wrote: > Need to DCHECK that install_methods_map_[event_id] exists and is bound. > > And, can we use 'std::vector<GURL>(sub_scopes.begin(), sub_scopes.end())' here > directly instead of a local variable 'urls'? Done. https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1306: int request_id = context_->install_event_callbacks.Add( On 2017/04/23 03:33:10, leonhsl wrote: > Better to uniform the naming as 'event_id' with Line.1141? Done. https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1311: mojom::ServiceWorkerInstallEventMethodsAssociatedPtr install_method_client_; On 2017/04/23 03:33:10, leonhsl wrote: > install_method_client_ --> install_methods ? Done. https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h (right): https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h:23: const int event_id, On 2017/04/23 03:33:10, leonhsl wrote: > No need to use 'const' for int type parameter here and all other related places. Done.
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:89: mojo::MakeStrongAssociatedBinding( On 2017/04/24 05:00:13, shimazu wrote: > I feel managing its lifecycle in browser process is better to understand the > behavior, so I prefer normal AssociatedBinding for this receiver. > WDYT? In some cases the callback(OnInstallFinished()) of install event may never be called because of unexpected disconnection of EventDispatcher mojo interface. For example, after dispatched Install event, the browser side timer out happened, ServiceWorkerVersion::OnTimeoutTimer() will close EventDispatcher mojo connection, or sometimes renderer process crashed unexpectedly. With StrongAssociatedBinding, the InstallEventMethodsReceiver instance still can be destroyed correctly even in above cases.
https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:429: bool handled = true; On 2017/04/24 05:00:13, shimazu wrote: > On 2017/04/23 03:33:10, leonhsl wrote: > > After install event mojofication this function will become trivial, and some > of > > its related functions will also become trivial. > > > > Hi, Shimazu-san, can we keep this as is in this CL and do thorough cleanup > with > > a follow-up CL? > > It's great work! > Let's remove no-op functions (inner_sink(), for example) and redundant members > like |inner_sink_| in this patch. Hi leon@, shimazu-san@, OnMessageToWorker is still used by TEST_F(ServiceWorkerVersionTest, DispatchEvent) in service_worker_version_unittest.cc to received TestMsg_TestEvent, etc. It's seems better to keeps it or mojofy it in another CL?
https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:429: bool handled = true; On 2017/04/24 05:44:08, xiaofengzhang wrote: > On 2017/04/24 05:00:13, shimazu wrote: > > On 2017/04/23 03:33:10, leonhsl wrote: > > > After install event mojofication this function will become trivial, and some > > of > > > its related functions will also become trivial. > > > > > > Hi, Shimazu-san, can we keep this as is in this CL and do thorough cleanup > > with > > > a follow-up CL? > > > > It's great work! > > Let's remove no-op functions (inner_sink(), for example) and redundant members > > like |inner_sink_| in this patch. > > Hi leon@, shimazu-san@, OnMessageToWorker is still used by > TEST_F(ServiceWorkerVersionTest, DispatchEvent) in > service_worker_version_unittest.cc to received TestMsg_TestEvent, etc. > It's seems better to keeps it or mojofy it in another CL? I feel separating cleanup work is not preferable because it's likely to keep some garbage left behind. Now I checked the code and found the last user of SWVersion::DispatchEvent is InstallEvent. I think SWVersion::DispatchEvent method is also no longer needed, and we can also remove the tests related to that while we should keep a test for SWVersion::RegisterRequestCallback because it's used for FetchEvent. In summary, what we should do here is: - removing SWVersion::DispatchEvent, EWTestHelper::innser_sink() and EWTestHelper::OnMessageToWorker. - removing ServiceWorkerVersionTest::DispatchEvent, ::DispatchConcurrentEvent, ::DispatchEvent_MultipleResponse and DispatchEventToStoppedWorker - adding a test for SWVersion::RegisterRequestCallback. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:89: mojo::MakeStrongAssociatedBinding( On 2017/04/24 05:34:26, leonhsl wrote: > On 2017/04/24 05:00:13, shimazu wrote: > > I feel managing its lifecycle in browser process is better to understand the > > behavior, so I prefer normal AssociatedBinding for this receiver. > > WDYT? > > In some cases the callback(OnInstallFinished()) of install event may never be > called because of unexpected disconnection of EventDispatcher mojo interface. > For example, after dispatched Install event, the browser side timer out > happened, ServiceWorkerVersion::OnTimeoutTimer() will close EventDispatcher mojo > connection, or sometimes renderer process crashed unexpectedly. > With StrongAssociatedBinding, the InstallEventMethodsReceiver instance still can > be destroyed correctly even in above cases. Thanks for explanation! It makes sense. If we have the instance as an unique_ptr in the callback of EventDispatcher, it will be also discarded correctly. It's like the following in short: event_dispatcher->DispatchEvent(base::Bind(&Callback, base::MakeUnique<Receiver>())); I prefer having a receiver instance in the callback because this pattern ensures the lifecycle matches to the event_dispatcher comparing to waiting for a message of "pipe has been closed" asynchronously.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:89: mojo::MakeStrongAssociatedBinding( On 2017/04/24 06:22:18, shimazu wrote: > On 2017/04/24 05:34:26, leonhsl wrote: > > On 2017/04/24 05:00:13, shimazu wrote: > > > I feel managing its lifecycle in browser process is better to understand the > > > behavior, so I prefer normal AssociatedBinding for this receiver. > > > WDYT? > > > > In some cases the callback(OnInstallFinished()) of install event may never be > > called because of unexpected disconnection of EventDispatcher mojo interface. > > For example, after dispatched Install event, the browser side timer out > > happened, ServiceWorkerVersion::OnTimeoutTimer() will close EventDispatcher > mojo > > connection, or sometimes renderer process crashed unexpectedly. > > With StrongAssociatedBinding, the InstallEventMethodsReceiver instance still > can > > be destroyed correctly even in above cases. > > Thanks for explanation! It makes sense. > > If we have the instance as an unique_ptr in the callback of EventDispatcher, it > will be also discarded correctly. > It's like the following in short: > event_dispatcher->DispatchEvent(base::Bind(&Callback, > base::MakeUnique<Receiver>())); > > > I prefer having a receiver instance in the callback because this pattern ensures > the lifecycle matches to the event_dispatcher comparing to waiting for a message > of "pipe has been closed" asynchronously. Oh I see.. I think you're right, even if EventDispatcher mojo interface disconnected unexpectedly, although the install callback can not be called, but mojo will destroy it, so that the base::Passed() unique_ptr could also be destroyed as well. Thanks!
Patchset #5 (id:160001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi shimazu@ leon@ please help to review again, thanks. https://codereview.chromium.org/2746783002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/2746783002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.h:17: #include "content/common/service_worker/service_worker_event_dispatcher.mojom.h" On 2017/04/23 03:33:10, leonhsl wrote: > We do not need this include here. Done. https://codereview.chromium.org/2746783002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.h:19: #include "mojo/public/cpp/bindings/associated_binding.h" On 2017/04/23 03:33:10, leonhsl wrote: > We do not need this include here. Done. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:429: bool handled = true; On 2017/04/24 06:22:18, shimazu wrote: > On 2017/04/24 05:44:08, xiaofengzhang wrote: > > On 2017/04/24 05:00:13, shimazu wrote: > > > On 2017/04/23 03:33:10, leonhsl wrote: > > > > After install event mojofication this function will become trivial, and > some > > > of > > > > its related functions will also become trivial. > > > > > > > > Hi, Shimazu-san, can we keep this as is in this CL and do thorough cleanup > > > with > > > > a follow-up CL? > > > > > > It's great work! > > > Let's remove no-op functions (inner_sink(), for example) and redundant > members > > > like |inner_sink_| in this patch. > > > > Hi leon@, shimazu-san@, OnMessageToWorker is still used by > > TEST_F(ServiceWorkerVersionTest, DispatchEvent) in > > service_worker_version_unittest.cc to received TestMsg_TestEvent, etc. > > It's seems better to keeps it or mojofy it in another CL? > > I feel separating cleanup work is not preferable because it's likely to keep > some garbage left behind. > > Now I checked the code and found the last user of SWVersion::DispatchEvent is > InstallEvent. > I think SWVersion::DispatchEvent method is also no longer needed, and we can > also remove the tests related to that while we should keep a test for > SWVersion::RegisterRequestCallback because it's used for FetchEvent. > > In summary, what we should do here is: > - removing SWVersion::DispatchEvent, EWTestHelper::innser_sink() and > EWTestHelper::OnMessageToWorker. > - removing ServiceWorkerVersionTest::DispatchEvent, ::DispatchConcurrentEvent, > ::DispatchEvent_MultipleResponse and DispatchEventToStoppedWorker > - adding a test for SWVersion::RegisterRequestCallback. Done. Thanks a lot for you detailed explanation! https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_unittest.cc:276: EXPECT_EQ(1UL, helper_->dispatched_events()->size()); On 2017/04/24 05:00:13, shimazu wrote: > ASSERT_EQ to check if enough elements exist before accessing the element. Done. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:82: class ServiceWorkerRegisterJob::InstallEventMethodsReceiver On 2017/04/24 05:00:13, shimazu wrote: > This is not necessary to be an inner class of ServcieWorkerRegisterJob. I think > you can implement this in unnamed namespace with passing a raw pointer to the > SWVersion in the constructor. Done. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:89: mojo::MakeStrongAssociatedBinding( On 2017/04/24 06:37:38, leonhsl wrote: > On 2017/04/24 06:22:18, shimazu wrote: > > On 2017/04/24 05:34:26, leonhsl wrote: > > > On 2017/04/24 05:00:13, shimazu wrote: > > > > I feel managing its lifecycle in browser process is better to understand > the > > > > behavior, so I prefer normal AssociatedBinding for this receiver. > > > > WDYT? > > > > > > In some cases the callback(OnInstallFinished()) of install event may never > be > > > called because of unexpected disconnection of EventDispatcher mojo > interface. > > > For example, after dispatched Install event, the browser side timer out > > > happened, ServiceWorkerVersion::OnTimeoutTimer() will close EventDispatcher > > mojo > > > connection, or sometimes renderer process crashed unexpectedly. > > > With StrongAssociatedBinding, the InstallEventMethodsReceiver instance still > > can > > > be destroyed correctly even in above cases. > > > > Thanks for explanation! It makes sense. > > > > If we have the instance as an unique_ptr in the callback of EventDispatcher, > it > > will be also discarded correctly. > > It's like the following in short: > > event_dispatcher->DispatchEvent(base::Bind(&Callback, > > base::MakeUnique<Receiver>())); > > > > > > I prefer having a receiver instance in the callback because this pattern > ensures > > the lifecycle matches to the event_dispatcher comparing to waiting for a > message > > of "pipe has been closed" asynchronously. > > Oh I see.. I think you're right, even if EventDispatcher mojo interface > disconnected unexpectedly, although the install callback can not be called, but > mojo will destroy it, so that the base::Passed() unique_ptr could also be > destroyed as well. Thanks! Done. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:470: base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished, On 2017/04/24 05:00:13, shimazu wrote: > To clarify InstallEventMethodsReceiver is valid only between > DispatchInstallEvent and OnInstallFinished, how about passing the receiver > instance to OnInstallFinished and remove it in OnInstallFinished? > > ex) > > ... > auto install_event_methods = > base::MakeUnique<InstallEventMethodsReceiver>(...); > new_version()->event_dispatcher()->DispatchInstallEvent( > install_event_methods.CreateAndBindInterface(), > base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished, > weak_factory_.GetWeakPtr(), request_id, > base::Passed(&install_event_methods))); > } > > void ServiceWorkerRegisterJob::OnInstallFinished( > int request_id, > std::unique_ptr<InstallEventMethodsReceiver> instal_event_methods, > ...) { > install_event_methods.reset(); > ... > } Done. https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/2746783002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_register_job.h:99: class InstallEventMethodsReceiver; On 2017/04/24 05:00:13, shimazu wrote: > This is not necessary to be a member of SWRegisterJob because no reference to it > in this header. Done. https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2746783002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:357: InstallEventMethodsMap install_methods_map_; On 2017/04/24 05:00:13, shimazu wrote: > Do you have any concerns to put this in WorkerContextData? > If it's possible to move this in WorkerContextData, you won't need to call > install_methods_map_.reset() when aborting. Done. https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h (right): https://codereview.chromium.org/2746783002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/InstallEvent.h:40: int event_id_; On 2017/04/24 05:00:13, shimazu wrote: > and let's add const here. Done.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with nits. https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (left): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:665: void EmbeddedWorkerTestHelper::OnMessageToWorkerStub( Now this function is useless, too. https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:50: if (!version_) Instead of if-clause here, add a 'DCHECK(version_)' into the ctor? https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_version.h:293: void DispatchEvent(const std::vector<int>& request_ids, Remove this declaration. https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (left): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:42: IPC_MESSAGE_CONTROL2(TestMsg_TestEvent_Multiple, int, int) Remove useless messages. https://codereview.chromium.org/2746783002/diff/200001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2746783002/diff/200001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:222: using InstallEventMethodsMap = Move this declaration into WorkerContextData where it's used?
https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (left): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:665: void EmbeddedWorkerTestHelper::OnMessageToWorkerStub( On 2017/04/25 06:53:27, leonhsl wrote: > Now this function is useless, too. I am wondering if it's better to keep this function because it will assert the worker and thread_id, after all the function still be used by OnMessageReceived which is inherited from OnMessageReceived. https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:50: if (!version_) On 2017/04/25 06:53:27, leonhsl wrote: > Instead of if-clause here, add a 'DCHECK(version_)' into the ctor? Acknowledged. https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_version.h:293: void DispatchEvent(const std::vector<int>& request_ids, On 2017/04/25 06:53:27, leonhsl wrote: > Remove this declaration. sorry, I remembered it was already deleted. :-( https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (left): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:42: IPC_MESSAGE_CONTROL2(TestMsg_TestEvent_Multiple, int, int) On 2017/04/25 06:53:27, leonhsl wrote: > Remove useless messages. Acknowledged. https://codereview.chromium.org/2746783002/diff/200001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2746783002/diff/200001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:222: using InstallEventMethodsMap = On 2017/04/25 06:53:27, leonhsl wrote: > Move this declaration into WorkerContextData where it's used? sorry, I should leave out some changes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:50: if (!version_) On 2017/04/25 06:53:27, leonhsl wrote: > Instead of if-clause here, add a 'DCHECK(version_)' into the ctor? Done. https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2746783002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_version.h:293: void DispatchEvent(const std::vector<int>& request_ids, On 2017/04/25 06:53:27, leonhsl wrote: > Remove this declaration. Done. https://codereview.chromium.org/2746783002/diff/200001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2746783002/diff/200001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:222: using InstallEventMethodsMap = On 2017/04/25 06:53:27, leonhsl wrote: > Move this declaration into WorkerContextData where it's used? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaofeng.zhang@intel.com changed reviewers: + horo@chromium.org, tsepez@chromium.org
PTAL, thanks a lot. Hi, Tom, would you PTAL for OWNER review of mojom files? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #9 (id:260001) has been deleted
lgtm
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry for late. I was in sick leave for the last two days. https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:163: mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo client, Can we have a test for SWInstannEventMethods? https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:44: // mojom::ServiceWorkerInstallEventMethod implementation. nit: We usually use a style of "Implements SomeInterface.". Could you change this to "Implements mojom::ServiceWorkerInstallEventMethods."? https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:468: mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo ptr_info; Why can this solve the test failures? I'm also not sure what's the cause of failures on Bind. I'll also investigate it, but if you have any idea, please let me know:)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:163: mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo client, On 2017/04/27 02:36:44, shimazu wrote: > Can we have a test for SWInstannEventMethods? Sure, I will try do it. https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:44: // mojom::ServiceWorkerInstallEventMethod implementation. On 2017/04/27 02:36:44, shimazu wrote: > nit: We usually use a style of "Implements SomeInterface.". > Could you change this to "Implements mojom::ServiceWorkerInstallEventMethods."? Sure. I refered to service_worker_register_job.h line 146 & 69. https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:468: mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo ptr_info; On 2017/04/27 02:36:44, shimazu wrote: > Why can this solve the test failures? > I'm also not sure what's the cause of failures on Bind. > > I'll also investigate it, but if you have any idea, please let me know:) It's seems the test case passed. We just guessed that if take "install_event_methods->CreateAndBindInterface()" as the parameter of "new_version()->event_dispatcher()->DispatchInstallEvent()", the execution order of it and "base::Passed(&install_event_methods)" maybe not certain?
https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:468: mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo ptr_info; On 2017/04/27 05:15:32, xiaofengzhang wrote: > On 2017/04/27 02:36:44, shimazu wrote: > > Why can this solve the test failures? > > I'm also not sure what's the cause of failures on Bind. > > > > I'll also investigate it, but if you have any idea, please let me know:) > > It's seems the test case passed. > We just guessed that if take "install_event_methods->CreateAndBindInterface()" > as the parameter of "new_version()->event_dispatcher()->DispatchInstallEvent()", > the execution order of it and "base::Passed(&install_event_methods)" maybe not > certain? And do you think it's better to define a static function in InstallEventMethodsReceiver as below: static std::unique_ptr<InstallEventMethodsReceiver> CreateAndBindInterface(ServiceWorkerVersion* version, mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo* ptr_info) { auto receiver_ptr = base::MakeUnique<InstallEventMethodsReceiver>(version); receiver_ptr->install_methods_binding_.Bind(ptr_info); return receiver_ptr; } and use it here: std::unique_ptr<InstallEventMethodsReceiver> install_methods_receiver = InstallEventMethodsReceiver::CreateAndBindInterface(new_version(), &ptr_info);
https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_register_job.cc:468: mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo ptr_info; On 2017/04/27 05:39:27, xiaofengzhang wrote: > On 2017/04/27 05:15:32, xiaofengzhang wrote: > > On 2017/04/27 02:36:44, shimazu wrote: > > > Why can this solve the test failures? > > > I'm also not sure what's the cause of failures on Bind. > > > > > > I'll also investigate it, but if you have any idea, please let me know:) > > > > It's seems the test case passed. > > We just guessed that if take "install_event_methods->CreateAndBindInterface()" > > as the parameter of > "new_version()->event_dispatcher()->DispatchInstallEvent()", > > the execution order of it and "base::Passed(&install_event_methods)" maybe not > > certain? I got it. That seems the problem. > > And do you think it's better to define a static function in > InstallEventMethodsReceiver as below: > > static std::unique_ptr<InstallEventMethodsReceiver> > CreateAndBindInterface(ServiceWorkerVersion* version, > mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo* ptr_info) > { > auto receiver_ptr = base::MakeUnique<InstallEventMethodsReceiver>(version); > receiver_ptr->install_methods_binding_.Bind(ptr_info); > return receiver_ptr; > } > > and use it here: > > std::unique_ptr<InstallEventMethodsReceiver> install_methods_receiver = > InstallEventMethodsReceiver::CreateAndBindInterface(new_version(), &ptr_info); I think it's okay as is.
The name "event_id" is ambiguous for RegisterForeignFetchScopes() methods. I think "install_event_id" is better. https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_register_job.h:24: } nit: } // namespace https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:738: // Aborts the all pending install event callbacks which has three Change AbortPendingEventCallbacks in line 248 to : template <typename T, class... TArgs> void AbortPendingEventCallbacks(T& callbacks, TArgs... args) { for (typename T::iterator it(&callbacks); !it.IsAtEnd(); it.Advance()) { std::move(*it.GetCurrentValue()) .Run(SERVICE_WORKER_ERROR_ABORT, args..., base::Time::Now()); } } And you can call: AbortPendingEventCallbacks(context_->install_event_callbacks, false /* has_fetch_handler */); https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1154: int event_id, nit: install_event_id is more understandable. https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:216: int event_id, nit: install_event_id is more understandable. https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:148: int event_id, nit: install_event_id https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:243: int event_id, nit: install_event_id https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h:129: void RegisterForeignFetchScopes(int event_id, nit: install_event_id https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:74: int event_id, nit: install_event_id https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:315: int event_id, nit: install_event_id
Description was changed from ========== [ServiceWorker] Mojofy InstallEvent of Service Worker This CL converts install event IPCs into mojo interface: ServiceWorkerMsg_InstallEvent ServiceWorkerHostMsg_InstallEventFinished Also add an associated interface for the ForeignFetchRegistration, so that this event be sent on the same message pipe as the service worker event dispatcher interface, so that we can guarantee the delivery order of foreign fetch register calls and event dispatcher interface callbacks. BUG=629701 TEST=content_unittests ========== to ========== [ServiceWorker] Mojofy InstallEvent of Service Worker This CL converts install event IPCs into mojo interface: ServiceWorkerMsg_InstallEvent ServiceWorkerHostMsg_InstallEventFinished Also add an associated interface for the ForeignFetchRegistration, so that this event be sent on the same message pipe as the service worker event dispatcher interface, so that we can guarantee the delivery order of foreign fetch register calls and event dispatcher interface callbacks. Also add the test cases for the associated interface. BUG=629701 TEST=content_unittests ==========
shimazu@ Please help to review again, thanks. https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_job_unittest.cc:1479: dispatched_events()->push_back(Event::Install); Delete it because it's useless for the tests that use EventCallbackHelper. https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_register_job.h:24: } On 2017/04/27 09:22:21, horo wrote: > nit: } // namespace Done. https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:738: // Aborts the all pending install event callbacks which has three On 2017/04/27 09:22:21, horo wrote: > Change AbortPendingEventCallbacks in line 248 to : > template <typename T, class... TArgs> > void AbortPendingEventCallbacks(T& callbacks, TArgs... args) { > for (typename T::iterator it(&callbacks); !it.IsAtEnd(); it.Advance()) { > std::move(*it.GetCurrentValue()) > .Run(SERVICE_WORKER_ERROR_ABORT, args..., base::Time::Now()); > } > } > > And you can call: > AbortPendingEventCallbacks(context_->install_event_callbacks, > false /* has_fetch_handler */); Done. Great! Thanks a lot! https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1154: int event_id, On 2017/04/27 09:22:21, horo wrote: > nit: install_event_id is more understandable. Done. https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2746783002/diff/280001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:216: int event_id, On 2017/04/27 09:22:21, horo wrote: > nit: install_event_id is more understandable. Done. https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:148: int event_id, On 2017/04/27 09:22:21, horo wrote: > nit: install_event_id Done. https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:243: int event_id, On 2017/04/27 09:22:21, horo wrote: > nit: install_event_id Done. https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h:129: void RegisterForeignFetchScopes(int event_id, On 2017/04/27 09:22:21, horo wrote: > nit: install_event_id Done. https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:74: int event_id, On 2017/04/27 09:22:21, horo wrote: > nit: install_event_id Done. https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2746783002/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:315: int event_id, On 2017/04/27 09:22:21, horo wrote: > nit: install_event_id Done.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:300001) has been deleted
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2746783002/diff/280001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:163: mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo client, On 2017/04/27 05:15:32, xiaofengzhang wrote: > On 2017/04/27 02:36:44, shimazu wrote: > > Can we have a test for SWInstannEventMethods? > > Sure, I will try do it. Done. https://codereview.chromium.org/2746783002/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2746783002/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_job_unittest.cc:340: // Must be the same with the defination in embedded_worker_test_helper.cc. This line will be deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. But please get lgtm from shimazu@ before submit.
lgtm, thanks a lot!:)
The CQ bit was checked by xiaofeng.zhang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from leon.han@intel.com, tsepez@chromium.org, horo@chromium.org, shimazu@chromium.org Link to the patchset: https://codereview.chromium.org/2746783002/#ps340001 (title: "Just delete a useless line")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
xiaofeng.zhang@intel.com changed reviewers: + haraken@chromium.org
Hi haraken could you help to review this patch as the owner of third_party/WebKit/Source/web?
On 2017/05/02 01:13:42, xiaofengzhang wrote: > Hi haraken > > could you help to review this patch as the owner of > third_party/WebKit/Source/web? WebKit LGTM
The CQ bit was checked by xiaofeng.zhang@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. This could be a Rietveld flake (see http://crbug.com/401833), so if this CL has no binary files, please re-check commit box. Thanks for your patience. Transient error: timed out
The CQ bit was checked by xiaofeng.zhang@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1493694682736380,
"parent_rev": "22025d9c19bd5ec9a927760b49d46dbdab35a0df", "commit_rev":
"aeae396182b48115df4170d4e6e61aea9b2e7416"}
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Mojofy InstallEvent of Service Worker This CL converts install event IPCs into mojo interface: ServiceWorkerMsg_InstallEvent ServiceWorkerHostMsg_InstallEventFinished Also add an associated interface for the ForeignFetchRegistration, so that this event be sent on the same message pipe as the service worker event dispatcher interface, so that we can guarantee the delivery order of foreign fetch register calls and event dispatcher interface callbacks. Also add the test cases for the associated interface. BUG=629701 TEST=content_unittests ========== to ========== [ServiceWorker] Mojofy InstallEvent of Service Worker This CL converts install event IPCs into mojo interface: ServiceWorkerMsg_InstallEvent ServiceWorkerHostMsg_InstallEventFinished Also add an associated interface for the ForeignFetchRegistration, so that this event be sent on the same message pipe as the service worker event dispatcher interface, so that we can guarantee the delivery order of foreign fetch register calls and event dispatcher interface callbacks. Also add the test cases for the associated interface. BUG=629701 TEST=content_unittests Review-Url: https://codereview.chromium.org/2746783002 Cr-Commit-Position: refs/heads/master@{#468544} Committed: https://chromium.googlesource.com/chromium/src/+/aeae396182b48115df4170d4e6e6... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:340001) as https://chromium.googlesource.com/chromium/src/+/aeae396182b48115df4170d4e6e6... |
