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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 329853012: [ServiceWorker] Make Request class better conformance with the spec. (Closed)

Created:
6 years, 5 months ago by horo
Modified:
6 years, 5 months ago
CC:
abarth-chromium, alecflett+watch_chromium.org, blink-reviews, dglazkov+blink, horo+watch_chromium.org, jamesr, jsbell+serviceworker_chromium.org, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, tzik
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

[ServiceWorker] Refactor Request class to make better conformance with the spec. This change introduce referrer, mode, and credentials attributes and change the class of headers attribute from HeaderMap to Headers and make all attributes readonly. And also this change implements the logic which is defined in the spec. http://fetch.spec.whatwg.org/#request-class http://fetch.spec.whatwg.org/#requests BUG=373120 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177712

Patch Set 1 : #

Total comments: 12

Patch Set 2 : rebase #

Patch Set 3 : rebase #

Patch Set 4 : incorporated yhirano's comment #

Patch Set 5 : Copy headers in FetchRequestData::createRestrictedCopy(). #

Total comments: 6

Patch Set 6 : incorporated yhirano's comment #

Patch Set 7 : rebase #

Patch Set 8 : rebase and fix test #

Total comments: 36

Patch Set 9 : incorporated falken's comment #

Total comments: 24

Patch Set 10 : incorporated falken's comment #

Total comments: 8

Patch Set 11 : #

Total comments: 16

Patch Set 12 : incorporated jochen's comment #

Total comments: 10

Patch Set 13 : incorporated jochen's comment #

Patch Set 14 : #

Patch Set 15 : remove m_ in the name of struct members #

Unified diffs Side-by-side diffs Delta from patch set Stats (+784 lines, -107 lines) Patch
M LayoutTests/http/tests/serviceworker/request-end-to-end.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/http/tests/serviceworker/resources/request-worker.js View 1 2 3 4 5 6 7 8 9 1 chunk +246 lines, -18 lines 0 comments Download
M Source/modules/modules.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/modules/serviceworkers/FetchEvent.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/serviceworkers/FetchEvent.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/modules/serviceworkers/FetchHeaderList.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/modules/serviceworkers/FetchHeaderList.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +9 lines, -1 line 0 comments Download
M Source/modules/serviceworkers/FetchManager.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M Source/modules/serviceworkers/FetchManager.cpp View 1 2 3 4 5 6 7 8 2 chunks +20 lines, -0 lines 0 comments Download
A Source/modules/serviceworkers/FetchRequestData.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +114 lines, -0 lines 0 comments Download
A Source/modules/serviceworkers/FetchRequestData.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +81 lines, -0 lines 0 comments Download
M Source/modules/serviceworkers/Headers.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -3 lines 0 comments Download
M Source/modules/serviceworkers/Request.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +25 lines, -14 lines 0 comments Download
M Source/modules/serviceworkers/Request.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +212 lines, -28 lines 0 comments Download
M Source/modules/serviceworkers/Request.idl View 1 chunk +18 lines, -15 lines 0 comments Download
M Source/modules/serviceworkers/RequestInit.h View 1 2 3 4 5 6 7 8 12 13 14 1 chunk +10 lines, -6 lines 0 comments Download
M Source/platform/exported/WebServiceWorkerRequest.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +27 lines, -16 lines 0 comments Download
M public/platform/WebServiceWorkerRequest.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +8 lines, -2 lines 0 comments Download

Messages

