Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(67)

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
Reviewers:
falken, Mike West, jsbell
CC:
blink-reviews, serviceworker-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Add 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 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -1 line) Patch
M Source/modules/serviceworkers/FetchEvent.h View 3 chunks +3 lines, -0 lines 0 comments Download
M Source/modules/serviceworkers/FetchEvent.cpp View 1 2 3 4 5 6 2 chunks +7 lines, -0 lines 0 comments Download
M Source/modules/serviceworkers/FetchEvent.idl View 1 chunk +1 line, -0 lines 0 comments Download
M Source/modules/serviceworkers/FetchEventInit.idl View 1 chunk +1 line, -0 lines 0 comments Download
M Source/platform/exported/WebServiceWorkerRequest.cpp View 1 2 3 2 chunks +11 lines, -0 lines 0 comments Download
M Source/web/ServiceWorkerGlobalScopeProxy.cpp View 1 2 2 chunks +8 lines, -1 line 0 comments Download
M public/platform/WebServiceWorkerRequest.h View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (5 generated)
Paritosh Kumar
This is Blink Side change for adding Client Attribute to FetchEvent Interface. Please have a ...
5 years, 7 months ago (2015-04-27 18:44:01 UTC) #2
jsbell
Do you have CLs for the Chromium-side changes and the Blink-side tests? https://codereview.chromium.org/1102363002/diff/1/Source/web/ServiceWorkerGlobalScopeProxy.cpp File Source/web/ServiceWorkerGlobalScopeProxy.cpp ...
5 years, 7 months ago (2015-04-27 23:32:47 UTC) #3
falken
Agree with jsbell's comments. I'd like to see the chromium-side patch and blink-side tests before ...
5 years, 7 months ago (2015-04-30 07:33:04 UTC) #4
Paritosh Kumar
On 2015/04/30 07:33:04, falken wrote: > Agree with jsbell's comments. I'd like to see the ...
5 years, 7 months ago (2015-04-30 07:37:33 UTC) #5
Paritosh Kumar
Thanks jsbell and falken Sorry for delay in update. Updated cl with a test for ...
5 years, 7 months ago (2015-05-05 14:22:25 UTC) #6
jsbell
https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html#newcode9 LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:9: function test() { Please do not override the `test` ...
5 years, 7 months ago (2015-05-05 17:45:42 UTC) #7
Paritosh Kumar
Thanks jsbell Addressed your comments, please have a look. https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/20001/LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html#newcode9 LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:9: ...
5 years, 7 months ago (2015-05-06 11:42:36 UTC) #8
jsbell
Can you add a test with a non-window client to verify that a non-WindowClient instance ...
5 years, 7 months ago (2015-05-06 16:21:27 UTC) #9
falken
looking good, I'm assuming the tests will have to be landed in a separate patch ...
5 years, 7 months ago (2015-05-07 04:06:03 UTC) #10
Paritosh Kumar
Thanks jsbell and falken Please have a look. https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html File LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html (right): https://codereview.chromium.org/1102363002/diff/40001/LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html#newcode17 LayoutTests/http/tests/serviceworker/fetch-event-client-attribute.html:17: assert_equals(frame.contentDocument.body.textContent, ...
5 years, 7 months ago (2015-05-07 16:46:54 UTC) #11
falken
I'm still confused about how to land this. Does this patch's test pass without the ...
5 years, 7 months ago (2015-05-08 22:20:26 UTC) #12
Paritosh Kumar
Thanks falken & jsbell Created new cl, for testing client attribute of FetchEvent. Updated Chromium ...
5 years, 7 months ago (2015-05-12 11:52:29 UTC) #14
jsbell
lgtm
5 years, 7 months ago (2015-05-12 16:45:05 UTC) #15
Paritosh Kumar
@Mike: Please have a look.
5 years, 7 months ago (2015-05-13 10:51:08 UTC) #17
Mike West
On 2015/05/13 at 10:51:08, paritosh.in wrote: > @Mike: Please have a look. I'll defer to ...
5 years, 7 months ago (2015-05-13 11:12:40 UTC) #18
falken
lgtm https://codereview.chromium.org/1102363002/diff/100002/Source/modules/serviceworkers/FetchEvent.cpp File Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/1102363002/diff/100002/Source/modules/serviceworkers/FetchEvent.cpp#newcode41 Source/modules/serviceworkers/FetchEvent.cpp:41: ServiceWorkerClient* FetchEvent::client() const nit: The definition order should ...
5 years, 7 months ago (2015-05-15 04:35:16 UTC) #19
Paritosh Kumar
Thanks. Done. https://codereview.chromium.org/1102363002/diff/100002/Source/modules/serviceworkers/FetchEvent.cpp File Source/modules/serviceworkers/FetchEvent.cpp (right): https://codereview.chromium.org/1102363002/diff/100002/Source/modules/serviceworkers/FetchEvent.cpp#newcode41 Source/modules/serviceworkers/FetchEvent.cpp:41: ServiceWorkerClient* FetchEvent::client() const On 2015/05/15 04:35:16, falken ...
5 years, 7 months ago (2015-05-15 05:47:37 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1102363002/130001
5 years, 7 months ago (2015-05-15 06:10:06 UTC) #23
commit-bot: I haz the power
5 years, 7 months ago (2015-05-15 07:31:03 UTC) #24
Message was sent while issue was closed.
Committed patchset #7 (id:130001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195398

Powered by Google App Engine
This is Rietveld 408576698