Implement Request.context for Fetch API

Implement Request.context for Fetch API

Spec:
https://fetch.spec.whatwg.org/#dom-request-context

BUG=455116
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193624

[shiva.jm, 2015-04-06 10:46:51]

shiva.jm@samsung.com changed reviewers:
+ hiroshige@chromium.org, horo@chromium.org, jsbell@chromium.org

[shiva.jm, 2015-04-06 10:46:52]

Please have a look. No method is given to set the request context, Firefox is
supporting setContext() method in Request.idl file.

[jsbell, 2015-04-06 17:04:15]

I didn't look at the implementation, just IDL and test.

I'd like to see layout tests for more of the contexts, using an actual service
worker and a controlled page that exercises at least a handful of the types
(e.g. fetch, image, script, style, xmlhttprequest)

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
File LayoutTests/http/tests/fetch/script-tests/request.js (right):

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:723: return req.blob()
Extracting the body of the request here should not be necessary; unlike the
above tests for MIME/content-type this is just inspecting properties of the
Request.

It doesn't need to be an async promise_test either.

(Unless I'm radically misunderstanding the spec)

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:725:
assert_equals(req.context, '');
Please add a message to the assertion, e.g. "RequestContext should be the empty
string for synthetic Request objects"

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:727: }, 'requests
context');
Please give the test a more descriptive name, e.g. "RequestContext of a
synthetic Request object"

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:729:
promise_test(function(t) {
What is this test verifying that's different than the previous case?

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.idl (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.idl:11: enum RequestContext { "", "audio",
"beacon", "cspreport", "download", "embed", "eventsource", "favicon",
Put this above RequestMode to match the spec order

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.idl:12: "fetch", "font", "form", "frame",
"hyperlink", "iframe", "image", "imageset", "import", "internal",
Can you indent these continuation lines?

Maybe try and match the spec and format like:

enum RequestContext {
    ...
};

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.idl:39: readonly attribute RequestContext context;
Move above referrer to match the attribute order in the spec.

[horo, 2015-04-07 00:21:25]

Thank you for taking this issue!

But pleasae add LayoutTests which check the Request.context of FetchEvent in
ServiceWorker.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/FetchM...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:469: 
Please set the context to "fetch".

https://fetch.spec.whatwg.org/#fetch-method
> 5.6 Fetch method
> ...
>  3. Set r's context to "fetch".

[hiroshige, 2015-04-07 03:56:11]

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/FetchR...
File Source/modules/fetch/FetchRequestData.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/FetchR...
Source/modules/fetch/FetchRequestData.cpp:33:
request->setContext(webRequest.requestContext());
Move this above m_referrer.setURL(... to match the order with the spec.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:37: request->setContext(original->context());
Should this be WebURLRequest::RequestContextUnspecified? (please double-check)
Spec: https://fetch.spec.whatwg.org/#dom-request
Step 4:
Set request to a new request whose url is request's url, ..., context is the
empty string, ...

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:315: String Request::context() const
Move context() above referrer() to match the order with the spec.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Request.h
File Source/modules/fetch/Request.h (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.h:51: String context() const;
Move context() declaration above referrer() to match the order in the spec
https://fetch.spec.whatwg.org/#request-class.

[shiva.jm, 2015-04-09 09:16:23]

Please have a look, updated the comments and added tests.

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
File LayoutTests/http/tests/fetch/script-tests/request.js (right):

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:723: return req.blob()
On 2015/04/06 17:04:15, jsbell wrote:
> Extracting the body of the request here should not be necessary; unlike the
> above tests for MIME/content-type this is just inspecting properties of the
> Request.
> 
> It doesn't need to be an async promise_test either.
> 
> (Unless I'm radically misunderstanding the spec)

Done.

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:725:
assert_equals(req.context, '');
On 2015/04/06 17:04:14, jsbell wrote:
> Please add a message to the assertion, e.g. "RequestContext should be the
empty
> string for synthetic Request objects"

Done.

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:727: }, 'requests
context');
On 2015/04/06 17:04:14, jsbell wrote:
> Please give the test a more descriptive name, e.g. "RequestContext of a
> synthetic Request object"

Done.

https://codereview.chromium.org/1060033002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:729:
promise_test(function(t) {
On 2015/04/06 17:04:14, jsbell wrote:
> What is this test verifying that's different than the previous case?

Done.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/FetchM...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:469: 
On 2015/04/07 00:21:25, horo wrote:
> Please set the context to "fetch".
> 
> https://fetch.spec.whatwg.org/#fetch-method
> > 5.6 Fetch method
> > ...
> >  3. Set r's context to "fetch".

Done.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/FetchR...
File Source/modules/fetch/FetchRequestData.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/FetchR...
Source/modules/fetch/FetchRequestData.cpp:33:
request->setContext(webRequest.requestContext());
On 2015/04/07 03:56:11, hiroshige wrote:
> Move this above m_referrer.setURL(... to match the order with the spec.

Done.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:37: request->setContext(original->context());
On 2015/04/07 03:56:11, hiroshige wrote:
> Should this be WebURLRequest::RequestContextUnspecified? (please double-check)
> Spec: https://fetch.spec.whatwg.org/#dom-request
> Step 4:
> Set request to a new request whose url is request's url, ..., context is the
> empty string, ...

I think we no need to set the context here, it will be set in the constructor
"FetchRequestData::FetchRequestData()", when FetchRequestData::create() is
called.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:315: String Request::context() const
On 2015/04/07 03:56:11, hiroshige wrote:
> Move context() above referrer() to match the order with the spec.

Done.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Request.h
File Source/modules/fetch/Request.h (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.h:51: String context() const;
On 2015/04/07 03:56:11, hiroshige wrote:
> Move context() declaration above referrer() to match the order in the spec
> https://fetch.spec.whatwg.org/#request-class.

Done.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.idl (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.idl:11: enum RequestContext { "", "audio",
"beacon", "cspreport", "download", "embed", "eventsource", "favicon",
On 2015/04/06 17:04:15, jsbell wrote:
> Put this above RequestMode to match the spec order

Done.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.idl:12: "fetch", "font", "form", "frame",
"hyperlink", "iframe", "image", "imageset", "import", "internal",
On 2015/04/06 17:04:15, jsbell wrote:
> Can you indent these continuation lines?
> 
> Maybe try and match the spec and format like:
> 
> enum RequestContext {
>     ...
> };

Done.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.idl:39: readonly attribute RequestContext context;
On 2015/04/06 17:04:15, jsbell wrote:
> Move above referrer to match the attribute order in the spec.

Done.

[horo, 2015-04-09 13:27:31]

lgtm with nits

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/fetch/script-tests/fetch.js (right):

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/fetch.js:70:
assert_equals(request.context, '');
nit: fix indent

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/fetch.js:120:
assert_equals(request.context, '');
nit: fox indent

[jsbell, 2015-04-09 16:02:28]

lgtm as well (with horo's nits)

[hiroshige, 2015-04-09 17:43:22]

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:37: request->setContext(original->context());
On 2015/04/09 09:16:22, shiva.jm wrote:
> On 2015/04/07 03:56:11, hiroshige wrote:
> > Should this be WebURLRequest::RequestContextUnspecified? (please
double-check)
> > Spec: https://fetch.spec.whatwg.org/#dom-request
> > Step 4:
> > Set request to a new request whose url is request's url, ..., context is the
> > empty string, ...
> 
> I think we no need to set the context here, it will be set in the constructor
> "FetchRequestData::FetchRequestData()", when FetchRequestData::create() is
> called.
Then we have to remove the line of
> request->setContext(original->context());
here.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/fetch/script-tests/request.js (right):

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:722: 'Request.context
should be empty string for synthetic Request object');
nit: please wrap to 80 columns, e.g.
                  'Request.context should be empty string ' +
                  'for synthetic Request object');
(https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style)

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-request-resources.html (right):

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-request-resources.html:52: function
font_face_test(frame, url, expected_context, expexted_mode,
expected_credentials) {
nit: wrap to 80 columns.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-request-resources.html:64: function
css_image_test(frame, url, type, expected_context, expexted_mode,
expected_credentials) {
nit: wrap to 80 columns.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-request-resources.html:148:
image_test(f, REMOTE_URL, 'use-credentials', 'image', 'cors', 'include');
nit: wrap to 80 columns (also some lines below).

https://codereview.chromium.org/1060033002/diff/20001/Source/modules/fetch/Re...
File Source/modules/fetch/Request.idl (right):

https://codereview.chromium.org/1060033002/diff/20001/Source/modules/fetch/Re...
Source/modules/fetch/Request.idl:10: "", "audio", "beacon", "cspreport",
"download", "embed", "eventsource", "favicon",
nit: indent with 4 spaces?

[shiva.jm, 2015-04-10 09:27:10]

Please have a look, updated the test and source files.

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:37: request->setContext(original->context());
On 2015/04/09 17:43:21, hiroshige wrote:
> On 2015/04/09 09:16:22, shiva.jm wrote:
> > On 2015/04/07 03:56:11, hiroshige wrote:
> > > Should this be WebURLRequest::RequestContextUnspecified? (please
> double-check)
> > > Spec: https://fetch.spec.whatwg.org/#dom-request
> > > Step 4:
> > > Set request to a new request whose url is request's url, ..., context is
the
> > > empty string, ...
> > 
> > I think we no need to set the context here, it will be set in the
constructor
> > "FetchRequestData::FetchRequestData()", when FetchRequestData::create() is
> > called.
> Then we have to remove the line of
> > request->setContext(original->context());
> here.

The call to setContext() should be present here, no need to remove, since here a
copy of original request is made.
Actually in my previous comment "no need to set the context here", means no need
to set the context with WebURLRequest::RequestContextUnspecified, since context
will be set with WebURLRequest::RequestContextUnspecified in the constructor
"FetchRequestData::FetchRequestData()", when FetchRequestData::create() is
called.
sorry, i did not put correctly in my previous comment.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/fetch/script-tests/fetch.js (right):

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/fetch.js:70:
assert_equals(request.context, '');
On 2015/04/09 13:27:31, horo wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/fetch.js:120:
assert_equals(request.context, '');
On 2015/04/09 13:27:31, horo wrote:
> nit: fox indent

Done.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/fetch/script-tests/request.js (right):

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:722: 'Request.context
should be empty string for synthetic Request object');
On 2015/04/09 17:43:21, hiroshige wrote:
> nit: please wrap to 80 columns, e.g.
>                   'Request.context should be empty string ' +
>                   'for synthetic Request object');
> (https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style)

Done.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-request-resources.html (right):

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-request-resources.html:52: function
font_face_test(frame, url, expected_context, expexted_mode,
expected_credentials) {
On 2015/04/09 17:43:21, hiroshige wrote:
> nit: wrap to 80 columns.

Done.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-request-resources.html:64: function
css_image_test(frame, url, type, expected_context, expexted_mode,
expected_credentials) {
On 2015/04/09 17:43:21, hiroshige wrote:
> nit: wrap to 80 columns.

Done.

https://codereview.chromium.org/1060033002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-request-resources.html:148:
image_test(f, REMOTE_URL, 'use-credentials', 'image', 'cors', 'include');
On 2015/04/09 17:43:21, hiroshige wrote:
> nit: wrap to 80 columns (also some lines below).

Done.

https://codereview.chromium.org/1060033002/diff/20001/Source/modules/fetch/Re...
File Source/modules/fetch/Request.idl (right):

https://codereview.chromium.org/1060033002/diff/20001/Source/modules/fetch/Re...
Source/modules/fetch/Request.idl:10: "", "audio", "beacon", "cspreport",
"download", "embed", "eventsource", "favicon",
On 2015/04/09 17:43:21, hiroshige wrote:
> nit: indent with 4 spaces?

Done.

[shiva.jm, 2015-04-10 10:19:21]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[shiva.jm, 2015-04-10 10:19:21]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
horo@chromium.org
Link to the patchset: https://codereview.chromium.org/1060033002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-04-10 10:20:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060033002/40001

[hiroshige, 2015-04-10 11:24:23]

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:37: request->setContext(original->context());
On 2015/04/10 09:27:10, shiva.jm wrote:
> On 2015/04/09 17:43:21, hiroshige wrote:
> > On 2015/04/09 09:16:22, shiva.jm wrote:
> > > On 2015/04/07 03:56:11, hiroshige wrote:
> > > > Should this be WebURLRequest::RequestContextUnspecified? (please
> > double-check)
> > > > Spec: https://fetch.spec.whatwg.org/#dom-request
> > > > Step 4:
> > > > Set request to a new request whose url is request's url, ..., context is
> the
> > > > empty string, ...
> > > 
> > > I think we no need to set the context here, it will be set in the
> constructor
> > > "FetchRequestData::FetchRequestData()", when FetchRequestData::create() is
> > > called.
> > Then we have to remove the line of
> > > request->setContext(original->context());
> > here.
> 
> The call to setContext() should be present here, no need to remove, since here
a
> copy of original request is made.
> Actually in my previous comment "no need to set the context here", means no
need
> to set the context with WebURLRequest::RequestContextUnspecified, since
context
> will be set with WebURLRequest::RequestContextUnspecified in the constructor
> "FetchRequestData::FetchRequestData()", when FetchRequestData::create() is
> called.
> sorry, i did not put correctly in my previous comment.

I still think we must NOT set context to |original->context()| here, because
createCopyOfFetchRequestDataForFetch() is not a general copy construction
function, but it corresponds to Step 4 of Request() constructor in the spec
https://fetch.spec.whatwg.org/#dom-request:
> Set request to a new request whose url is request's url, method is request's
method, header list is a copy of request's header list, unsafe request flag is
set, client is entry settings object, origin is entry settings object's origin,
force Origin header flag is set, same-origin data URL flag is set, context is
the empty string, mode is request's mode, credentials mode is request's
credentials mode, cache mode is request's cache mode, and redirect mode is
request's redirect mode.

Here, the spec states "context is the empty string", so |request|'s context must
remain the empty string here.

From another point of view,
(new Request(event.request)).context
must be the empty string due to the Step 4 of the Request() constructor I cited
above (because this is synthetic Request object).
In this CL (Patch Set 3), (new Request(event.request)).context is
event.request.context, which can be e.g. 'script' in serviceworker.
Removing this setContext() here leads (new Request(event.request)).context to be
the empty string.
(For test code see https://codereview.chromium.org/1079493002/, Patch Set 2. The
added tests fails when we setContext() here.)

[shiva.jm, 2015-04-10 11:42:37]

The CQ bit was unchecked by shiva.jm@samsung.com

[shiva.jm, 2015-04-10 11:51:39]

On 2015/04/10 11:24:23, hiroshige wrote:
>
https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
> File Source/modules/fetch/Request.cpp (right):
> 
>
https://codereview.chromium.org/1060033002/diff/1/Source/modules/fetch/Reques...
> Source/modules/fetch/Request.cpp:37: request->setContext(original->context());
> On 2015/04/10 09:27:10, shiva.jm wrote:
> > On 2015/04/09 17:43:21, hiroshige wrote:
> > > On 2015/04/09 09:16:22, shiva.jm wrote:
> > > > On 2015/04/07 03:56:11, hiroshige wrote:
> > > > > Should this be WebURLRequest::RequestContextUnspecified? (please
> > > double-check)
> > > > > Spec: https://fetch.spec.whatwg.org/#dom-request
> > > > > Step 4:
> > > > > Set request to a new request whose url is request's url, ..., context
is
> > the
> > > > > empty string, ...
> > > > 
> > > > I think we no need to set the context here, it will be set in the
> > constructor
> > > > "FetchRequestData::FetchRequestData()", when FetchRequestData::create()
is
> > > > called.
> > > Then we have to remove the line of
> > > > request->setContext(original->context());
> > > here.
> > 
> > The call to setContext() should be present here, no need to remove, since
here
> a
> > copy of original request is made.
> > Actually in my previous comment "no need to set the context here", means no
> need
> > to set the context with WebURLRequest::RequestContextUnspecified, since
> context
> > will be set with WebURLRequest::RequestContextUnspecified in the constructor
> > "FetchRequestData::FetchRequestData()", when FetchRequestData::create() is
> > called.
> > sorry, i did not put correctly in my previous comment.
> 
> I still think we must NOT set context to |original->context()| here, because
> createCopyOfFetchRequestDataForFetch() is not a general copy construction
> function, but it corresponds to Step 4 of Request() constructor in the spec
> https://fetch.spec.whatwg.org/#dom-request:
> > Set request to a new request whose url is request's url, method is request's
> method, header list is a copy of request's header list, unsafe request flag is
> set, client is entry settings object, origin is entry settings object's
origin,
> force Origin header flag is set, same-origin data URL flag is set, context is
> the empty string, mode is request's mode, credentials mode is request's
> credentials mode, cache mode is request's cache mode, and redirect mode is
> request's redirect mode.
> 
> Here, the spec states "context is the empty string", so |request|'s context
must
> remain the empty string here.
> 
> From another point of view,
> (new Request(event.request)).context
> must be the empty string due to the Step 4 of the Request() constructor I
cited
> above (because this is synthetic Request object).
> In this CL (Patch Set 3), (new Request(event.request)).context is
> event.request.context, which can be e.g. 'script' in serviceworker.
> Removing this setContext() here leads (new Request(event.request)).context to
be
> the empty string.
> (For test code see https://codereview.chromium.org/1079493002/, Patch Set 2.
The
> added tests fails when we setContext() here.)

Thanks for the detailed inputs, got the context now. I will remove the
setContext()
from createCopyOfFetchRequestDataForFetch() function.
Shall we add the new tests, that are covered in
https://codereview.chromium.org/1079493002/
for these patch ? Or you have plan to commit these changes in
https://codereview.chromium.org/1079493002/
itself ?

[hiroshige, 2015-04-10 11:54:49]

> Shall we add the new tests, that are covered in
> https://codereview.chromium.org/1079493002/
> for these patch ? Or you have plan to commit these changes in
> https://codereview.chromium.org/1079493002/
> itself ?
Either is fine with me.

[shiva.jm, 2015-04-13 06:55:07]

Please have a look, updated the test and source files. also merged the changes
from https://codereview.chromium.org/1079493002/ as well.

[shiva.jm, 2015-04-13 06:57:07]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[shiva.jm, 2015-04-13 06:57:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
horo@chromium.org
Link to the patchset: https://codereview.chromium.org/1060033002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-04-13 06:57:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060033002/60001

[hiroshige, 2015-04-13 08:10:48]

lgtm.

[shiva.jm, 2015-04-13 08:43:53]

The CQ bit was unchecked by shiva.jm@samsung.com

[shiva.jm, 2015-04-13 08:44:38]

The CQ bit was checked by shiva.jm@samsung.com

[commit-bot: I haz the power, 2015-04-13 08:45:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060033002/60001

[commit-bot: I haz the power, 2015-04-13 09:52:20]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193624

[yhirano, 2015-04-14 07:07:10]

yhirano@chromium.org changed reviewers:
+ yhirano@chromium.org

[yhirano, 2015-04-14 07:07:11]

https://codereview.chromium.org/1060033002/diff/60001/Source/modules/fetch/Re...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1060033002/diff/60001/Source/modules/fetch/Re...
Source/modules/fetch/Request.cpp:58: // "3. Set |request| to a new request whose
url is |request|'s url, method
(Mainly for module/fetch reviewers) As the spec changes, this comment  should be
changed **if we want to keep it updated**, or delete it otherwise. There are
other spec text changes as well, and I'm wondering if we can / should keep the
spec comments updated in modules/fetch. What do you think?