Total messages: 35 (0 generated)
horo
yhirano@ Could you please review?
6 years, 5 months ago (2014-06-30 07:07:57 UTC) #1
yhirano
https://codereview.chromium.org/329853012/diff/40001/Source/modules/serviceworkers/FetchRequestData.cpp File Source/modules/serviceworkers/FetchRequestData.cpp (right): https://codereview.chromium.org/329853012/diff/40001/Source/modules/serviceworkers/FetchRequestData.cpp#newcode23 Source/modules/serviceworkers/FetchRequestData.cpp:23: RefPtr<FetchRequestData> request = adoptRef(new FetchRequestData()); Let's use |create|. https://codereview.chromium.org/329853012/diff/40001/Source/modules/serviceworkers/FetchRequestData.cpp#newcode47 ...
6 years, 5 months ago (2014-07-02 01:53:07 UTC) #2
horo
https://codereview.chromium.org/329853012/diff/40001/Source/modules/serviceworkers/FetchRequestData.cpp File Source/modules/serviceworkers/FetchRequestData.cpp (right): https://codereview.chromium.org/329853012/diff/40001/Source/modules/serviceworkers/FetchRequestData.cpp#newcode23 Source/modules/serviceworkers/FetchRequestData.cpp:23: RefPtr<FetchRequestData> request = adoptRef(new FetchRequestData()); On 2014/07/02 01:53:06, yhirano ...
6 years, 5 months ago (2014-07-02 03:33:04 UTC) #3
yhirano
It is completely optional and you don't do it in this CL, but having a ...
6 years, 5 months ago (2014-07-02 08:28:15 UTC) #4
horo
https://codereview.chromium.org/329853012/diff/150001/LayoutTests/http/tests/serviceworker/resources/request-worker.js File LayoutTests/http/tests/serviceworker/resources/request-worker.js (right): https://codereview.chromium.org/329853012/diff/150001/LayoutTests/http/tests/serviceworker/resources/request-worker.js#newcode9 LayoutTests/http/tests/serviceworker/resources/request-worker.js:9: On 2014/07/02 08:28:14, yhirano wrote: > Do you have ...
6 years, 5 months ago (2014-07-02 13:49:14 UTC) #5
yhirano
lgtm
6 years, 5 months ago (2014-07-03 07:07:11 UTC) #6
horo
falken@ Could you please review?
6 years, 5 months ago (2014-07-03 07:27:27 UTC) #7
falken
Some comments so far, I didn't read everything yet. https://codereview.chromium.org/329853012/diff/230001/LayoutTests/http/tests/serviceworker/resources/request-worker.js File LayoutTests/http/tests/serviceworker/resources/request-worker.js (right): https://codereview.chromium.org/329853012/diff/230001/LayoutTests/http/tests/serviceworker/resources/request-worker.js#newcode4 LayoutTests/http/tests/serviceworker/resources/request-worker.js:4: ...
6 years, 5 months ago (2014-07-03 10:05:33 UTC) #8
horo
https://codereview.chromium.org/329853012/diff/230001/LayoutTests/http/tests/serviceworker/resources/request-worker.js File LayoutTests/http/tests/serviceworker/resources/request-worker.js (right): https://codereview.chromium.org/329853012/diff/230001/LayoutTests/http/tests/serviceworker/resources/request-worker.js#newcode4 LayoutTests/http/tests/serviceworker/resources/request-worker.js:4: var url = 'https://www.example.com/test.html'; On 2014/07/03 10:05:32, falken wrote: ...
6 years, 5 months ago (2014-07-03 12:40:30 UTC) #9
falken
Thanks for the update. Here's another round of comments. https://codereview.chromium.org/329853012/diff/270001/LayoutTests/http/tests/serviceworker/resources/request-worker.js File LayoutTests/http/tests/serviceworker/resources/request-worker.js (right): https://codereview.chromium.org/329853012/diff/270001/LayoutTests/http/tests/serviceworker/resources/request-worker.js#newcode3 LayoutTests/http/tests/serviceworker/resources/request-worker.js:3: ...
6 years, 5 months ago (2014-07-04 05:43:40 UTC) #10
horo
https://codereview.chromium.org/329853012/diff/270001/LayoutTests/http/tests/serviceworker/resources/request-worker.js File LayoutTests/http/tests/serviceworker/resources/request-worker.js (right): https://codereview.chromium.org/329853012/diff/270001/LayoutTests/http/tests/serviceworker/resources/request-worker.js#newcode3 LayoutTests/http/tests/serviceworker/resources/request-worker.js:3: var url = 'https://www.example.com/test.html'; On 2014/07/04 05:43:39, falken wrote: ...
6 years, 5 months ago (2014-07-04 08:00:38 UTC) #11
falken
I think this lgtm. Please make sure you have an API owner review both the ...
6 years, 5 months ago (2014-07-04 10:05:01 UTC) #12
horo
https://codereview.chromium.org/329853012/diff/310001/Source/modules/serviceworkers/Request.cpp File Source/modules/serviceworkers/Request.cpp (right): https://codereview.chromium.org/329853012/diff/310001/Source/modules/serviceworkers/Request.cpp#newcode38 Source/modules/serviceworkers/Request.cpp:38: // We use the current mode instead of null, ...
6 years, 5 months ago (2014-07-04 10:29:29 UTC) #13
horo
jochen@ Could you please review Source/platform/exported/WebServiceWorkerRequest.cpp and public/platform/WebServiceWorkerRequest.h?
6 years, 5 months ago (2014-07-05 04:05:21 UTC) #14
falken
According to tkent@ an API owner should review the *.idl changes too (although the OWNER ...
6 years, 5 months ago (2014-07-07 01:21:50 UTC) #15
jochen (gone - plz use gerrit)
initial round of comments, tl;dr: if you touch the referrer header, you *must* pass along ...
6 years, 5 months ago (2014-07-07 07:15:33 UTC) #16
horo
https://codereview.chromium.org/329853012/diff/330001/Source/modules/serviceworkers/FetchHeaderList.cpp File Source/modules/serviceworkers/FetchHeaderList.cpp (right): https://codereview.chromium.org/329853012/diff/330001/Source/modules/serviceworkers/FetchHeaderList.cpp#newcode22 Source/modules/serviceworkers/FetchHeaderList.cpp:22: RefPtr<FetchHeaderList> list(create()); On 2014/07/07 07:15:33, jochen wrote: > please ...
6 years, 5 months ago (2014-07-07 09:31:38 UTC) #17
jochen (gone - plz use gerrit)
https://codereview.chromium.org/329853012/diff/350001/Source/modules/serviceworkers/FetchHeaderList.cpp File Source/modules/serviceworkers/FetchHeaderList.cpp (right): https://codereview.chromium.org/329853012/diff/350001/Source/modules/serviceworkers/FetchHeaderList.cpp#newcode23 Source/modules/serviceworkers/FetchHeaderList.cpp:23: for (size_t i = 0; i < m_headerList.size(); ++i) ...
6 years, 5 months ago (2014-07-07 15:31:52 UTC) #18
horo
https://codereview.chromium.org/329853012/diff/350001/Source/modules/serviceworkers/FetchHeaderList.cpp File Source/modules/serviceworkers/FetchHeaderList.cpp (right): https://codereview.chromium.org/329853012/diff/350001/Source/modules/serviceworkers/FetchHeaderList.cpp#newcode23 Source/modules/serviceworkers/FetchHeaderList.cpp:23: for (size_t i = 0; i < m_headerList.size(); ++i) ...
6 years, 5 months ago (2014-07-08 02:05:59 UTC) #19
jochen (gone - plz use gerrit)
lgtm
6 years, 5 months ago (2014-07-08 09:26:10 UTC) #20
horo
tkent@ Could you please review this as API owner?
6 years, 5 months ago (2014-07-09 01:55:05 UTC) #21
tkent
On 2014/07/09 01:55:05, horo wrote: > tkent@ > Could you please review this as API ...
6 years, 5 months ago (2014-07-09 01:59:23 UTC) #22
horo
On 2014/07/09 01:59:23, tkent wrote: > On 2014/07/09 01:55:05, horo wrote: > > tkent@ > ...
6 years, 5 months ago (2014-07-09 02:01:04 UTC) #23
horo
The CQ bit was checked by horo@chromium.org
6 years, 5 months ago (2014-07-09 02:01:10 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/329853012/430001
6 years, 5 months ago (2014-07-09 02:02:15 UTC) #25
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink ...
6 years, 5 months ago (2014-07-09 03:14:49 UTC) #26
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-09 03:25:35 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15968)
6 years, 5 months ago (2014-07-09 03:25:37 UTC) #28
horo
The CQ bit was checked by horo@chromium.org
6 years, 5 months ago (2014-07-09 03:34:05 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/329853012/430001
6 years, 5 months ago (2014-07-09 03:35:11 UTC) #30
commit-bot: I haz the power
Change committed as 177712
6 years, 5 months ago (2014-07-09 04:08:49 UTC) #31
eseidel
Looks like this broke: http/tests/serviceworker/request-end-to-end.html http/tests/serviceworker/request.html at least on mountain lion. Likely rebaselines needed.
6 years, 5 months ago (2014-07-09 05:23:54 UTC) #32
horo
On 2014/07/09 05:23:54, eseidel wrote: > Looks like this broke: > http/tests/serviceworker/request-end-to-end.html > http/tests/serviceworker/request.html > ...
6 years, 5 months ago (2014-07-09 07:27:41 UTC) #33
falken
On 2014/07/09 07:27:41, horo wrote: > On 2014/07/09 05:23:54, eseidel wrote: > > Looks like ...
6 years, 5 months ago (2014-07-09 07:45:22 UTC) #34
horo
6 years, 5 months ago (2014-07-09 08:03:25 UTC) #35
Message was sent while issue was closed. 
On 2014/07/09 07:45:22, falken wrote:
> On 2014/07/09 07:27:41, horo wrote:
> > On 2014/07/09 05:23:54, eseidel wrote:
> > > Looks like this broke:
> > > http/tests/serviceworker/request-end-to-end.html
> > > http/tests/serviceworker/request.html
> > > 
> > > at least on mountain lion.  Likely rebaselines needed.
> > 
> > Really?
> > In my Marvericks they don't fail.
> > Did you rebuild the test binary?
> 
> Eric probably saw failures on the waterfall:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
> 
> It looks like the tests failed immediately after the commit, then green after.
> They may be flaky (or did a revert happen?). Let's watch to see if it fails
> again.

The tests looks executed with the old worker script.

Powered by Google App Engine
This is Rietveld 408576698