Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 2416843002: Implement FetchEvent.navigationPreload (Closed)

Created:
2 years, 8 months ago by horo
Modified:
2 years, 7 months ago
CC:
chromium-reviews, michaeln, mlamouri+watch-content_chromium.org, kenjibaheux+watch_chromium.org, tzik, blink-reviews-api_chromium.org, jsbell+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, blink-reviews-bindings_chromium.org, dglazkov+blink, kinuko+serviceworker, horo+watch_chromium.org, blink-reviews, darin-cc_chromium.org, kinuko+watch, shimazu+serviceworker_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement FetchEvent.navigationPreload This CL depends on https://codereview.chromium.org/2410333006/ FetchEvent.navigationPreload will be implemented in the following steps. [1/4] Introduce FetchEventPreloadHandle to pass the preload response to FetchEvent. https://codereview.chromium.org/2417793002/ [2/4] Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). https://codereview.chromium.org/2410333006/ [3/4] Implement FetchEvent.navigationPreload. https://codereview.chromium.org/2416843002/ This CL. [4/4] Add browser tests for NavigationPreload. https://codereview.chromium.org/2413643005/ If ServiceWorkerContextClient::FetchEventDispatcherImpl receives preload_handle, it creates a PreloadRequest. This PreloadRequest is a mojom::URLLoaderClient owning mojom::URLLoader. ServiceWorkerContextClient::DispatchFetchEvent() stores the PreloadRequest to |context_| and calls ServiceWorkerGlobalScopeProxy::dispatchFetchEvent() with |navigationPreloadSent| flag set. If the flag is set, ServiceWorkerGlobalScopeProxy::dispatchFetchEvent() calls FetchEvent::createPreloadResponseCallback() to create WebServiceWorkerPreloadResponseCallbacks and calls registerPreloadResponseCallback to pass the callback to ServiceWorkerContextClient. ServiceWorkerContextClient::registerPreloadResponseCallback() calls PreloadRequest::RegisterCallback() and the callback will be called when OnStartLoadingResponseBody() is called or OnComplete() is called with an error. BUG=649558 Committed: https://crrev.com/111601216214188fc2ace390da5b2af0095564fd Cr-Commit-Position: refs/heads/master@{#427303}

Patch Set 1 #

Total comments: 10

Patch Set 2 : incorporated falken's comment #

Total comments: 10

Patch Set 3 : rebase #

Total comments: 10

Patch Set 4 : incorporated dcheng and haraken's comment #

Patch Set 5 : stop using nullable promise #

Unified diffs Side-by-side diffs Delta from patch set Stats (+295 lines, -25 lines) Patch
M content/renderer/service_worker/service_worker_context_client.h View 1 2 3 4 chunks +16 lines, -3 lines 0 comments Download
M content/renderer/service_worker/service_worker_context_client.cc View 1 2 3 10 chunks +130 lines, -7 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/service-worker-navigation-preload/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperties.h View 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h View 1 2 3 4 5 chunks +20 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp View 1 2 3 4 5 chunks +64 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/modules/serviceworkers/FetchEvent.idl View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h View 1 2 3 5 chunks +16 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp View 1 2 3 5 chunks +31 lines, -3 lines 0 comments Download
M third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h View 1 2 3 3 chunks +14 lines, -3 lines 0 comments Download

Messages

