Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2352173004:
Mojoify FetchEvent of Service Worker. (Closed)
|
Created:
4 years, 3 months ago by horo Modified:
4 years, 2 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, mlamouri+watch-content_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMojoify FetchEvent of Service Worker.
I'd like to use Mojo to send navigation preload response (crbug.com/649558) from
the browser process to the service worker. But currently we are using the legacy
IPC when the browser process sends the FetchEvent to the service worker. There
will be an ordering issue if we use both the legacy IPC and Mojo IPC.
To avoid the ordering issue, this CL changes the FetchEvent IPC to use Mojo.
BUG=629701, 649558
Committed: https://crrev.com/bdc8e1e4477f7dcd911528720a50d639be6d39b5
Cr-Commit-Position: refs/heads/master@{#422996}
Patch Set 1 #Patch Set 2 : fix public_deps #
Total comments: 7
Patch Set 3 : incorporated shimazu's comment #Patch Set 4 : Use EnumTraits #
Total comments: 6
Patch Set 5 : refine .typemap and BUILD.gn #Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : pass the unretained raw pointer of |version_| to the callback #
Total comments: 8
Patch Set 8 : incorporated dcheng's comment #Messages
Total messages: 112 (90 generated)
The CQ bit was checked by horo@chromium.org 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_asan_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 horo@chromium.org 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by horo@chromium.org 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_...)
The CQ bit was checked by horo@chromium.org 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by horo@chromium.org 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.
The CQ bit was checked by horo@chromium.org 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 checked by horo@chromium.org 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 checked by horo@chromium.org 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_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by horo@chromium.org 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 checked by horo@chromium.org 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by horo@chromium.org 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 checked by horo@chromium.org 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 #9 (id:150001) has been deleted
Patchset #8 (id:130001) has been deleted
Patchset #7 (id:110001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== mojofy fetchevent BUG= ========== to ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL change FetchEvent IPC to use Mojo. BUG=629701,649558 ==========
Description was changed from ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL change FetchEvent IPC to use Mojo. BUG=629701,649558 ========== to ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL change FetchEvent IPC to use Mojo. BUG=629701,649558 ==========
Description was changed from ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL change FetchEvent IPC to use Mojo. BUG=629701,649558 ========== to ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL changes the FetchEvent IPC to use Mojo. BUG=629701,649558 ==========
horo@chromium.org changed reviewers: + shimazu@chromium.org
shimazu@ Could you please review this?
https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.h:66: base::Callback<void(blink::mojom::ServiceWorkerEventStatus, Now we can use EnumTraits<blink::mojom::ServiceWorkerEventStatus>. We already have TypeConvertor to ServiceWorkerStatusCode at [1], so I think we can have similar code. Example of typemapping using EnumTraits: https://codesearch.chromium.org/chromium/src/mojo/public/cpp/bindings/tests/s... [1] https://cs.chromium.org/chromium/src/content/common/service_worker/service_wo... https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:75: void OnFetchEventFinished(scoped_refptr<ServiceWorkerVersion> active_version, If I understand correctly, I'm worried that this scoped_refptr<> could make a cyclic reference as below: version -(owns)-> dispatcher -(owns)-> callback -(retains)-> version I think this callback won't be called after SWVersion is destructed, so it's okay to have a raw pointer, or WeakPtr might be fine. WDYT? https://codereview.chromium.org/2352173004/diff/190001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2352173004/diff/190001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:227: mojo::MakeStrongBinding(base::WrapUnique(new FetchEventDispatcherImpl), MakeUnique<> might be better according to this: https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by horo@chromium.org 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/2352173004/diff/190001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.h:66: base::Callback<void(blink::mojom::ServiceWorkerEventStatus, On 2016/09/23 03:26:11, shimazu wrote: > Now we can use EnumTraits<blink::mojom::ServiceWorkerEventStatus>. We already > have TypeConvertor to ServiceWorkerStatusCode at [1], so I think we can have > similar code. > > Example of typemapping using EnumTraits: > https://codesearch.chromium.org/chromium/src/mojo/public/cpp/bindings/tests/s... > > [1] > https://cs.chromium.org/chromium/src/content/common/service_worker/service_wo... Is it possible to convert 'const Callback<void (::blink::mojom::ServiceWorkerEventStatus, base::Time)>' to 'const Callback<void (content::ServiceWorkerStatusCode, base::Time)>' using EnumTraits? https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:75: void OnFetchEventFinished(scoped_refptr<ServiceWorkerVersion> active_version, On 2016/09/23 03:26:11, shimazu wrote: > If I understand correctly, I'm worried that this scoped_refptr<> could make a > cyclic reference as below: > version -(owns)-> dispatcher -(owns)-> callback -(retains)-> version > > I think this callback won't be called after SWVersion is destructed, so it's > okay to have a raw pointer, or WeakPtr might be fine. > > WDYT? Done. https://codereview.chromium.org/2352173004/diff/190001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2352173004/diff/190001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:227: mojo::MakeStrongBinding(base::WrapUnique(new FetchEventDispatcherImpl), On 2016/09/23 03:26:11, shimazu wrote: > MakeUnique<> might be better according to this: > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by horo@chromium.org 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 a nit https://codereview.chromium.org/2352173004/diff/230001/content/common/service... File content/common/service_worker/service_worker_status_code_traits.cc (right): https://codereview.chromium.org/2352173004/diff/230001/content/common/service... content/common/service_worker/service_worker_status_code_traits.cc:27: return blink::mojom::ServiceWorkerEventStatus::ABORTED; Do we have some rules for 'default'? Which pattern is preferred? # I prefer the pattern B = Pattern A = func() { switch (...) { default: return; } // no return } = Pattern B = func() { switch (...) { // no default } return; }
https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2352173004/diff/190001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.h:66: base::Callback<void(blink::mojom::ServiceWorkerEventStatus, On 2016/09/23 05:57:56, horo wrote: > On 2016/09/23 03:26:11, shimazu wrote: > > Now we can use EnumTraits<blink::mojom::ServiceWorkerEventStatus>. We already > > have TypeConvertor to ServiceWorkerStatusCode at [1], so I think we can have > > similar code. > > > > Example of typemapping using EnumTraits: > > > https://codesearch.chromium.org/chromium/src/mojo/public/cpp/bindings/tests/s... > > > > [1] > > > https://cs.chromium.org/chromium/src/content/common/service_worker/service_wo... > > Is it possible to convert 'const Callback<void > (::blink::mojom::ServiceWorkerEventStatus, base::Time)>' to 'const Callback<void > (content::ServiceWorkerStatusCode, base::Time)>' using EnumTraits? Ah, I misunderstood about typemap and EnumTraits. I succeeded to use EnumTraits in Pathc Set 4. https://codereview.chromium.org/2352173004/diff/230001/content/common/service... File content/common/service_worker/service_worker_status_code_traits.cc (right): https://codereview.chromium.org/2352173004/diff/230001/content/common/service... content/common/service_worker/service_worker_status_code_traits.cc:27: return blink::mojom::ServiceWorkerEventStatus::ABORTED; On 2016/09/23 08:15:03, shimazu wrote: > Do we have some rules for 'default'? > Which pattern is preferred? > # I prefer the pattern B > > = Pattern A = > func() { > switch (...) { > default: > return; > } > // no return > } > > = Pattern B = > func() { > switch (...) { > // no default > } > return; > } We need a "default" case. The compiler checks it. ../../content/common/service_worker/service_worker_status_code_traits.cc:18:11: error: 17 enumeration values not handled in switch: 'SERVICE_WORKER_ERROR_FAILED', 'SERVICE_WORKER_ERROR_START_WORKER_FAILED', 'SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND'... [-Werror,-Wswitch] switch (input) { ^
horo@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@ Could you please review content/common/service_worker/fetch_event_dispatcher.mojom
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 horo@chromium.org 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_...)
Sorry for late comment, but let me add some comments. https://codereview.chromium.org/2352173004/diff/230001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/2352173004/diff/230001/content/common/BUILD.g... content/common/BUILD.gn:313: "service_worker/service_worker_status_code_traits.h", I think we can remove these two lines from here because they are specified at the typemap file. https://codereview.chromium.org/2352173004/diff/230001/content/common/service... File content/common/service_worker/fetch_event_dispatcher.typemap (right): https://codereview.chromium.org/2352173004/diff/230001/content/common/service... content/common/service_worker/fetch_event_dispatcher.typemap:13: ] We can use 'source' parameter in typemap instead of adding BUILD.gn. source = ["//content/common/service_worker/service_worker_status_code_traits.cc"]
Ah, I didn't know this. Thank you. https://codereview.chromium.org/2352173004/diff/230001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/2352173004/diff/230001/content/common/BUILD.g... content/common/BUILD.gn:313: "service_worker/service_worker_status_code_traits.h", On 2016/09/27 01:01:07, shimazu wrote: > I think we can remove these two lines from here because they are specified at > the typemap file. Done. https://codereview.chromium.org/2352173004/diff/230001/content/common/service... File content/common/service_worker/fetch_event_dispatcher.typemap (right): https://codereview.chromium.org/2352173004/diff/230001/content/common/service... content/common/service_worker/fetch_event_dispatcher.typemap:13: ] On 2016/09/27 01:01:07, shimazu wrote: > We can use 'source' parameter in typemap instead of adding BUILD.gn. > > source = > ["//content/common/service_worker/service_worker_status_code_traits.cc"] Done.
The CQ bit was checked by horo@chromium.org 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.
On 2016/09/23 09:15:27, horo wrote: > dcheng@ > Could you please review > content/common/service_worker/fetch_event_dispatcher.mojom Ping?
On 2016/09/28 00:04:47, horo wrote: > On 2016/09/23 09:15:27, horo wrote: > > dcheng@ > > Could you please review > > content/common/service_worker/fetch_event_dispatcher.mojom > > Ping?
The CQ bit was checked by horo@chromium.org 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.
Some initial comments. https://codereview.chromium.org/2352173004/diff/270001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2352173004/diff/270001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:80: DCHECK(active_version); Why bind WeakPtr if we just assert that's it's always non null? https://codereview.chromium.org/2352173004/diff/270001/content/common/service... File content/common/service_worker/fetch_event_dispatcher.mojom (right): https://codereview.chromium.org/2352173004/diff/270001/content/common/service... content/common/service_worker/fetch_event_dispatcher.mojom:13: interface FetchEventDispatcher { Is there an overall design doc of how SW will work in Mojo? This seems like a very small interface, and I'm wondering if it would make more sense as part of a larger SW interface?
The CQ bit was checked by horo@chromium.org 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 #7 (id:290001) has been deleted
The CQ bit was checked by horo@chromium.org 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/2352173004/diff/270001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2352173004/diff/270001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:80: DCHECK(active_version); On 2016/09/30 08:54:43, dcheng wrote: > Why bind WeakPtr if we just assert that's it's always non null? Done. Passing the unretained raw pointer seems better. https://codereview.chromium.org/2352173004/diff/270001/content/common/service... File content/common/service_worker/fetch_event_dispatcher.mojom (right): https://codereview.chromium.org/2352173004/diff/270001/content/common/service... content/common/service_worker/fetch_event_dispatcher.mojom:13: interface FetchEventDispatcher { On 2016/09/30 08:54:43, dcheng wrote: > Is there an overall design doc of how SW will work in Mojo? This seems like a > very small interface, and I'm wondering if it would make more sense as part of a > larger SW interface? shimazu@ is working on the Mojofication of SW. BUG: https://crbug.com/629701 design doc: https://docs.google.com/document/d/1DwPpvZG1gTCk5JAULmjPf0eCOHt01Pk59YdwRrbQW... We don't think we can mojofy all SW events so quickly. But the priority of "navigation preload" which needs Mojo is high. (We will use Mojo's datapipe to pass the preloaded response to the SW.) So we want to mojofy FetchEvent before mojofying all SW events. SyncEvent has been already mojofied (crbug.com/479251) So this CL is using the similar approach.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojo lgtm https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.h:245: std::map<int, int> thread_id_embedded_worker_id_map_; Nit: a comment here would be helpful to understand what the key and value types are https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:225: // |dispatcher\ is owned by |version_|. So it is safe to pass the unretained Nit: \ => | https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:228: response_id, *request_.get(), Nit: *request_.get() can be written simply as *request_ https://codereview.chromium.org/2352173004/diff/310001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2352173004/diff/310001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:850: int event_finish_id = Nit: I don't have a better idea, but I found it a little confusing that we have |event_finish_id| in both browser and renderer code, but they aren't the same thing.
The CQ bit was checked by horo@chromium.org 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...
Thank you! https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.h:245: std::map<int, int> thread_id_embedded_worker_id_map_; On 2016/10/04 07:19:53, dcheng wrote: > Nit: a comment here would be helpful to understand what the key and value types > are Done. https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:225: // |dispatcher\ is owned by |version_|. So it is safe to pass the unretained On 2016/10/04 07:19:53, dcheng wrote: > Nit: \ => | Done. https://codereview.chromium.org/2352173004/diff/310001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:228: response_id, *request_.get(), On 2016/10/04 07:19:53, dcheng wrote: > Nit: *request_.get() can be written simply as *request_ Done. https://codereview.chromium.org/2352173004/diff/310001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2352173004/diff/310001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:850: int event_finish_id = On 2016/10/04 07:19:53, dcheng wrote: > Nit: I don't have a better idea, but I found it a little confusing that we have > |event_finish_id| in both browser and renderer code, but they aren't the same > thing. I'm thinking about changing all to fetch_event_id. But I will do so in another CL.
horo@chromium.org changed reviewers: + clamy@chromium.org
clamy@ Could you please review content/common/BUILD.gn?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
content/common/BUIL.gn lgtm
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2352173004/#ps330001 (title: "incorporated dcheng's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL changes the FetchEvent IPC to use Mojo. BUG=629701,649558 ========== to ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL changes the FetchEvent IPC to use Mojo. BUG=629701,649558 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:330001)
Message was sent while issue was closed.
Description was changed from ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL changes the FetchEvent IPC to use Mojo. BUG=629701,649558 ========== to ========== Mojoify FetchEvent of Service Worker. I'd like to use Mojo to send navigation preload response (crbug.com/649558) from the browser process to the service worker. But currently we are using the legacy IPC when the browser process sends the FetchEvent to the service worker. There will be an ordering issue if we use both the legacy IPC and Mojo IPC. To avoid the ordering issue, this CL changes the FetchEvent IPC to use Mojo. BUG=629701,649558 Committed: https://crrev.com/bdc8e1e4477f7dcd911528720a50d639be6d39b5 Cr-Commit-Position: refs/heads/master@{#422996} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bdc8e1e4477f7dcd911528720a50d639be6d39b5 Cr-Commit-Position: refs/heads/master@{#422996}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:330001) has been created in https://codereview.chromium.org/2399933003/ by dcheng@chromium.org. The reason for reverting is: Release blocker crash: https://crbug.com/653403.
Message was sent while issue was closed.
On 2016/10/04 23:25:46, commit-bot: I haz the power wrote: > Patchset 8 (id:??) landed as > https://crrev.com/bdc8e1e4477f7dcd911528720a50d639be6d39b5 > Cr-Commit-Position: refs/heads/master@{#422996} Reverting because this cl appears to be causing crashes in canary, with the following on the stack trace: CRASHED [EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000240 ] 0x00007ff93557e42b (chrome_child.dll -id_map.h:142 ) IDMap<content::PlatformEventObserverBase,1,int>::Lookup(int) 0x00007ff935570db9 (chrome_child.dll -service_worker_context_client.cc:595 ) content::ServiceWorkerContextClient::didHandleFetchEvent(int,blink::WebServiceWorkerEventResult,double) which is code modified in this cl.
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:330001) has been created in https://codereview.chromium.org/2394183003/ by garykac@chromium.org. The reason for reverting is: Crashes in Canary: crbug.com/653403. |
