Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416843002:
Implement FetchEvent.navigationPreload (Closed)
|
Created:
4 years, 2 months ago by horo Modified:
4 years, 1 month 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. |
DescriptionImplement 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 #Messages
Total messages: 76 (52 generated)
Description was changed from ========== Implement FetchEvent.navigationPreload This CL depends on https://codereview.chromium.org/2410333006/ BUG=649558 ========== to ========== 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 ==========
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 #1 (id:1) has been deleted
Patchset #1 (id:20001) 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...
Patchset #1 (id:40001) has been deleted
Patchset #5 (id:130001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Patchset #6 (id:170001) has been deleted
Patchset #5 (id:150001) has been deleted
Patchset #4 (id:110001) has been deleted
Patchset #3 (id:90001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:90001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Could you please review this?
I think this looks great. just some nits https://codereview.chromium.org/2416843002/diff/190001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2416843002/diff/190001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:234: class ServiceWorkerContextClient::PreloadRequest final nit: I slightly prefer writing out NavigationPreload to avoid confusion with <link preload>. and other preloads (maybe too late for this as there's already FetchEventPreloadHandle) https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:112: explicit PreloadResponseCallbackImpl( nit: dont need explicit https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:128: virtual void registerPreloadResponseCallback( I think it's time to break the pattern of no comments in this file :) Can you add a brief comment like "Registers the callback used to settle the FetchEvent.navigationPreload promise"? https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:130: WebServiceWorkerPreloadResponseCallbacks*) = 0; Could this be std::unique_ptr (I think previously we couldn't until yutak's work migrating OwnPtr to unique_ptr) https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:195: // navigationPreloadSent flag set to register the preload responce callback. nit: response
https://codereview.chromium.org/2416843002/diff/190001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2416843002/diff/190001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:234: class ServiceWorkerContextClient::PreloadRequest final On 2016/10/19 06:00:13, falken wrote: > nit: I slightly prefer writing out NavigationPreload to avoid confusion with > <link preload>. and other preloads (maybe too late for this as there's already > FetchEventPreloadHandle) Done. https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:112: explicit PreloadResponseCallbackImpl( On 2016/10/19 06:00:13, falken wrote: > nit: dont need explicit Done. https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:128: virtual void registerPreloadResponseCallback( On 2016/10/19 06:00:13, falken wrote: > I think it's time to break the pattern of no comments in this file :) > Can you add a brief comment like "Registers the callback used to settle the > FetchEvent.navigationPreload promise"? Done. https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:130: WebServiceWorkerPreloadResponseCallbacks*) = 0; On 2016/10/19 06:00:13, falken wrote: > Could this be std::unique_ptr (I think previously we couldn't until yutak's work > migrating OwnPtr to unique_ptr) Done. https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2416843002/diff/190001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:195: // navigationPreloadSent flag set to register the preload responce callback. On 2016/10/19 06:00:13, falken wrote: > nit: response Done.
lgtm
horo@chromium.org changed reviewers: + yhirano@chromium.org
yhirano@ Could you please review third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperties.h and third_party/WebKit/Source/bindings/core/v8/V8Binding.h?
horo@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@ Could you please review this change as a security reviewer? This CL is the renderer process side implementation of FetchEvent.navigationPreload.
Patchset #3 (id:230001) 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...
Patchset #3 (id:250001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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...
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:122: if (!m_navigationPreloadProperty->getExecutionContext() || You can replace this with: if (!m_scriptState->contextIsValid()) return; https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:151: if (!m_navigationPreloadProperty->getExecutionContext() || Ditto. if (!m_scriptState->contextIsValid()) return; https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:161: Persistent<FetchEvent::PreloadResponseProperty> m_navigationPreloadProperty; I'm a bit concerned if this Persistent handle may cause a reference cycle if the ScriptPromiseProperty holds something that has a strong reference back to the FetchEvent. Can we move PreloadResponseCallbackImpl to the Oilpan's heap and use Member here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:375: inline void v8SetReturnValueFast(const CallbackInfo& callbackInfo, Can you tell me why this is needed?
https://codereview.chromium.org/2416843002/diff/210001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2416843002/diff/210001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:293: if (!callback_) Just to help me understand: why does this need to check |callback_| but OnStartLoadingResponseBody can DCHECK? https://codereview.chromium.org/2416843002/diff/210001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2416843002/diff/210001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:198: int eventID, Nit: event_id https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:376: blink::ScriptPromise promise, Should this be const ScriptPromise&? https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:138: for (HTTPHeaderMap::const_iterator i = result->response->headers().begin(), Nit: for (const auto& header : result->response->headers()) responseData->headerList()->append(header.key, header.value); https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h (right): https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h:34: typedef ScriptPromiseProperty<Member<FetchEvent>, Nit: prefer `using A = B` over `typedef B A` in new code.
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 #4 (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/2416843002/diff/210001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2416843002/diff/210001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:293: if (!callback_) On 2016/10/21 06:44:23, dcheng wrote: > Just to help me understand: why does this need to check |callback_| but > OnStartLoadingResponseBody can DCHECK? If an error happens before receiving the HTTP response header, only OnComplete() is called. Otherwise both OnStartLoadingResponseBody() and OnComplete() are called. Added comment. https://codereview.chromium.org/2416843002/diff/210001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2416843002/diff/210001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:198: int eventID, On 2016/10/21 06:44:23, dcheng wrote: > Nit: event_id Done. https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:376: blink::ScriptPromise promise, On 2016/10/21 06:44:23, dcheng wrote: > Should this be const ScriptPromise&? Done. https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:138: for (HTTPHeaderMap::const_iterator i = result->response->headers().begin(), On 2016/10/21 06:44:23, dcheng wrote: > Nit: > for (const auto& header : result->response->headers()) > responseData->headerList()->append(header.key, header.value); Done. https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h (right): https://codereview.chromium.org/2416843002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h:34: typedef ScriptPromiseProperty<Member<FetchEvent>, On 2016/10/21 06:44:23, dcheng wrote: > Nit: prefer `using A = B` over `typedef B A` in new code. Done. https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:375: inline void v8SetReturnValueFast(const CallbackInfo& callbackInfo, On 2016/10/21 01:48:58, yhirano wrote: > Can you tell me why this is needed? navigationPreloadAttributeGetter() in V8FetchEvent.cpp which is generated from FetchEvent.idl requires this method. static void navigationPreloadAttributeGetter(const v8::FunctionCallbackInfo<v8::Value>& info) { v8::Local<v8::Object> holder = info.Holder(); FetchEvent* impl = V8FetchEvent::toImpl(holder); ScriptState* scriptState = ScriptState::forReceiverObject(info); bool isNull = false; ScriptPromise cppValue(impl->navigationPreload(scriptState, isNull)); if (isNull) { v8SetReturnValueNull(info); return; } v8SetReturnValueFast(info, cppValue, impl); } https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:122: if (!m_navigationPreloadProperty->getExecutionContext() || On 2016/10/20 08:52:45, haraken wrote: > > You can replace this with: > > if (!m_scriptState->contextIsValid()) > return; Done. https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:151: if (!m_navigationPreloadProperty->getExecutionContext() || On 2016/10/20 08:52:45, haraken wrote: > > Ditto. > > if (!m_scriptState->contextIsValid()) > return; Done. https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:161: Persistent<FetchEvent::PreloadResponseProperty> m_navigationPreloadProperty; On 2016/10/20 08:52:45, haraken wrote: > > I'm a bit concerned if this Persistent handle may cause a reference cycle if the > ScriptPromiseProperty holds something that has a strong reference back to the > FetchEvent. > > Can we move PreloadResponseCallbackImpl to the Oilpan's heap and use Member > here? Done. I introduced m_pendingPreloadFetchEvents in ServiceWorkerGlobalScopeProxy.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yhirano@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:375: inline void v8SetReturnValueFast(const CallbackInfo& callbackInfo, On 2016/10/21 10:56:40, horo wrote: > On 2016/10/21 01:48:58, yhirano wrote: > > Can you tell me why this is needed? > > > navigationPreloadAttributeGetter() in V8FetchEvent.cpp which is generated from > FetchEvent.idl requires this method. > > static void navigationPreloadAttributeGetter(const > v8::FunctionCallbackInfo<v8::Value>& info) > { > v8::Local<v8::Object> holder = info.Holder(); > > FetchEvent* impl = V8FetchEvent::toImpl(holder); > > ScriptState* scriptState = ScriptState::forReceiverObject(info); > bool isNull = false; > > ScriptPromise cppValue(impl->navigationPreload(scriptState, isNull)); > > if (isNull) { > v8SetReturnValueNull(info); > return; > } > > v8SetReturnValueFast(info, cppValue, impl); > } > +yukishiino@chromium.org I think this is a bug in the generator; nullable Promise is treated as ScriptWrappable. yukishiino@, what do you think?
https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:375: inline void v8SetReturnValueFast(const CallbackInfo& callbackInfo, On 2016/10/24 07:13:39, yhirano wrote: > On 2016/10/21 10:56:40, horo wrote: > > On 2016/10/21 01:48:58, yhirano wrote: > > > Can you tell me why this is needed? > > > > > > navigationPreloadAttributeGetter() in V8FetchEvent.cpp which is generated from > > FetchEvent.idl requires this method. > > > > static void navigationPreloadAttributeGetter(const > > v8::FunctionCallbackInfo<v8::Value>& info) > > { > > v8::Local<v8::Object> holder = info.Holder(); > > > > FetchEvent* impl = V8FetchEvent::toImpl(holder); > > > > ScriptState* scriptState = ScriptState::forReceiverObject(info); > > bool isNull = false; > > > > ScriptPromise cppValue(impl->navigationPreload(scriptState, isNull)); > > > > if (isNull) { > > v8SetReturnValueNull(info); > > return; > > } > > > > v8SetReturnValueFast(info, cppValue, impl); > > } > > > > mailto:+yukishiino@chromium.org > > I think this is a bug in the generator; nullable Promise is treated as > ScriptWrappable. yukishiino@, what do you think? Is this spec-conformant? I've never hard of nullable-Promise, and it looks a little weird. For DOM operations, they must be either of promise-returning function or non-promise-returning function. If it's a promise returning function, an exception must be converted to a reject promise. Having said that, there is no such rule for DOM attributes. Also it's not prohibited to make a promise type nullable seeing the following spec. https://heycam.github.io/webidl/#idl-nullable-type However, I'm afraid that it's just because the spec is not consistent nor up-to-date. It's possible that someone forgot to update the definition of "nullable" when they introduced promise type. Are you sure that it's okay to introduce a nullable promise type? There is no such definition at https://www.w3.org/TR/service-workers/#fetch-event-section
On 2016/10/24 08:43:21, Yuki wrote: > https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): > > https://codereview.chromium.org/2416843002/diff/270001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/V8Binding.h:375: inline void > v8SetReturnValueFast(const CallbackInfo& callbackInfo, > On 2016/10/24 07:13:39, yhirano wrote: > > On 2016/10/21 10:56:40, horo wrote: > > > On 2016/10/21 01:48:58, yhirano wrote: > > > > Can you tell me why this is needed? > > > > > > > > > navigationPreloadAttributeGetter() in V8FetchEvent.cpp which is generated > from > > > FetchEvent.idl requires this method. > > > > > > static void navigationPreloadAttributeGetter(const > > > v8::FunctionCallbackInfo<v8::Value>& info) > > > { > > > v8::Local<v8::Object> holder = info.Holder(); > > > > > > FetchEvent* impl = V8FetchEvent::toImpl(holder); > > > > > > ScriptState* scriptState = ScriptState::forReceiverObject(info); > > > bool isNull = false; > > > > > > ScriptPromise cppValue(impl->navigationPreload(scriptState, isNull)); > > > > > > if (isNull) { > > > v8SetReturnValueNull(info); > > > return; > > > } > > > > > > v8SetReturnValueFast(info, cppValue, impl); > > > } > > > > > > > mailto:+yukishiino@chromium.org > > > > I think this is a bug in the generator; nullable Promise is treated as > > ScriptWrappable. yukishiino@, what do you think? > > Is this spec-conformant? I've never hard of nullable-Promise, and it looks a > little weird. > > For DOM operations, they must be either of promise-returning function or > non-promise-returning function. If it's a promise returning function, an > exception must be converted to a reject promise. > > Having said that, there is no such rule for DOM attributes. Also it's not > prohibited to make a promise type nullable seeing the following spec. > https://heycam.github.io/webidl/#idl-nullable-type > However, I'm afraid that it's just because the spec is not consistent nor > up-to-date. It's possible that someone forgot to update the definition of > "nullable" when they introduced promise type. > > Are you sure that it's okay to introduce a nullable promise type? There is no > such definition at > https://www.w3.org/TR/service-workers/#fetch-event-section This feature is not in the spec yet. It is still under discussion. https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 We are speculatively implementing this behind "--enable-features=ServiceWorkerNavigationPreload" flag. To support this code in the spec discussion, |navigationPreload| must return a nullable promise. ------------------------ self.addEventListener('fetch', event => { event.respondWith(event.navigationPreload || fetch(event.request)); }); ------------------------
yukishiino@chromium.org changed reviewers: + domenic@chromium.org
+domenic@ Domenic, could you give us an advice? The story here is: 1. Fetch API is about to introduce a new DOM attribute that returns a **nullable promise type**. DIscussions are here: https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 2. AFAIK, this is the first time to introduce a nullable promise type. 3. In case of DOM operations, we have a rule that we convert any exception into a reject promise if the return type is a promise type. https://heycam.github.io/webidl/#dfn-create-operation-function 1. If the operation has a return type that is a promise type, then: 1. Let reject be the initial value of %Promise%.reject. 2. Return the result of calling reject with %Promise% as the this object and the exception as the single argument value. But we don't have the same rule for DOM attribute for now. 4. It's not prohibited to make a promise type nullable. https://heycam.github.io/webidl/#idl-nullable-type So, it seems technically okay to introduce a nullable promise type, but I'm personally very uncomfortable to introduce a nullable promise type because it's inconsistent with DOM operations, and there will be no chance to introduce the same rule to DOM attributes. What do you think of a nullable promise type? I'm just too concerned or misunderstanding? If nullable promise is okay, then Promise<T>? shoudl be considered as "return type is a promise type" or not?
On 2016/10/24 at 09:25:14, yukishiino wrote: > +domenic@ > > Domenic, could you give us an advice? > > The story here is: > > 1. Fetch API is about to introduce a new DOM attribute that returns a **nullable promise type**. > DIscussions are here: > https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 > 2. AFAIK, this is the first time to introduce a nullable promise type. > 3. In case of DOM operations, we have a rule that we convert any exception into a reject promise if the return type is a promise type. > https://heycam.github.io/webidl/#dfn-create-operation-function > 1. If the operation has a return type that is a promise type, then: > 1. Let reject be the initial value of %Promise%.reject. > 2. Return the result of calling reject with %Promise% as the this object and the exception as the single argument value. > But we don't have the same rule for DOM attribute for now. > 4. It's not prohibited to make a promise type nullable. > https://heycam.github.io/webidl/#idl-nullable-type > > So, it seems technically okay to introduce a nullable promise type, but I'm personally very uncomfortable to introduce a nullable promise type because it's inconsistent with DOM operations, and there will be no chance to introduce the same rule to DOM attributes. > > What do you think of a nullable promise type? I'm just too concerned or misunderstanding? > > If nullable promise is okay, then Promise<T>? shoudl be considered as "return type is a promise type" or not? Oh, this is not good. We have an open Web IDL issue to prohibit nullable promises (i.e. remove Promise<T>? from the grammar entirely). https://www.w3.org/Bugs/Public/show_bug.cgi?id=25049 Regarding attribute vs. operation, we are hoping to make those compatible. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=25048. Let me try to get involved in the service worker spec discussion.
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...
On 2016/10/24 21:39:35, domenic wrote: > On 2016/10/24 at 09:25:14, yukishiino wrote: > > +domenic@ > > > > Domenic, could you give us an advice? > > > > The story here is: > > > > 1. Fetch API is about to introduce a new DOM attribute that returns a > **nullable promise type**. > > DIscussions are here: > > https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 > > 2. AFAIK, this is the first time to introduce a nullable promise type. > > 3. In case of DOM operations, we have a rule that we convert any exception > into a reject promise if the return type is a promise type. > > https://heycam.github.io/webidl/#dfn-create-operation-function > > 1. If the operation has a return type that is a promise type, then: > > 1. Let reject be the initial value of %Promise%.reject. > > 2. Return the result of calling reject with %Promise% as the this > object and the exception as the single argument value. > > But we don't have the same rule for DOM attribute for now. > > 4. It's not prohibited to make a promise type nullable. > > https://heycam.github.io/webidl/#idl-nullable-type > > > > So, it seems technically okay to introduce a nullable promise type, but I'm > personally very uncomfortable to introduce a nullable promise type because it's > inconsistent with DOM operations, and there will be no chance to introduce the > same rule to DOM attributes. > > > > What do you think of a nullable promise type? I'm just too concerned or > misunderstanding? > > > > If nullable promise is okay, then Promise<T>? shoudl be considered as "return > type is a promise type" or not? > > Oh, this is not good. We have an open Web IDL issue to prohibit nullable > promises (i.e. remove Promise<T>? from the grammar entirely). > https://www.w3.org/Bugs/Public/show_bug.cgi?id=25049 > > Regarding attribute vs. operation, we are hoping to make those compatible. See > https://www.w3.org/Bugs/Public/show_bug.cgi?id=25048. > > Let me try to get involved in the service worker spec discussion. Uploaded PatchSet 5. I stopped using nullable promise and added comment in FetchEvent.cpp.
bindings/ in PS5 lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM on my side
No bindings change, hence LGTM. (Changes in *.idl are fine.)
And, thank you domenic@ for taking care of the spec discussion. It's good to know that we're going to remove nullable promises and to make attributes and operations consistent.
mojo lgtm
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/2416843002/#ps330001 (title: "stop using nullable promise")
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 ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:330001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/111601216214188fc2ace390da5b2af0095564fd Cr-Commit-Position: refs/heads/master@{#427303} |