Total messages: 76 (52 generated)
horo
falken@ Could you please review this?
2 years, 7 months ago (2016-10-19 04:24:32 UTC) #24
falken
I think this looks great. just some nits https://codereview.chromium.org/2416843002/diff/190001/content/renderer/service_worker/service_worker_context_client.cc File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2416843002/diff/190001/content/renderer/service_worker/service_worker_context_client.cc#newcode234 content/renderer/service_worker/service_worker_context_client.cc:234: class ...
2 years, 7 months ago (2016-10-19 06:00:14 UTC) #25
horo
https://codereview.chromium.org/2416843002/diff/190001/content/renderer/service_worker/service_worker_context_client.cc File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2416843002/diff/190001/content/renderer/service_worker/service_worker_context_client.cc#newcode234 content/renderer/service_worker/service_worker_context_client.cc:234: class ServiceWorkerContextClient::PreloadRequest final On 2016/10/19 06:00:13, falken wrote: > ...
2 years, 7 months ago (2016-10-19 07:11:45 UTC) #26
falken
lgtm
2 years, 7 months ago (2016-10-20 02:15:07 UTC) #27
horo
yhirano@ Could you please review third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperties.h and third_party/WebKit/Source/bindings/core/v8/V8Binding.h?
2 years, 7 months ago (2016-10-20 02:20:00 UTC) #29
horo
dcheng@ Could you please review this change as a security reviewer? This CL is the ...
2 years, 7 months ago (2016-10-20 02:51:37 UTC) #31
haraken
https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp#newcode122 third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:122: if (!m_navigationPreloadProperty->getExecutionContext() || You can replace this with: if ...
2 years, 7 months ago (2016-10-20 08:52:45 UTC) #40
yhirano
https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/bindings/core/v8/V8Binding.h File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/bindings/core/v8/V8Binding.h#newcode375 third_party/WebKit/Source/bindings/core/v8/V8Binding.h:375: inline void v8SetReturnValueFast(const CallbackInfo& callbackInfo, Can you tell me ...
2 years, 7 months ago (2016-10-21 01:48:59 UTC) #43
dcheng
https://codereview.chromium.org/2416843002/diff/210001/content/renderer/service_worker/service_worker_context_client.cc File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2416843002/diff/210001/content/renderer/service_worker/service_worker_context_client.cc#newcode293 content/renderer/service_worker/service_worker_context_client.cc:293: if (!callback_) Just to help me understand: why does ...
2 years, 7 months ago (2016-10-21 06:44:24 UTC) #44
horo
https://codereview.chromium.org/2416843002/diff/210001/content/renderer/service_worker/service_worker_context_client.cc File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2416843002/diff/210001/content/renderer/service_worker/service_worker_context_client.cc#newcode293 content/renderer/service_worker/service_worker_context_client.cc:293: if (!callback_) On 2016/10/21 06:44:23, dcheng wrote: > Just ...
2 years, 7 months ago (2016-10-21 10:56:40 UTC) #50
yhirano
https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/bindings/core/v8/V8Binding.h File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/bindings/core/v8/V8Binding.h#newcode375 third_party/WebKit/Source/bindings/core/v8/V8Binding.h:375: inline void v8SetReturnValueFast(const CallbackInfo& callbackInfo, On 2016/10/21 10:56:40, horo ...
2 years, 7 months ago (2016-10-24 07:13:40 UTC) #54
Yuki
https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/bindings/core/v8/V8Binding.h File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/bindings/core/v8/V8Binding.h#newcode375 third_party/WebKit/Source/bindings/core/v8/V8Binding.h:375: inline void v8SetReturnValueFast(const CallbackInfo& callbackInfo, On 2016/10/24 07:13:39, yhirano ...
2 years, 7 months ago (2016-10-24 08:43:21 UTC) #55
horo
On 2016/10/24 08:43:21, Yuki wrote: > https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/bindings/core/v8/V8Binding.h > File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): > > https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Source/bindings/core/v8/V8Binding.h#newcode375 > ...
2 years, 7 months ago (2016-10-24 09:00:11 UTC) #56
Yuki
+domenic@ Domenic, could you give us an advice? The story here is: 1. Fetch API ...
2 years, 7 months ago (2016-10-24 09:25:14 UTC) #58
domenic
On 2016/10/24 at 09:25:14, yukishiino wrote: > +domenic@ > > Domenic, could you give us ...
2 years, 7 months ago (2016-10-24 21:39:35 UTC) #59
horo
On 2016/10/24 21:39:35, domenic wrote: > On 2016/10/24 at 09:25:14, yukishiino wrote: > > +domenic@ ...
2 years, 7 months ago (2016-10-25 06:14:52 UTC) #62
yhirano
bindings/ in PS5 lgtm
2 years, 7 months ago (2016-10-25 06:19:50 UTC) #63
haraken
LGTM on my side
2 years, 7 months ago (2016-10-25 07:41:08 UTC) #66
Yuki
No bindings change, hence LGTM. (Changes in *.idl are fine.)
2 years, 7 months ago (2016-10-25 07:52:13 UTC) #67
Yuki
And, thank you domenic@ for taking care of the spec discussion. It's good to know ...
2 years, 7 months ago (2016-10-25 08:12:00 UTC) #68
dcheng
mojo lgtm
2 years, 7 months ago (2016-10-25 08:23:42 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2416843002/330001
2 years, 7 months ago (2016-10-25 08:54:13 UTC) #72
commit-bot: I haz the power
Committed patchset #5 (id:330001)
2 years, 7 months ago (2016-10-25 09:42:27 UTC) #74
commit-bot: I haz the power
2 years, 7 months ago (2016-10-25 09:47:34 UTC) #76
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/111601216214188fc2ace390da5b2af0095564fd
Cr-Commit-Position: refs/heads/master@{#427303}

Powered by Google App Engine
This is Rietveld 408576698