Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1391583002:
Introduce "navigate" mode in Requests (Closed)
|
Created:
5 years, 2 months ago by shiva.jm Modified:
5 years, 1 month ago Reviewers:
tyoshino (SeeGerritForStatus), jochen (gone - plz use gerrit), jar (doing other things), hiroshige, philipj_slow, Mark P, Mike West, jwd, horo, pdr., Nate Chapin, yhirano Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce "navigate" mode in Requests.
It's a new mode to support navigations.
As in Spec link:
https://fetch.spec.whatwg.org/#request-class
Issue Discussion can be found in link:
https://github.com/whatwg/fetch/issues/126
https://github.com/whatwg/fetch/commit/a74b317de6164cc8d599f31a762c11d2e100107c
Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/el1cAmGOrvY
BUG=537138
Committed: https://crrev.com/75f4ce50721c692f8348e7bfcfba594272e3eb44
Cr-Commit-Position: refs/heads/master@{#360026}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 14
Patch Set 6 : #Patch Set 7 : #
Total comments: 6
Patch Set 8 : #Patch Set 9 : #
Total comments: 12
Patch Set 10 : #Patch Set 11 : #
Total comments: 17
Patch Set 12 : #
Total comments: 14
Patch Set 13 : #
Total comments: 6
Patch Set 14 : #Patch Set 15 : #
Total comments: 2
Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Messages
Total messages: 107 (27 generated)
shiva.jm@samsung.com changed reviewers: + hiroshige@chromium.org, tyoshino@chromium.org, yhirano@chromium.org
pls have a look.
shiva.jm@samsung.com changed reviewers: + horo@chromium.org, jochen@chromium.org
pls have a look.
FETCH_REQUEST_MODE_NAVIGATE and WebURLRequest::FetchRequestModeNavigate have no actual use cases, because the "navigate" mode is intended to be used only by navigation, but navigation in Chromium/Blink does not use these enums. I'm afraid adding these enums introduces unnecessary complexity to the code and confuses readers (these enums are not actually used for navigation and thus not tested). How about just rejecting init.mode == "navigate" case to make the implementation spec-conformant?
horo@, I noticed you merged Issue 540967 into this issue. Do you have a plan to use FETCH_REQUEST_MODE_NAVIGATE or WebURLRequest::FetchRequestModeNavigate?
shiva.jm@ Thank you very much for taking this! WebURLRequest::FetchRequestModeNavigate must be set in FrameLoadRequest::initializeFetchFlags() And please change the expected mode in LayoutTests/http/tests/serviceworker/request-end-to-end.html, and LayoutTests/http/tests/serviceworker/fetch-request-fallback.html. These values were changed in https://codereview.chromium.org/1320023005/.
On 2015/10/09 10:47:00, horo wrote: > shiva.jm@ > Thank you very much for taking this! > > WebURLRequest::FetchRequestModeNavigate must be set in > FrameLoadRequest::initializeFetchFlags() > And please change the expected mode in > LayoutTests/http/tests/serviceworker/request-end-to-end.html, > and LayoutTests/http/tests/serviceworker/fetch-request-fallback.html. > These values were changed in https://codereview.chromium.org/1320023005/. horo@, Thanks! So I pull back my comment #5 (we should rather add code/tests that uses WebURLRequest::FetchRequestModeNavigate).
updated the patch with tests, pls have a look.
https://codereview.chromium.org/1391583002/diff/80001/content/common/service_... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1391583002/diff/80001/content/common/service_... content/common/service_worker/service_worker_types.h:81: FETCH_REQUEST_MODE_NAVIGATE, Please move FETCH_REQUEST_MODE_NAVIGATE to after FETCH_REQUEST_MODE_CORS_WITH_FORCED_PREFLIGHT. enum FetchRequestMode { FETCH_REQUEST_MODE_SAME_ORIGIN, FETCH_REQUEST_MODE_NO_CORS, FETCH_REQUEST_MODE_CORS, FETCH_REQUEST_MODE_CORS_WITH_FORCED_PREFLIGHT, FETCH_REQUEST_MODE_NAVIGATE, FETCH_REQUEST_MODE_LAST = FETCH_REQUEST_MODE_NAVIGATE }; And please add <int value="4" label="Navigate"/> in <enum name="FetchRequestMode" type="int"> of tools/metrics/histograms/histograms.xml. We are collecting the user metrics of the request modes in ServiceWorkerMetrics::RecordFallbackedRequestMode(). So we don't want to change the value of them. https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:213: m_request->setResponseTainting(FetchRequestData::OpaqueTainting); I think this should be "ASSERT_NOT_REACHED();". The request mode can't be "navigate" here. The redirect mode of "navigate" request must be "manual". When the redirect mode is manual, DocumentThreadableLoader doesn't follow the redirect response. So the origin of response url must be same as the request's origin. https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:549: threadableLoaderOptions.crossOriginRequestPolicy = AllowCrossOriginRequests; We don't need to allow cross origin requests. This should be DenyCrossOriginRequests. https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebURLRequest.h (right): https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebURLRequest.h:114: FetchRequestModeNavigate, Same as FETCH_REQUEST_MODE_NAVIGATE. enum FetchRequestMode { FetchRequestModeSameOrigin, FetchRequestModeNoCORS, FetchRequestModeCORS, FetchRequestModeCORSWithForcedPreflight, FetchRequestModeNavigate };
horo@ Thank you very much for inputs. Updated the patch with comments, also reverted FrameLoadRequest changes and tests. Earlier patch set has few tests failed, will check with new patch and fix in next CL, if any failures reported. https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:213: m_request->setResponseTainting(FetchRequestData::OpaqueTainting); On 2015/10/14 10:05:31, horo wrote: > I think this should be "ASSERT_NOT_REACHED();". > > The request mode can't be "navigate" here. > The redirect mode of "navigate" request must be "manual". > When the redirect mode is manual, DocumentThreadableLoader doesn't follow the > redirect response. > So the origin of response url must be same as the request's origin. Done, had same opinion in patchset2, but miss read from git hub discussions. shall we maintain the order of request mode enums in Request.idl same as enum FetchRequestMode as in WebURLRequest.h ? https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:549: threadableLoaderOptions.crossOriginRequestPolicy = AllowCrossOriginRequests; On 2015/10/14 10:05:31, horo wrote: > We don't need to allow cross origin requests. > This should be DenyCrossOriginRequests. Done, had same opinion in patchset2, but miss read from git hub discussions. https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebURLRequest.h (right): https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebURLRequest.h:114: FetchRequestModeNavigate, On 2015/10/14 10:05:32, horo wrote: > Same as FETCH_REQUEST_MODE_NAVIGATE. > > enum FetchRequestMode { > FetchRequestModeSameOrigin, > FetchRequestModeNoCORS, > FetchRequestModeCORS, > FetchRequestModeCORSWithForcedPreflight, > FetchRequestModeNavigate > }; Done.
updated the patch to add enum value in histograms.xml, missed in previous patch. https://codereview.chromium.org/1391583002/diff/80001/content/common/service_... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1391583002/diff/80001/content/common/service_... content/common/service_worker/service_worker_types.h:81: FETCH_REQUEST_MODE_NAVIGATE, On 2015/10/14 10:05:31, horo wrote: > Please move FETCH_REQUEST_MODE_NAVIGATE to after > FETCH_REQUEST_MODE_CORS_WITH_FORCED_PREFLIGHT. > enum FetchRequestMode { > FETCH_REQUEST_MODE_SAME_ORIGIN, > FETCH_REQUEST_MODE_NO_CORS, > FETCH_REQUEST_MODE_CORS, > FETCH_REQUEST_MODE_CORS_WITH_FORCED_PREFLIGHT, > FETCH_REQUEST_MODE_NAVIGATE, > FETCH_REQUEST_MODE_LAST = FETCH_REQUEST_MODE_NAVIGATE > }; > > And please add > <int value="4" label="Navigate"/> > in <enum name="FetchRequestMode" type="int"> of > tools/metrics/histograms/histograms.xml. > > We are collecting the user metrics of the request modes in > ServiceWorkerMetrics::RecordFallbackedRequestMode(). > So we don't want to change the value of them. Done.
https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:213: m_request->setResponseTainting(FetchRequestData::OpaqueTainting); > The redirect mode of "navigate" request must be "manual". horo@, What poses this limitation? The current Fetch spec text doesn't seem to do. https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:549: threadableLoaderOptions.crossOriginRequestPolicy = AllowCrossOriginRequests; > We don't need to allow cross origin requests. horo@, I want to clarify why this should be DenyCrossOriginRequests here. The Fetch spec seems to allow cross-origin requests. https://codereview.chromium.org/1391583002/diff/120001/content/common/service... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1391583002/diff/120001/content/common/service... content/common/service_worker/service_worker_types.h:86: FETCH_REQUEST_MODE_LAST = FETCH_REQUEST_MODE_CORS_WITH_FORCED_PREFLIGHT Should be "... = FETCH_REQUEST_MODE_NAVIGATE". https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:125: // => RequestInit::RequestInit. The following check should be placed here. https://fetch.spec.whatwg.org/#dom-request > 13. If any of init's members are present, run these substeps: > 1. If request's mode is "navigate", throw a TypeError. Also, adding a test for Request(req, {...}) where req.mode == 'navigate' is preferrable, but I'm not sure we can write such a test. > If any of init's members are present This predicate corresponds to |RequestInit.isReferrerSet|. In the draft patch set of the CL that introduced |isReferrerSet|, |isReferrerSet| was |RequestInit.areAnyMembersSet|. https://codereview.chromium.org/1291073004/diff2/460001:560001/Source/modules... This predicate was used only for referrer setting at that time, but now it is also used for rejecting navigate mode. How about renaming it again to |RequestInit.areAnyMembersSet| and using it here? yhirano@, how do you think? Doesn't this conflict your intention of renaming |areAnyMemberSet| to |isReferrerSet|?
https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:125: // => RequestInit::RequestInit. On 2015/10/15 06:37:03, hiroshige wrote: > The following check should be placed here. > https://fetch.spec.whatwg.org/#dom-request > > 13. If any of init's members are present, run these substeps: > > 1. If request's mode is "navigate", throw a TypeError. > > Also, adding a test for Request(req, {...}) where req.mode == 'navigate' is > preferrable, but I'm not sure we can write such a test. > > > > If any of init's members are present > This predicate corresponds to |RequestInit.isReferrerSet|. > > In the draft patch set of the CL that introduced |isReferrerSet|, > |isReferrerSet| was |RequestInit.areAnyMembersSet|. > https://codereview.chromium.org/1291073004/diff2/460001:560001/Source/modules... > This predicate was used only for referrer setting at that time, but now it is > also used for rejecting navigate mode. > How about renaming it again to |RequestInit.areAnyMembersSet| and using it here? > yhirano@, how do you think? Doesn't this conflict your intention of renaming > |areAnyMemberSet| to |isReferrerSet|? There is no problem, but adding some comments here would be helpful because there is a small difference between what the spec says and what we do while the result is same, and renaming the variable may cause some confusion without further explanation.
On 2015/10/15 07:00:09, yhirano wrote: > https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/fetch/Request.cpp (right): > > https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/Request.cpp:125: // => > RequestInit::RequestInit. > On 2015/10/15 06:37:03, hiroshige wrote: > > The following check should be placed here. > > https://fetch.spec.whatwg.org/#dom-request > > > 13. If any of init's members are present, run these substeps: > > > 1. If request's mode is "navigate", throw a TypeError. > > > > Also, adding a test for Request(req, {...}) where req.mode == 'navigate' is > > preferrable, but I'm not sure we can write such a test. > > > > > > > If any of init's members are present > > This predicate corresponds to |RequestInit.isReferrerSet|. > > > > In the draft patch set of the CL that introduced |isReferrerSet|, > > |isReferrerSet| was |RequestInit.areAnyMembersSet|. > > > https://codereview.chromium.org/1291073004/diff2/460001:560001/Source/modules... > > This predicate was used only for referrer setting at that time, but now it is > > also used for rejecting navigate mode. > > How about renaming it again to |RequestInit.areAnyMembersSet| and using it > here? > > yhirano@, how do you think? Doesn't this conflict your intention of renaming > > |areAnyMemberSet| to |isReferrerSet|? > > There is no problem, but adding some comments here would be helpful because > there is a small difference between what the spec says and what we do while the > result is same, and renaming the variable may cause some confusion without > further explanation. yhirano@, hiroshige@, Thanks for inputs. So we can rename the current isReferrerSet ---> isAnyMembersSet, since areAnyMembersSet is used in RequestInit::RequestInit() ? We should implement all the Steps 1 to 4 of https://fetch.spec.whatwg.org/#dom-request in these CL Or just do only Step1 (i.e If request's mode is "navigate", throw a TypeError.) and do step 3 to 4 in other CL?) Adding a test for Request(req, {...}), not seems feasible, will check on it.
https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:213: m_request->setResponseTainting(FetchRequestData::OpaqueTainting); On 2015/10/15 06:37:03, hiroshige wrote: > > The redirect mode of "navigate" request must be "manual". > horo@, > What poses this limitation? > The current Fetch spec text doesn't seem to do. "navigate" request is created only while "Navigating across documents". https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-documents https://github.com/whatwg/html/commit/80f052b44f3842084221b1104d1fecb02228699a > Set request's client to the source browsing context's active document's Window object's environment settings object, target browsing context to the browsing context being navigated, destination to "document", mode to "navigate", credentials mode to "include", use-URL-credentials flag, and redirect mode to "manual". https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:549: threadableLoaderOptions.crossOriginRequestPolicy = AllowCrossOriginRequests; On 2015/10/15 06:37:03, hiroshige wrote: > > We don't need to allow cross origin requests. > horo@, > I want to clarify why this should be DenyCrossOriginRequests here. > The Fetch spec seems to allow cross-origin requests. "navigate" request is only available in ServiceWorker. The request url's origin must be same as the SW's origin. The request's redirect mode is "manual". So we don't need to allow cross origin requests.
shiva.jm@samsung.com changed reviewers: + jar@chromium.org, jwd@chromium.org, mpearson@chromium.org
Updated with comments, pls have a look. https://codereview.chromium.org/1391583002/diff/120001/content/common/service... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1391583002/diff/120001/content/common/service... content/common/service_worker/service_worker_types.h:86: FETCH_REQUEST_MODE_LAST = FETCH_REQUEST_MODE_CORS_WITH_FORCED_PREFLIGHT On 2015/10/15 06:37:03, hiroshige wrote: > Should be "... = FETCH_REQUEST_MODE_NAVIGATE". Done. https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:125: // => RequestInit::RequestInit. On 2015/10/15 06:37:03, hiroshige wrote: > The following check should be placed here. > https://fetch.spec.whatwg.org/#dom-request > > 13. If any of init's members are present, run these substeps: > > 1. If request's mode is "navigate", throw a TypeError. > > Also, adding a test for Request(req, {...}) where req.mode == 'navigate' is > preferrable, but I'm not sure we can write such a test. > > > > If any of init's members are present > This predicate corresponds to |RequestInit.isReferrerSet|. > > In the draft patch set of the CL that introduced |isReferrerSet|, > |isReferrerSet| was |RequestInit.areAnyMembersSet|. > https://codereview.chromium.org/1291073004/diff2/460001:560001/Source/modules... > This predicate was used only for referrer setting at that time, but now it is > also used for rejecting navigate mode. > How about renaming it again to |RequestInit.areAnyMembersSet| and using it here? > yhirano@, how do you think? Doesn't this conflict your intention of renaming > |areAnyMemberSet| to |isReferrerSet|? Done. https://codereview.chromium.org/1391583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:125: // => RequestInit::RequestInit. On 2015/10/15 07:00:08, yhirano wrote: > On 2015/10/15 06:37:03, hiroshige wrote: > > The following check should be placed here. > > https://fetch.spec.whatwg.org/#dom-request > > > 13. If any of init's members are present, run these substeps: > > > 1. If request's mode is "navigate", throw a TypeError. > > > > Also, adding a test for Request(req, {...}) where req.mode == 'navigate' is > > preferrable, but I'm not sure we can write such a test. > > > > > > > If any of init's members are present > > This predicate corresponds to |RequestInit.isReferrerSet|. > > > > In the draft patch set of the CL that introduced |isReferrerSet|, > > |isReferrerSet| was |RequestInit.areAnyMembersSet|. > > > https://codereview.chromium.org/1291073004/diff2/460001:560001/Source/modules... > > This predicate was used only for referrer setting at that time, but now it is > > also used for rejecting navigate mode. > > How about renaming it again to |RequestInit.areAnyMembersSet| and using it > here? > > yhirano@, how do you think? Doesn't this conflict your intention of renaming > > |areAnyMemberSet| to |isReferrerSet|? > > There is no problem, but adding some comments here would be helpful because > there is a small difference between what the spec says and what we do while the > result is same, and renaming the variable may cause some confusion without > further explanation. Done.
lgtm
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms.xml with one minor comment https://codereview.chromium.org/1391583002/diff/160001/content/common/service... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1391583002/diff/160001/content/common/service... content/common/service_worker/service_worker_types.h:80: enum FetchRequestMode { Please comment on this enum and all other enums here that are written to histograms that entries cannot be deleted or reordered, and all new entries must go immediately before the end.
https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:549: threadableLoaderOptions.crossOriginRequestPolicy = AllowCrossOriginRequests; On 2015/10/15 11:43:05, horo wrote: > On 2015/10/15 06:37:03, hiroshige wrote: > > > We don't need to allow cross origin requests. > > horo@, > > I want to clarify why this should be DenyCrossOriginRequests here. > > The Fetch spec seems to allow cross-origin requests. > > "navigate" request is only available in ServiceWorker. > The request url's origin must be same as the SW's origin. > The request's redirect mode is "manual". > So we don't need to allow cross origin requests. (to: horo@) I feel it confusing. How about - using AllowCrossOriginRequests, - adding an assertion checking that the request url's origin is this origin, - adding an assertion about the redirect mode and - adding comments? https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:388: if (m_request->mode() == WebURLRequest::FetchRequestModeNavigate) { It would be good to merge this block with the previous one. https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:138: // hence the referrer member, so prefer using isAnyMembersSet instead isReferrerSet Please wrap comments in 80 columns. Sorry, what do you mean by this comment? https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.cpp:28: bool areAnyMembersSet = false; You can use the member variable directly instead of having a local variable. https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/RequestInit.h (right): https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.h:41: // so prefer using isAnyMembersSet instead isReferrerSet ?? https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.h:43: bool isAnyMembersSet; |areAnyMembersSet| would be better.
https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:549: threadableLoaderOptions.crossOriginRequestPolicy = AllowCrossOriginRequests; On 2015/10/16 18:15:53, yhirano wrote: > On 2015/10/15 11:43:05, horo wrote: > > On 2015/10/15 06:37:03, hiroshige wrote: > > > > We don't need to allow cross origin requests. > > > horo@, > > > I want to clarify why this should be DenyCrossOriginRequests here. > > > The Fetch spec seems to allow cross-origin requests. > > > > "navigate" request is only available in ServiceWorker. > > The request url's origin must be same as the SW's origin. > > The request's redirect mode is "manual". > > So we don't need to allow cross origin requests. > (to: horo@) > I feel it confusing. How about > - using AllowCrossOriginRequests, > - adding an assertion checking that the request url's origin is this origin, > - adding an assertion about the redirect mode and > - adding comments? Yes, we should have comments. But I think we should use DenyCrossOriginRequests here to reduce the security risk. ASSERT() doesn't work in release build. So if there are any bugs in handling the request and the response, severe security accident could happen.
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/200001
Updated with comments, pls have a look. https://codereview.chromium.org/1391583002/diff/160001/content/common/service... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1391583002/diff/160001/content/common/service... content/common/service_worker/service_worker_types.h:80: enum FetchRequestMode { On 2015/10/16 17:32:24, Mark P wrote: > Please comment on this enum and all other enums here that are written to > histograms that entries cannot be deleted or reordered, and all new entries must > go immediately before the end. Done. https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:388: if (m_request->mode() == WebURLRequest::FetchRequestModeNavigate) { On 2015/10/16 18:15:53, yhirano wrote: > It would be good to merge this block with the previous one. Done. https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:138: // hence the referrer member, so prefer using isAnyMembersSet instead isReferrerSet On 2015/10/16 18:15:53, yhirano wrote: > Please wrap comments in 80 columns. > Sorry, what do you mean by this comment? Done. https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.cpp:28: bool areAnyMembersSet = false; On 2015/10/16 18:15:53, yhirano wrote: > You can use the member variable directly instead of having a local variable. Done. https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/RequestInit.h (right): https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.h:41: // so prefer using isAnyMembersSet instead isReferrerSet On 2015/10/16 18:15:53, yhirano wrote: > ?? Done. https://codereview.chromium.org/1391583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.h:43: bool isAnyMembersSet; On 2015/10/16 18:15:53, yhirano wrote: > |areAnyMembersSet| would be better. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms.xml lgtm for the record
shiva.jm@samsung.com changed reviewers: + japhet@chromium.org, mkwst@chromium.org, pdr@chromium.org, philipj@opera.com
adding more Reviewers for WebURLRequest.h, pls have a look.
modules/fetch lgtm
https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:307: }, 'Request mode throw test'); I'd also like to see a test for the copy case ("If any of init's members are present and "navigate" was copied, throw." from https://github.com/whatwg/fetch/issues/126#issuecomment-142224197). https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:223: ASSERT_NOT_REACHED(); Should this be a `RELEASE_ASSERT_NOT_REACHED()`? https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:138: // hence the referrer member Nit: hence the referrer member what? https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.cpp:28: areAnyMembersSet = DictionaryHelper::get(options, "method", method) || areAnyMembersSet; Nit: here and elsewhere, `|=` would be clearer.
https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLRequest.h (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLRequest.h:118: FetchRequestModeNavigate Here and elsewhere, can you use the same order as in the spec, that is "navigate" before "same-origin"?
can you send an intent to ship to blink-dev please and link it from the CL description? as far as I can see this will be directly web exposed, right?
metrics lgtm
On 2015/10/20 08:03:47, jochen wrote: > can you send an intent to ship to blink-dev please and link it from the CL > description? > > as far as I can see this will be directly web exposed, right? The CL adds new request mode "navigate" for the existing mode category, as main Request Mode feature is already done. So is it OK to have intent ? if so will send new intent to ship to blink-dev.
updated the patch with comments, no code changes. will add the test as described in comments, if success in RequestTest.cpp. https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:307: }, 'Request mode throw test'); On 2015/10/20 07:27:04, Mike West (slow) wrote: > I'd also like to see a test for the copy case ("If any of init's members are > present and "navigate" was copied, throw." from > https://github.com/whatwg/fetch/issues/126#issuecomment-142224197). It's bit tricky in making layout test for these case, where Request(req, {...}) where req.mode == 'navigate', as we discussed in https://codereview.chromium.org/1391583002/#msg13. will add test in RequestTest.cpp, by constructing Request object using WebServiceWorkerRequest object, which has Navigate mode filled, and next try to construct Request object using above Request object created and use Dictionary to fill the RequestInit and use ASSERT_TRUE OR EXPECT_THROW to check typeError returned, will it be good ? https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:223: ASSERT_NOT_REACHED(); On 2015/10/20 07:27:04, Mike West (slow) wrote: > Should this be a `RELEASE_ASSERT_NOT_REACHED()`? Yes right, we can use RELEASE_ASSERT_NOT_REACHED(), for security vulnerability (from Assertions.h), but horo suggested to use ASSERT_NOT_REACHED() as in comment: https://codereview.chromium.org/1391583002/#msg26, he is now OOO till 25th, shall we take his intention to consider ASSERT_NOT_REACHED() OR shall we use RELEASE_ASSERT_NOT_REACHED() ?. https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:138: // hence the referrer member On 2015/10/20 07:27:04, Mike West (slow) wrote: > Nit: hence the referrer member what? actually below condition is checking, If |init|'s referrer member is present, since we have renamed earlier used 'isReferrerSet' --> 'areAnyMembersSet', so added above comments, so that user will not have confusion. also if, 'areAnyMembersSet' is true means, any of RequestInit members are set, which might include referrer member as well. as discussed in comments: https://codereview.chromium.org/1391583002/#msg14 https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.cpp:28: areAnyMembersSet = DictionaryHelper::get(options, "method", method) || areAnyMembersSet; On 2015/10/20 07:27:04, Mike West (slow) wrote: > Nit: here and elsewhere, `|=` would be clearer. only at these line(30), it holds good, but for other lines, it might fail for cases where more than 1 member is set, i.e if 'method' also true != 'headers' also true. so we can only apply `|=` at line 30 only. https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLRequest.h (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLRequest.h:118: FetchRequestModeNavigate On 2015/10/20 08:00:32, philipj wrote: > Here and elsewhere, can you use the same order as in the spec, that is > "navigate" before "same-origin"? We thought of having same order as in spec, but discussed in comments: https://codereview.chromium.org/1391583002/#msg10, Since, We are collecting the user metrics of the request modes, So we don't want to change the order of values.
https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLRequest.h (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLRequest.h:118: FetchRequestModeNavigate On 2015/10/21 08:34:59, shiva.jm wrote: > On 2015/10/20 08:00:32, philipj wrote: > > Here and elsewhere, can you use the same order as in the spec, that is > > "navigate" before "same-origin"? > > We thought of having same order as in spec, but discussed in comments: > https://codereview.chromium.org/1391583002/#msg10, > Since, We are collecting the user metrics of the request modes, > So we don't want to change the order of values. Acknowledged.
On 2015/10/21 at 08:29:20, shiva.jm wrote: > On 2015/10/20 08:03:47, jochen wrote: > > can you send an intent to ship to blink-dev please and link it from the CL > > description? > > > > as far as I can see this will be directly web exposed, right? > > The CL adds new request mode "navigate" for the existing mode > category, as main Request Mode feature is already done. So is it OK to have intent ? > if so will send new intent to ship to blink-dev. yes please
Description was changed from ========== Introduce "navigate" mode in Requests. It's a new mode to support navigations. As in Spec link: https://fetch.spec.whatwg.org/#request-class Issue Discussion can be found in link: https://github.com/whatwg/fetch/issues/126 https://github.com/whatwg/fetch/commit/a74b317de6164cc8d599f31a762c11d2e100107c BUG=537138 ========== to ========== Introduce "navigate" mode in Requests. It's a new mode to support navigations. As in Spec link: https://fetch.spec.whatwg.org/#request-class Issue Discussion can be found in link: https://github.com/whatwg/fetch/issues/126 https://github.com/whatwg/fetch/commit/a74b317de6164cc8d599f31a762c11d2e100107c Intent to implement and ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/el1cAmGOrvY BUG=537138 ==========
updated the CL with intent link, pls have a look.
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/220001
updated the patch to latest code base, also Intent to ship got 3 lgtm now, pls have a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Mostly nits (except for one for RELEASE_ASSERT_NOT_REACHED()). https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:223: ASSERT_NOT_REACHED(); On 2015/10/21 08:34:59, shiva.jm wrote: > On 2015/10/20 07:27:04, Mike West (slow) wrote: > > Should this be a `RELEASE_ASSERT_NOT_REACHED()`? > > Yes right, we can use RELEASE_ASSERT_NOT_REACHED(), > for security vulnerability (from Assertions.h), > but horo suggested to use > ASSERT_NOT_REACHED() as in comment: > https://codereview.chromium.org/1391583002/#msg26, > he is now OOO till 25th, shall we take his > intention to consider ASSERT_NOT_REACHED() OR > shall we use RELEASE_ASSERT_NOT_REACHED() ?. I think horo@ just suggested to use DenyCrossOriginRequests there and, he worries about security risks, RELEASE_ASSERT_NOT_REACHED() can be used here. horo@, does this make sense? https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.cpp:28: areAnyMembersSet = DictionaryHelper::get(options, "method", method) || areAnyMembersSet; On 2015/10/21 08:34:59, shiva.jm wrote: > On 2015/10/20 07:27:04, Mike West (slow) wrote: > > Nit: here and elsewhere, `|=` would be clearer. > > only at these line(30), it holds good, > but for other lines, it might fail for cases > where more than 1 member is set, i.e > if 'method' also true != 'headers' also true. > so we can only apply `|=` at line 30 only. Anyway, if we want to do that change, I think it is better to do it in a separate CL, so my comment below doesn't block this CL. How it fails when both |method| and |headers| are set? These two alternatives have pros and cons. I slightly prefer 2. as mkwst@ suggested. 1. areAnyMembersSet = get() || areAnyMembersSet get() is on the left-hand side to avoid short-circuit of || operator. I.e. "areAnyMembersSet = areAnyMembersSet || get(options, "headers", headers)" is a bug, because get() is not called and thus headers field is not set if areAnyMembersSet is true. This is somehow error-prone. 2. areAnyMembersSet |= get() '|=' is bitwise OR (not ||) and thus does not short-circuit. So get() is always called, avoiding the problem of 1. A possible drawback is that we use logical computation using bitwise operators -- reminds us of the ghost of the problem that (1 && 2) is true but (1 & 2) is false. In this case, it seems OK: - areAnyMembersSet is bool, and bool has only 0 or 1. - This is OR operator, so both (1 || 2) and (1 | 2) are true. 3. or should we write like the following? if (get()) areAnyMembersSet = true; https://codereview.chromium.org/1391583002/diff/220001/content/common/service... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1391583002/diff/220001/content/common/service... content/common/service_worker/service_worker_types.h:79: // entries written to histograms cannot be deleted or reordered Nit: Capitalize the first letter in L79 and L80. Nit: Add a period at the end of the statement in L79. Nit optional: how about the following for L79? // The enum entries below are written to histograms and thus cannot be deleted // or reordered. https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:302: {name: 'TypeError'}, nit: -2 indent for L302, 303, 305, 306, -4 indent for L304, +2 indent for L307. i.e. test(function() { assert_throws( {name: 'TypeError'}, function() { var request = new Request(URL, {mode: 'navigate'}); }, 'new Request with a navigate mode should throw'); }, 'Request mode throw test'); https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:603: // Using DenyCrossOriginRequests here to reduce the security risk nit: add a period at the end of line. https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:127: exceptionState.throwTypeError("Cannot construct a Request with a Request mode as navigate."); Making the message here and L177 will be better. (I don't have good sentences though... "Cannot construct a Request from a Request with 'navigate' mode and RequestInit" and "Mode 'navigate' cannot be specified in Request constructor"? Not so good though...) https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:137: // areAnyMembersSet will be True, if any members in RequestInit are set and nit: This comment (L137-138) is not needed. IIUC "if (init.areAnyMembersSet)" is used in L139 (rather than checking |init|'s referrer member) to implement the following spec statement: > If any of init's members are present, run these substeps: > Set request's referrer to "client" > Set request's referrer policy to the empty string. If |init|'s referrer member is not present, then |request|'s referrer is set to "client" via the default value set in |init.referrer| in RequestInit. yhirano@, is this correct? https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:174: // "17.If mode is "navigate", throw a TypeError. nit: a space between "17." and "If".
https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:307: }, 'Request mode throw test'); On 2015/10/21 08:34:59, shiva.jm wrote: > On 2015/10/20 07:27:04, Mike West (slow) wrote: > > I'd also like to see a test for the copy case ("If any of init's members are > > present and "navigate" was copied, throw." from > > https://github.com/whatwg/fetch/issues/126#issuecomment-142224197). > > It's bit tricky in making layout test for these case, > where Request(req, {...}) where req.mode == 'navigate', > as we discussed in https://codereview.chromium.org/1391583002/#msg13. > will add test in RequestTest.cpp, by constructing Request object > using WebServiceWorkerRequest object, which has Navigate mode filled, > and next try to construct Request object using above Request object > created and use Dictionary to fill the RequestInit and use > ASSERT_TRUE OR EXPECT_THROW to check typeError returned, will it be good ? As mentioned above, creating layout tests is hard (we cannot obtain Request with mode == 'navigate' in this CL) and perhaps test is not needed? I expect FrameLoadRequest.h change (suggested by horo@ and removed from this CL) will follow after this CL, and after that we might be able to add layout tests? (not sure though) Waiting mkwst@'s response.
https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:137: // areAnyMembersSet will be True, if any members in RequestInit are set and On 2015/10/30 12:31:30, hiroshige wrote: > nit: This comment (L137-138) is not needed. > > IIUC "if (init.areAnyMembersSet)" is used in L139 (rather than checking |init|'s > referrer member) to implement the following spec statement: > > If any of init's members are present, run these substeps: > > Set request's referrer to "client" > > Set request's referrer policy to the empty string. > If |init|'s referrer member is not present, then |request|'s referrer is set to > "client" via the default value set in |init.referrer| in RequestInit. > yhirano@, is this correct? - If no member is set on |init| => use |request|'s referrer. - Elif "referrer" member is present => use that value. - Else => use "client". In Blink, RequestInit::RequestInit implements part of the logic and I think mentioning that would be helpful. That's why I added the comment at L125 in the original code.
https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:223: ASSERT_NOT_REACHED(); On 2015/10/30 12:31:30, hiroshige wrote: > On 2015/10/21 08:34:59, shiva.jm wrote: > > On 2015/10/20 07:27:04, Mike West (slow) wrote: > > > Should this be a `RELEASE_ASSERT_NOT_REACHED()`? > > > > Yes right, we can use RELEASE_ASSERT_NOT_REACHED(), > > for security vulnerability (from Assertions.h), > > but horo suggested to use > > ASSERT_NOT_REACHED() as in comment: > > https://codereview.chromium.org/1391583002/#msg26, > > he is now OOO till 25th, shall we take his > > intention to consider ASSERT_NOT_REACHED() OR > > shall we use RELEASE_ASSERT_NOT_REACHED() ?. > > I think horo@ just suggested to use DenyCrossOriginRequests there and, he > worries about security risks, RELEASE_ASSERT_NOT_REACHED() can be used here. > horo@, does this make sense? I forgot about RELEASE_ASSERT_NOT_REACHED. I think we should use RELEASE_ASSERT_NOT_REACHED here.
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/240001
updated the comments, pls have a look. shall we commit? https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:307: }, 'Request mode throw test'); On 2015/10/30 12:57:42, hiroshige wrote: > On 2015/10/21 08:34:59, shiva.jm wrote: > > On 2015/10/20 07:27:04, Mike West (slow) wrote: > > > I'd also like to see a test for the copy case ("If any of init's members are > > > present and "navigate" was copied, throw." from > > > https://github.com/whatwg/fetch/issues/126#issuecomment-142224197). > > > > It's bit tricky in making layout test for these case, > > where Request(req, {...}) where req.mode == 'navigate', > > as we discussed in https://codereview.chromium.org/1391583002/#msg13. > > will add test in RequestTest.cpp, by constructing Request object > > using WebServiceWorkerRequest object, which has Navigate mode filled, > > and next try to construct Request object using above Request object > > created and use Dictionary to fill the RequestInit and use > > ASSERT_TRUE OR EXPECT_THROW to check typeError returned, will it be good ? > > As mentioned above, creating layout tests is hard (we cannot obtain Request with > mode == 'navigate' in this CL) and perhaps test is not needed? > I expect FrameLoadRequest.h change (suggested by horo@ and removed from this CL) > will follow after this CL, and after that we might be able to add layout tests? > (not sure though) > Waiting mkwst@'s response. The change was removed from FrameLoadRequest.h, since the enum order cannot be changed, as it is used for collecting the user metrics of the request modes. So you mean, we need to make that change as horo@ suggested in : https://codereview.chromium.org/1391583002/#msg7 ? https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/1391583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/RequestInit.cpp:28: areAnyMembersSet = DictionaryHelper::get(options, "method", method) || areAnyMembersSet; On 2015/10/30 12:31:30, hiroshige wrote: > On 2015/10/21 08:34:59, shiva.jm wrote: > > On 2015/10/20 07:27:04, Mike West (slow) wrote: > > > Nit: here and elsewhere, `|=` would be clearer. > > > > only at these line(30), it holds good, > > but for other lines, it might fail for cases > > where more than 1 member is set, i.e > > if 'method' also true != 'headers' also true. > > so we can only apply `|=` at line 30 only. > > Anyway, if we want to do that change, I think it is better to do it in a > separate CL, so my comment below doesn't block this CL. > > How it fails when both |method| and |headers| are set? > > These two alternatives have pros and cons. > I slightly prefer 2. as mkwst@ suggested. > > 1. areAnyMembersSet = get() || areAnyMembersSet > get() is on the left-hand side to avoid short-circuit of || operator. > I.e. "areAnyMembersSet = areAnyMembersSet || get(options, "headers", headers)" > is a bug, because get() is not called and thus headers field is not set if > areAnyMembersSet is true. > This is somehow error-prone. > > 2. areAnyMembersSet |= get() > '|=' is bitwise OR (not ||) and thus does not short-circuit. > So get() is always called, avoiding the problem of 1. > > A possible drawback is that we use logical computation using bitwise operators > -- reminds us of the ghost of the problem that (1 && 2) is true but (1 & 2) is > false. > In this case, it seems OK: > - areAnyMembersSet is bool, and bool has only 0 or 1. > - This is OR operator, so both (1 || 2) and (1 | 2) are true. > > 3. or should we write like the following? > if (get()) > areAnyMembersSet = true; Thanks for more detailed inputs, i misunderstand the bitwise '|=' operator operation. agree with option 2 above, so we can change as below: 1) areAnyMembersSet = DictionaryHelper::get() || areAnyMembersSet; ----> areAnyMembersSet |= get(); 2) areAnyMembersSet = areAnyMembersSet || isReferrerStringSet; ----> areAnyMembersSet |= isReferrerStringSet; 3) areAnyMembersSet = areAnyMembersSet || isBodySet; ----> areAnyMembersSet |= isBodySet; shall we change in these CL Or separate CL? https://codereview.chromium.org/1391583002/diff/220001/content/common/service... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1391583002/diff/220001/content/common/service... content/common/service_worker/service_worker_types.h:79: // entries written to histograms cannot be deleted or reordered On 2015/10/30 12:31:30, hiroshige wrote: > Nit: Capitalize the first letter in L79 and L80. > Nit: Add a period at the end of the statement in L79. > Nit optional: how about the following for L79? > // The enum entries below are written to histograms and thus cannot be deleted > // or reordered. Done. https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:302: {name: 'TypeError'}, On 2015/10/30 12:31:30, hiroshige wrote: > nit: > -2 indent for L302, 303, 305, 306, > -4 indent for L304, > +2 indent for L307. > > i.e. > test(function() { > assert_throws( > {name: 'TypeError'}, > function() { > var request = new Request(URL, {mode: 'navigate'}); > }, > 'new Request with a navigate mode should throw'); > }, 'Request mode throw test'); > > https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style Done. https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:603: // Using DenyCrossOriginRequests here to reduce the security risk On 2015/10/30 12:31:30, hiroshige wrote: > nit: add a period at the end of line. Done. https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:127: exceptionState.throwTypeError("Cannot construct a Request with a Request mode as navigate."); On 2015/10/30 12:31:30, hiroshige wrote: > Making the message here and L177 will be better. > > (I don't have good sentences though... > "Cannot construct a Request from a Request with 'navigate' mode and RequestInit" > and > "Mode 'navigate' cannot be specified in Request constructor"? > Not so good though...) Done. https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:137: // areAnyMembersSet will be True, if any members in RequestInit are set and On 2015/10/30 12:31:30, hiroshige wrote: > nit: This comment (L137-138) is not needed. > > IIUC "if (init.areAnyMembersSet)" is used in L139 (rather than checking |init|'s > referrer member) to implement the following spec statement: > > If any of init's members are present, run these substeps: > > Set request's referrer to "client" > > Set request's referrer policy to the empty string. > If |init|'s referrer member is not present, then |request|'s referrer is set to > "client" via the default value set in |init.referrer| in RequestInit. > yhirano@, is this correct? These comment is needed, as discussed in: https://codereview.chromium.org/1391583002/#msg14 https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:137: // areAnyMembersSet will be True, if any members in RequestInit are set and On 2015/10/30 13:18:41, yhirano wrote: > On 2015/10/30 12:31:30, hiroshige wrote: > > nit: This comment (L137-138) is not needed. > > > > IIUC "if (init.areAnyMembersSet)" is used in L139 (rather than checking > |init|'s > > referrer member) to implement the following spec statement: > > > If any of init's members are present, run these substeps: > > > Set request's referrer to "client" > > > Set request's referrer policy to the empty string. > > If |init|'s referrer member is not present, then |request|'s referrer is set > to > > "client" via the default value set in |init.referrer| in RequestInit. > > yhirano@, is this correct? > > - If no member is set on |init| => use |request|'s referrer. > - Elif "referrer" member is present => use that value. > - Else => use "client". > > In Blink, RequestInit::RequestInit implements part of the logic and I think > mentioning that would be helpful. That's why I added the comment at L125 in the > original code. Done, retained old comment, after line 129 in new code, will make code changes for these in next CL ( i.e to set request's referrer to "client" and referrer policy to the empty string). I think "no member is set on |init|", is handled as default case? https://codereview.chromium.org/1391583002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:174: // "17.If mode is "navigate", throw a TypeError. On 2015/10/30 12:31:30, hiroshige wrote: > nit: a space between "17." and "If". Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/11/02 12:21:19, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run.
CL has passed the CQ dry run, shall we commit now?. considering couple of changes will be done in separate CLs.
On 2015/11/03 11:25:47, shiva.jm wrote: > CL has passed the CQ dry run, shall we commit now?. > considering couple of changes will be done in > separate CLs. Working on CL: https://codereview.chromium.org/1426923004/ do complete other changes
On 2015/11/03 11:25:47, shiva.jm wrote: > CL has passed the CQ dry run, shall we commit now?. > considering couple of changes will be done in > separate CLs. hiroshige@ has actively reviewed, so I think it's good to wait for his approval. As I said on IRC, I expect you will make it able to catch a "mode: nativate" request in service workers shortly.
On 2015/11/06 03:14:39, yhirano wrote:
> On 2015/11/03 11:25:47, shiva.jm wrote:
> > CL has passed the CQ dry run, shall we commit now?.
> > considering couple of changes will be done in
> > separate CLs.
>
> hiroshige@ has actively reviewed, so I think it's good to wait for his
approval.
> As I said on IRC, I expect you will make it able to catch a "mode: nativate"
> request in service workers shortly.
Ok Sure, Below are the list of couple of changes will be done in separate CLs.
(1) request in service workers for "mode: nativate" and adding layout tests
for ( Request with mode == 'navigate')
(2) Using bitwise OR ('|=') for RequestInit() constructor in RequestInit.cpp
(3) Set request's referrer as per Spec, i.e implement 2,3,4 of Step 14
in Request::createRequestWithRequestOrString().
(2) and (3) will be done in CL: https://codereview.chromium.org/1426923004/.
But for (1), the service worker change made in thess CL was removed from
FrameLoadRequest.h,
since the enum order cannot be changed, as it is used for collecting the user
metrics
of the request modes. So not sure is it the right place to change Or we need to
change
at some other place in service worker side.
lgtm with a nit. However, I'm still not sure about the TypeError messages in Request.cpp. Reviewers, any suggestions? And, I think this CL and another CL for (1) should be committed together (at least in the same milestone). So, shiva.jm@, could you create a CL for (1) before committing this CL? https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:307: }, 'Request mode throw test'); nit: Change " }, 'Request mode throw test');" into " }, 'Request mode throw test');"
https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:127: exceptionState.throwTypeError("Cannot construct a Request from a Request having mode set as 'navigate' with RequestInit members being set and mode 'navigate' cannot be specified in Request constructor."); [optional] "Cannot construct a Request with a Request whose mode is 'navigate' and a non-empty RequestInit." might be clearer. https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:181: exceptionState.throwTypeError("Cannot construct a Request from a Request having mode set as 'navigate' with RequestInit members being set and mode 'navigate' cannot be specified in Request constructor."); "Cannot construct a Request with a RequestInit whose mode member is set as 'navigate'."
> But for (1), the service worker change made in thess CL was removed from > FrameLoadRequest.h, > since the enum order cannot be changed, as it is used for collecting the user > metrics > of the request modes. So not sure is it the right place to change Or we need to > change > at some other place in service worker side. Is this the change in third_party/WebKit/Source/core/loader/FrameLoadRequest.h? How the enum order matters? > And, I think this CL and another CL for (1) should be committed together (at > least in the same milestone). This is because, for example if we commit this CL in M-48 and another CL for (1) in M-49, a part of mode navigate is shipped in M-48 and another part is shipped in M-49. Since shiva.jm@ sent out Intent-to-Ship for this feature, it is better to ship the whole feature in a single release.
On 2015/11/06 07:58:46, hiroshige (slow) wrote: > > But for (1), the service worker change made in thess CL was removed from > > FrameLoadRequest.h, > > since the enum order cannot be changed, as it is used for collecting the user > > metrics > > of the request modes. So not sure is it the right place to change Or we need > to > > change > > at some other place in service worker side. > Is this the change in third_party/WebKit/Source/core/loader/FrameLoadRequest.h? > How the enum order matters? > > > And, I think this CL and another CL for (1) should be committed together (at > > least in the same milestone). > This is because, for example if we commit this CL in M-48 and another CL for (1) > in M-49, a part of mode navigate is shipped in M-48 and another part is shipped > in M-49. > Since shiva.jm@ sent out Intent-to-Ship for this feature, it is better to ship > the whole feature in a single release. Ok, will make the changes for (1), i.e for service worker in these CL itself, instead of creating separate CL. As per @horo comments in: https://codereview.chromium.org/1391583002/#msg10, i understood that FrameLoadRequest.h changes are not needed as navigate mode will be added at the end of enum values. Will try adding these service worker changes in these CL itself, but still not sure how to add layout tests for (Request with mode == 'navigate') case. @horo, pls confirm do we still need the changes suggested from your side in comments: https://codereview.chromium.org/1391583002/#msg10 for FrameLoadRequest.h ?
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
updated the patch with review comments and added service worker changes as well, pls have a look. https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:307: }, 'Request mode throw test'); On 2015/11/06 07:43:02, hiroshige (slow) wrote: > nit: > Change > " }, 'Request mode throw test');" > into > " }, 'Request mode throw test');" Done. https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:127: exceptionState.throwTypeError("Cannot construct a Request from a Request having mode set as 'navigate' with RequestInit members being set and mode 'navigate' cannot be specified in Request constructor."); On 2015/11/06 07:50:24, yhirano wrote: > [optional] "Cannot construct a Request with a Request whose mode is 'navigate' > and a non-empty RequestInit." might be clearer. Done. https://codereview.chromium.org/1391583002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Request.cpp:181: exceptionState.throwTypeError("Cannot construct a Request from a Request having mode set as 'navigate' with RequestInit members being set and mode 'navigate' cannot be specified in Request constructor."); On 2015/11/06 07:50:24, yhirano wrote: > "Cannot construct a Request with a RequestInit whose mode member is set as > 'navigate'." Done.
horo@, could you take a look at the change in FrameLoadRequest.h and related? Are the tests related for the FetchRequestMode already included in Blink and sufficient? https://codereview.chromium.org/1391583002/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html (right): https://codereview.chromium.org/1391583002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html:23: assert_equals(result.mode, 'navigate', 'request.mode'); Here (or in http/tests/serviceworker/fetch-request-fallback.html), we get |result| originating a Request with navigate mode, so we can put the test there (in resources/request-end-to-end-worker.js?) and get the test result here via |result|.
On 2015/11/13 11:45:24, hiroshige (slow) wrote: > horo@, could you take a look at the change in FrameLoadRequest.h and related? > Are the tests related for the FetchRequestMode already included in Blink and > sufficient? > > https://codereview.chromium.org/1391583002/diff/280001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html > (right): > > https://codereview.chromium.org/1391583002/diff/280001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html:23: > assert_equals(result.mode, 'navigate', 'request.mode'); > Here (or in http/tests/serviceworker/fetch-request-fallback.html), we get > |result| originating a Request with navigate mode, so we can put the test there > (in resources/request-end-to-end-worker.js?) and get the test result here via > |result|. lgtm. Sorry, I didn't noticed #73. request-end-to-end.html is checking the mode of the navigation request.
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@horo, @hiroshige, thanks for inputs.
Updated the patch for layout test case,
Request(req, {...}) where req.mode == 'navigate', should throw.
@Mark and @Jesse, after updating the patch to latest code base, got Presubmit
error for histograms.xml saying "histograms.xml is not formatted correctly, so
run pretty_print.py to fix", after running these script, some entries are
re-ordered in histograms.xml, same has been uploaded in Patch16, so pls have a
look, is it ok ?
https://codereview.chromium.org/1391583002/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html
(right):
https://codereview.chromium.org/1391583002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html:23:
assert_equals(result.mode, 'navigate', 'request.mode');
On 2015/11/13 11:45:24, hiroshige (slow) wrote:
> Here (or in http/tests/serviceworker/fetch-request-fallback.html), we get
> |result| originating a Request with navigate mode, so we can put the test
there
> (in resources/request-end-to-end-worker.js?) and get the test result here via
> |result|.
Done.
On 2015/11/16 10:08:01, shiva.jm wrote:
> @horo, @hiroshige, thanks for inputs.
> Updated the patch for layout test case,
> Request(req, {...}) where req.mode == 'navigate', should throw.
>
> @Mark and @Jesse, after updating the patch to latest code base, got Presubmit
> error for histograms.xml saying "histograms.xml is not formatted correctly, so
> run pretty_print.py to fix", after running these script, some entries are
> re-ordered in histograms.xml, same has been uploaded in Patch16, so pls have a
> look, is it ok ?
histograms.xml lgtm
It's supposed to be alphabetized. It looks like someone else already fixed the
order.
Please rebase and it should look fine again. In any case, lgtm
In case you're curious, see bug
https://code.google.com/p/chromium/issues/detail?id=556558
--mark
>
>
https://codereview.chromium.org/1391583002/diff/280001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html
> (right):
>
>
https://codereview.chromium.org/1391583002/diff/280001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html:23:
> assert_equals(result.mode, 'navigate', 'request.mode');
> On 2015/11/13 11:45:24, hiroshige (slow) wrote:
> > Here (or in http/tests/serviceworker/fetch-request-fallback.html), we get
> > |result| originating a Request with navigate mode, so we can put the test
> there
> > (in resources/request-end-to-end-worker.js?) and get the test result here
via
> > |result|.
>
> Done.
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/340001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rebased the patch to latest, pls have a look. patch18 has all the changes and tests covered.
The CQ bit was checked by shiva.jm@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, jochen@chromium.org, hiroshige@chromium.org, horo@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1391583002/#ps340001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391583002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391583002/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/75f4ce50721c692f8348e7bfcfba594272e3eb44 Cr-Commit-Position: refs/heads/master@{#360026}
Message was sent while issue was closed.
lgtm Made a followup CL to clean up the comments https://codereview.chromium.org/1451333002/
Message was sent while issue was closed.
lgtm Made a followup CL to clean up the comments https://codereview.chromium.org/1451333002/ |
