Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1451333002:
Update comments in Request constructor to explain referrer handling more clearly (Closed)
|
Created:
5 years, 1 month ago by tyoshino (SeeGerritForStatus) Modified:
5 years, 1 month ago Reviewers:
yhirano CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate comments in Request constructor to explain referrer handling more clearly
BUG=537138
Committed: https://crrev.com/b1215f56f5b904bd3abad3662a5f689ca2cd8846
Cr-Commit-Position: refs/heads/master@{#360799}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed #5 #
Total comments: 10
Patch Set 3 : Addressed #10 #Patch Set 4 : Rebase #
Messages
Total messages: 16 (5 generated)
Description was changed from ========== Update comments in Request constructor to explain referrer handling more clearly BUG=none ========== to ========== Update comments in Request constructor to explain referrer handling more clearly BUG=none ==========
tyoshino@chromium.org changed reviewers: + yhirano@chromium.org
Description was changed from ========== Update comments in Request constructor to explain referrer handling more clearly BUG=none ========== to ========== Update comments in Request constructor to explain referrer handling more clearly BUG=537138 ==========
Maybe it's good to remove any item numbers in Request::createRequestWithRequestOrString? https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:137: // constructor to "about:client" when any of |options|'s member are members https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:143: // "Set |request|'s referrer policy to the empty string." This is implemented :) https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:148: // - "If |init|'s referrerPolicy member is present, set |request|'s This is not implemented :) https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:156: // Note that JS null and undefined are encoded as an empty string, and thus redundant? https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:169: // |baseURL|. +" https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/RequestInit.cpp:52: // A part of the Request constructor algorithm is performed here. See the Please wrap comments in 80 columns.
On 2015/11/17 09:31:55, yhirano wrote: > Maybe it's good to remove any item numbers in > Request::createRequestWithRequestOrString? > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/fetch/Request.cpp (right): > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/Request.cpp:137: // constructor to > "about:client" when any of |options|'s member are > members > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/Request.cpp:143: // "Set |request|'s > referrer policy to the empty string." > This is implemented :) > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/Request.cpp:148: // - "If |init|'s > referrerPolicy member is present, set |request|'s > This is not implemented :) > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/Request.cpp:156: // Note that JS null > and undefined are encoded as an empty string, and thus > redundant? > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/Request.cpp:169: // |baseURL|. > +" > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/RequestInit.cpp:52: // A part of the > Request constructor algorithm is performed here. See the > Please wrap comments in 80 columns. @tyoshino and @yhirano, I will be doing changes for below step (1) in one of follow up CL, as communicated in CL: https://codereview.chromium.org/1391583002/ (1) // "Unset |request|'s omit-Origin-header flag." To avoid rebase again, will submit my CL after these CL commits. Also as yhirano commented below steps are already implemented: Set |request|'s referrer to "client" and Set |request|'s referrer policy to the empty string."
On 2015/11/17 09:59:51, shiva.jm wrote: > On 2015/11/17 09:31:55, yhirano wrote: > > Maybe it's good to remove any item numbers in > > Request::createRequestWithRequestOrString? > > > > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/fetch/Request.cpp (right): > > > > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/fetch/Request.cpp:137: // constructor to > > "about:client" when any of |options|'s member are > > members > > > > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/fetch/Request.cpp:143: // "Set |request|'s > > referrer policy to the empty string." > > This is implemented :) > > > > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/fetch/Request.cpp:148: // - "If |init|'s > > referrerPolicy member is present, set |request|'s > > This is not implemented :) > > > > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/fetch/Request.cpp:156: // Note that JS null > > and undefined are encoded as an empty string, and thus > > redundant? > > > > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/fetch/Request.cpp:169: // |baseURL|. > > +" > > > > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): > > > > > https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/fetch/RequestInit.cpp:52: // A part of the > > Request constructor algorithm is performed here. See the > > Please wrap comments in 80 columns. > > @tyoshino and @yhirano, > I will be doing changes for below step (1) in one of follow up CL, > as communicated in CL: https://codereview.chromium.org/1391583002/ > (1) // "Unset |request|'s omit-Origin-header flag." > To avoid rebase again, will submit my CL after these CL commits. > > Also as yhirano commented below steps are already implemented: > Set |request|'s referrer to "client" and Set |request|'s referrer policy > to the empty string." CL created for the same: https://codereview.chromium.org/1448333002/
https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:137: // constructor to "about:client" when any of |options|'s member are On 2015/11/17 09:31:55, yhirano wrote: > members Done. https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:143: // "Set |request|'s referrer policy to the empty string." On 2015/11/17 09:31:54, yhirano wrote: > This is implemented :) Fixed https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:148: // - "If |init|'s referrerPolicy member is present, set |request|'s On 2015/11/17 09:31:54, yhirano wrote: > This is not implemented :) Fixed https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:156: // Note that JS null and undefined are encoded as an empty string, and thus On 2015/11/17 09:31:54, yhirano wrote: > redundant? Removed https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Request.cpp:169: // |baseURL|. On 2015/11/17 09:31:55, yhirano wrote: > +" Done. https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/1451333002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/RequestInit.cpp:52: // A part of the Request constructor algorithm is performed here. See the On 2015/11/17 09:31:55, yhirano wrote: > Please wrap comments in 80 columns. Done.
On 2015/11/17 09:31:55, yhirano wrote: > Maybe it's good to remove any item numbers in > Request::createRequestWithRequestOrString? Done
lgtm https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:66: // - "Let temporaryBody be |input|'s request's body if |input| is a Request |temporaryBody| https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:83: // - "If request's window is an environment settings object and its origin |request| https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:84: // is same origin with entry settings object's origin, set window to |window| https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:85: // request's window." |request| https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:343: // "Set |r|'s body to |temporaryBody|. |r|'s request's body
https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:66: // - "Let temporaryBody be |input|'s request's body if |input| is a Request On 2015/11/19 11:41:15, yhirano wrote: > |temporaryBody| Done. https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:83: // - "If request's window is an environment settings object and its origin On 2015/11/19 11:41:15, yhirano wrote: > |request| Done. https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:84: // is same origin with entry settings object's origin, set window to On 2015/11/19 11:41:15, yhirano wrote: > |window| Done. https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:85: // request's window." On 2015/11/19 11:41:15, yhirano wrote: > |request| Done. https://codereview.chromium.org/1451333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:343: // "Set |r|'s body to |temporaryBody|. On 2015/11/19 11:41:15, yhirano wrote: > |r|'s request's body Done.
The CQ bit was checked by tyoshino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1451333002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1451333002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1451333002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b1215f56f5b904bd3abad3662a5f689ca2cd8846 Cr-Commit-Position: refs/heads/master@{#360799} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
