Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1102363002:
Add Client Attribute to FetchEvent- Blink Side. (Closed)
|
Created:
5 years, 7 months ago by Paritosh Kumar Modified:
5 years, 7 months ago CC:
blink-reviews, serviceworker-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd Client Attribute to FetchEvent- Blink Side.
..per https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-interface, FetchEvent Interface should have a Client Attribute.
Intent to Implement and Ship Link: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/fetchevent/blink-dev/naYY_V77Amo/zd4v_jwWlvsJ
[PATCH 1]: Blink Side: This Patch
[Patch 2]: Chromium Side: https://codereview.chromium.org/1108273002/
[PATCH 3]: Layout Tests: https://codereview.chromium.org/1138223002/
BUG=470036
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195398
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #
Total comments: 19
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Messages
Total messages: 24 (5 generated)
paritosh.in@samsung.com changed reviewers: + falken@chromium.org, jsbell@chromium.org
This is Blink Side change for adding Client Attribute to FetchEvent Interface. Please have a look.
Do you have CLs for the Chromium-side changes and the Blink-side tests? https://codereview.chromium.org/1102363002/diff/1/Source/web/ServiceWorkerGlo... File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right): https://codereview.chromium.org/1102363002/diff/1/Source/web/ServiceWorkerGlo... Source/web/ServiceWorkerGlobalScopeProxy.cpp:108: eventInit.setClient(ServiceWorkerClient::create(webRequest.client())); This should create a ServiceWorkerWindowClient if clientType is WebServiceWorkerClientTypeWindow, shouldn't it? Maybe factor out a helper function from ClientArray::take()?
Agree with jsbell's comments. I'd like to see the chromium-side patch and blink-side tests before landing anything
On 2015/04/30 07:33:04, falken wrote: > Agree with jsbell's comments. I'd like to see the chromium-side patch and > blink-side tests before landing anything Thanks falken & jsbell I'm working on chromium side changes and blink tests. Will update soon on this. https://codereview.chromium.org/1108273002/ is a WIP patch for chromium side changes.
Thanks jsbell and falken Sorry for delay in update. Updated cl with a test for client attribute of fetch event. Have a chromium side cl here https://codereview.chromium.org/1108273002/ Please have a look. https://codereview.chromium.org/1102363002/diff/1/Source/web/ServiceWorkerGlo... File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right): https://codereview.chromium.org/1102363002/diff/1/Source/web/ServiceWorkerGlo... Source/web/ServiceWorkerGlobalScopeProxy.cpp:108: eventInit.setClient(ServiceWorkerClient::create(webRequest.client())); Yes.. Done.
https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:9: function test() { Please do not override the `test` function provided by testharness.js https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:13: async_test(function(t) { You should be able to use service_worker_test() to simplify this dramatically. See skip-waiting-without-client.html and resources/skip-waiting-worker.js for an example. If that is insufficient (due to not having an appropriate fetch) then please at least use promise_test. https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js (right): https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js:2: event.respondWith(new Response((event.client.frameType).toString())); The only assertion in the test is looking at data.expected and data.actual; since the message data here is a string, both of those will be undefined, and so pass. So this is not actually validating anything. https://codereview.chromium.org/1102363002/diff/20001/Source/web/ServiceWorke... File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right): https://codereview.chromium.org/1102363002/diff/20001/Source/web/ServiceWorke... Source/web/ServiceWorkerGlobalScopeProxy.cpp:113: eventInit.setIsReload(false); Is this necessary, or will it default to false?
Thanks jsbell Addressed your comments, please have a look. https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:9: function test() { On 2015/05/05 17:45:42, jsbell wrote: > Please do not override the `test` function provided by testharness.js Ohh sorry for noise, this was useless method. https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:13: async_test(function(t) { Using primise_test, as do not have an appropriate fetch. https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js (right): https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js:2: event.respondWith(new Response((event.client.frameType).toString())); On 2015/05/05 17:45:42, jsbell wrote: > The only assertion in the test is looking at data.expected and data.actual; > since the message data here is a string, both of those will be undefined, and so > pass. So this is not actually validating anything. We are using an assertion in promise_test as: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); And this return value will be passed to client frame. https://codereview.chromium.org/1102363002/diff/20001/Source/web/ServiceWorke... File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right): https://codereview.chromium.org/1102363002/diff/20001/Source/web/ServiceWorke... Source/web/ServiceWorkerGlobalScopeProxy.cpp:113: eventInit.setIsReload(false); On 2015/05/05 17:45:42, jsbell wrote: > Is this necessary, or will it default to false? Yes, default will be false only. Not necessary. Thanks.
Can you add a test with a non-window client to verify that a non-WindowClient instance is created in that case? https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js (right): https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js:2: event.respondWith(new Response((event.client.frameType).toString())); On 2015/05/06 11:42:36, Paritosh Kumar wrote: > On 2015/05/05 17:45:42, jsbell wrote: > > The only assertion in the test is looking at data.expected and data.actual; > > since the message data here is a string, both of those will be undefined, and > so > > pass. So this is not actually validating anything. > > We are using an assertion in promise_test as: > assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType > should be top-level"); > And this return value will be passed to client frame. Got it, thanks for the explanation! I was looking at the unused assert. This makes sense! https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); Please wrap to 80 columns (e.g. break after the "top-level", argument) https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); Interesting... given the table in fetch.spec.whatwg.org I would have expected the context frame type to be "nested" for an iframe. Is this a bug in our implementation of fetch? If so, fine to leave this as-is. Likely I'm just misunderstanding some subtlety in fetch and sw specs. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js:2: event.respondWith(new Response((event.client.frameType).toString())); Is the (...).toString() part necessary? https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js:85: false, 'Default FetchEvent.bubbles should be false'); Thanks for fixing this message! https://codereview.chromium.org/1102363002/diff/40001/public/platform/WebServ... File public/platform/WebServiceWorkerRequest.h (right): https://codereview.chromium.org/1102363002/diff/40001/public/platform/WebServ... public/platform/WebServiceWorkerRequest.h:80: void setClient(WebServiceWorkerClientInfo); Can this be a const ref? https://codereview.chromium.org/1102363002/diff/40001/public/platform/WebServ... public/platform/WebServiceWorkerRequest.h:81: WebServiceWorkerClientInfo client() const; This could be a const ref as well
looking good, I'm assuming the tests will have to be landed in a separate patch after the chromium patch lands? https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); On 2015/05/06 16:21:26, jsbell wrote: > Interesting... given the table in http://fetch.spec.whatwg.org I would have expected > the context frame type to be "nested" for an iframe. > > Is this a bug in our implementation of fetch? If so, fine to leave this as-is. > Likely I'm just misunderstanding some subtlety in fetch and sw specs. Can you file a bug if it's an existing bug? https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:19: add_completion_callback(function() { I don't think you need this since you call service_worker_unregister_and_done() https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:24: }, 'Service Worker responds to fetch event with client frameType'); this indentation is 2 spaces too many https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js:3: }); indent this two spaces
Thanks jsbell and falken Please have a look. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); On 2015/05/07 04:06:03, falken wrote: > On 2015/05/06 16:21:26, jsbell wrote: > > Interesting... given the table in http://fetch.spec.whatwg.org I would have > expected > > the context frame type to be "nested" for an iframe. > > > > Is this a bug in our implementation of fetch? If so, fine to leave this as-is. > > Likely I'm just misunderstanding some subtlety in fetch and sw specs. > > Can you file a bug if it's an existing bug? I'm not updating on this now, first we will have to test it by sending it to 'event.client' using postMessage from worker that we should receive in frame window, from there we can check it. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:19: add_completion_callback(function() { On 2015/05/07 04:06:03, falken wrote: > I don't think you need this since you call service_worker_unregister_and_done() Yes, Not Required. Thanks. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:24: }, 'Service Worker responds to fetch event with client frameType'); On 2015/05/07 04:06:03, falken wrote: > this indentation is 2 spaces too many Done. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js:2: event.respondWith(new Response((event.client.frameType).toString())); On 2015/05/06 16:21:26, jsbell wrote: > Is the (...).toString() part necessary? No. Done. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-event-client-attribute-test-worker.js:3: }); On 2015/05/07 04:06:03, falken wrote: > indent this two spaces Done. https://codereview.chromium.org/1102363002/diff/40001/public/platform/WebServ... File public/platform/WebServiceWorkerRequest.h (right): https://codereview.chromium.org/1102363002/diff/40001/public/platform/WebServ... public/platform/WebServiceWorkerRequest.h:80: void setClient(WebServiceWorkerClientInfo); On 2015/05/06 16:21:26, jsbell wrote: > Can this be a const ref? Yes. https://codereview.chromium.org/1102363002/diff/40001/public/platform/WebServ... public/platform/WebServiceWorkerRequest.h:81: WebServiceWorkerClientInfo client() const; On 2015/05/06 16:21:26, jsbell wrote: > This could be a const ref as well Yes. Thanks.
I'm still confused about how to land this. Does this patch's test pass without the Chromium patch? If so, you'd need additional tests after the Chromium patch to test that the Chromium patch is correct. If not, it'll have to land in a separate patch after the Chromium patch so the tree isn't broken. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); On 2015/05/07 16:46:54, Paritosh Kumar wrote: > On 2015/05/07 04:06:03, falken wrote: > > On 2015/05/06 16:21:26, jsbell wrote: > > > Interesting... given the table in http://fetch.spec.whatwg.org I would have > > expected > > > the context frame type to be "nested" for an iframe. > > > > > > Is this a bug in our implementation of fetch? If so, fine to leave this > as-is. > > > Likely I'm just misunderstanding some subtlety in fetch and sw specs. > > > > Can you file a bug if it's an existing bug? > > I'm not updating on this now, first we will have to test it by sending it to > 'event.client' using > postMessage from worker that we should receive in frame window, from there we > can check it. Can you elaborate on this? It looks like you're testing it in this line because the SW populates body with event.client.frameType. I'm wondering why event.client.frameType is top-level rather than nested. https://codereview.chromium.org/1102363002/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); ServiceWorker testing style is to single-quote JavaScript strings
Patchset #6 (id:100001) has been deleted
Thanks falken & jsbell Created new cl, for testing client attribute of FetchEvent. Updated Chromium Side changes. Please have a look. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); On 2015/05/08 22:20:25, falken wrote: > On 2015/05/07 16:46:54, Paritosh Kumar wrote: > > On 2015/05/07 04:06:03, falken wrote: > > > On 2015/05/06 16:21:26, jsbell wrote: > > > > Interesting... given the table in http://fetch.spec.whatwg.org I would > have > > > expected > > > > the context frame type to be "nested" for an iframe. > > > > > > > > Is this a bug in our implementation of fetch? If so, fine to leave this > > as-is. > > > > Likely I'm just misunderstanding some subtlety in fetch and sw specs. > > > > > > Can you file a bug if it's an existing bug? > > > > I'm not updating on this now, first we will have to test it by sending it to > > 'event.client' using > > postMessage from worker that we should receive in frame window, from there we > > can check it. > > Can you elaborate on this? It looks like you're testing it in this line because > the SW populates body with event.client.frameType. I'm wondering why > event.client.frameType is top-level rather than nested. I investigated this issue and found that, this is not due to our implementation of fetch. We can see this problem only with event.client, actually in my implementation of client attribute of FetchEvent, I was not initializing the client attribute of ServiceWorkerFetchRequest in Chromium Side changes, that lead to default value of client, and hence other property of client also. https://codereview.chromium.org/1102363002/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, "top-level", "frameType should be top-level"); On 2015/05/08 22:20:25, falken wrote: > ServiceWorker testing style is to single-quote JavaScript strings Thanks, Done in new cl.
lgtm
paritosh.in@samsung.com changed reviewers: + mkwst@chromium.org
@Mike: Please have a look.
On 2015/05/13 at 10:51:08, paritosh.in wrote: > @Mike: Please have a look. I'll defer to jsbell@; if he's happy, RS LGTM for public/, platform/, and web/.
lgtm https://codereview.chromium.org/1102363002/diff/100002/Source/modules/service... File Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/1102363002/diff/100002/Source/modules/service... Source/modules/serviceworkers/FetchEvent.cpp:41: ServiceWorkerClient* FetchEvent::client() const nit: The definition order should match declaration order in the header file.
Thanks. Done. https://codereview.chromium.org/1102363002/diff/100002/Source/modules/service... File Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/1102363002/diff/100002/Source/modules/service... Source/modules/serviceworkers/FetchEvent.cpp:41: ServiceWorkerClient* FetchEvent::client() const On 2015/05/15 04:35:16, falken wrote: > nit: The definition order should match declaration order in the header file. Done.
The CQ bit was checked by paritosh.in@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, mkwst@chromium.org, jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/1102363002/#ps130001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1102363002/130001
Message was sent while issue was closed.
Committed patchset #7 (id:130001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195398 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
