Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2060093002:
Allow 'history.pushState' to modify query string for unique and file origins. (Closed)
|
Created:
4 years, 6 months ago by Peng Xinchao Modified:
4 years, 6 months ago CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow 'history.pushState' to modify query string for unique and file origins.
Add function equalIgnoringQueryAndFragment to ignore query and fragment for History.
BUG=618989
Committed: https://crrev.com/c21f0b11ac83ea970d0eaf6a0b223d48a32a4b32
Cr-Commit-Position: refs/heads/master@{#400418}
Patch Set 1 #Patch Set 2 : Allow 'history.pushState' to modify query string for unique and file origins. #
Total comments: 6
Patch Set 3 : Allow 'history.pushState' to modify query string for unique and file origins. #
Total comments: 9
Patch Set 4 : Allow 'history.pushState' to modify query string for unique and file origins. #
Messages
Total messages: 22 (8 generated)
Description was changed from ========== fix the issue of pushState url with '?' the function equalIgnoringFragmentIdentifier is to ignore '#', it can not ignore '?' BUG=618989 ========== to ========== fix the issue of pushState url with '?' the function equalIgnoringFragmentIdentifier is to ignore '#', it can not ignore '?' BUG=618989 ==========
pxinchao@gmail.com changed reviewers: + japhet@chromium.org
Description was changed from ========== fix the issue of pushState url with '?' the function equalIgnoringFragmentIdentifier is to ignore '#', it can not ignore '?' BUG=618989 ========== to ========== fix the issue of history.pushState() with "?" for file:// URLs the function equalIgnoringFragmentIdentifier is to ignore '#', it can not ignore '?' BUG=618989 ==========
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
This looks like a pretty reasonable change, but please add some tests to verify the behavior. https://codereview.chromium.org/1632513002 has some examples of layout- and unit-tests that might be useful for you to repurpose for the new behavior. As it looks like this might be your first patch, please also make sure you take a look at https://cla.developers.google.com/, sign the contributor license agreements, and add yourself to the AUTHORS file in the root of the repository.
Actually, sorry, I misread the patch. As written, this allows fragment, query, and path modification. As noted in the bug, I don't think it's a good idea to allow path modifications for these kinds of URLs. Fragment is benign, query is not terribly dangerous, path is potentially confusing, especially for `file:` URLs. Please adjust the patch to exclude path modifications. Also, I'd appreciate it if you cleaned up the CL description a bit. I'd suggest something like: """ Allow 'history.pushState' to modify query string for unique origins. https://codereview.chromium.org/1632513002 carved out an exception to `pushState`'s defined behavior in unique origins[1], allowing fragment identifier updates. This patch extends that exception to ... [1]: https://html.spec.whatwg.org/multipage/browsers.html#dom-history-pushstate """ Thanks!
Description was changed from ========== fix the issue of history.pushState() with "?" for file:// URLs the function equalIgnoringFragmentIdentifier is to ignore '#', it can not ignore '?' BUG=618989 ========== to ========== Allow 'history.pushState' to modify query string for unique and file origins. Add function equalIgnoringQueryAndFragment to ignore query and fragment for History. BUG=618989 ==========
On 2016/06/12 16:17:36, Mike West wrote: > Actually, sorry, I misread the patch. As written, this allows fragment, query, > and path modification. As noted in the bug, I don't think it's a good idea to > allow path modifications for these kinds of URLs. Fragment is benign, query is > not terribly dangerous, path is potentially confusing, especially for `file:` > URLs. Please adjust the patch to exclude path modifications. > > Also, I'd appreciate it if you cleaned up the CL description a bit. I'd suggest > something like: > > """ > Allow 'history.pushState' to modify query string for unique origins. > > https://codereview.chromium.org/1632513002 carved out an exception to > `pushState`'s > defined behavior in unique origins[1], allowing fragment identifier updates. > This > patch extends that exception to ... > > [1]: https://html.spec.whatwg.org/multipage/browsers.html#dom-history-pushstate > > """ > > Thanks! I ask other question: If you've never submitted code before, you must add your (or your organization's) name and contact info to the AUTHORS file for Chromium or Chromium OS. How to add my name ?
On 2016/06/14 06:40:12, pxinchao wrote: > On 2016/06/12 16:17:36, Mike West wrote: > > Actually, sorry, I misread the patch. As written, this allows fragment, query, > > and path modification. As noted in the bug, I don't think it's a good idea to > > allow path modifications for these kinds of URLs. Fragment is benign, query is > > not terribly dangerous, path is potentially confusing, especially for `file:` > > URLs. Please adjust the patch to exclude path modifications. > > > > Also, I'd appreciate it if you cleaned up the CL description a bit. I'd > suggest > > something like: > > > > """ > > Allow 'history.pushState' to modify query string for unique origins. > > > > https://codereview.chromium.org/1632513002 carved out an exception to > > `pushState`'s > > defined behavior in unique origins[1], allowing fragment identifier updates. > > This > > patch extends that exception to ... > > > > [1]: > https://html.spec.whatwg.org/multipage/browsers.html#dom-history-pushstate > > > > """ > > > > Thanks! > I want to ask other question: "If you've never submitted code before, you must add your (or your organization's) name and contact info to the AUTHORS file for Chromium or Chromium OS." How to add my name to AUTHORS file ? thank you
On 2016/06/14 at 07:16:56, pxinchao wrote: > On 2016/06/14 06:40:12, pxinchao wrote: > > On 2016/06/12 16:17:36, Mike West wrote: > > > Actually, sorry, I misread the patch. As written, this allows fragment, query, > > > and path modification. As noted in the bug, I don't think it's a good idea to > > > allow path modifications for these kinds of URLs. Fragment is benign, query is > > > not terribly dangerous, path is potentially confusing, especially for `file:` > > > URLs. Please adjust the patch to exclude path modifications. > > > > > > Also, I'd appreciate it if you cleaned up the CL description a bit. I'd > > suggest > > > something like: > > > > > > """ > > > Allow 'history.pushState' to modify query string for unique origins. > > > > > > https://codereview.chromium.org/1632513002 carved out an exception to > > > `pushState`'s > > > defined behavior in unique origins[1], allowing fragment identifier updates. > > > This > > > patch extends that exception to ... > > > > > > [1]: > > https://html.spec.whatwg.org/multipage/browsers.html#dom-history-pushstate > > > > > > """ > > > > > > Thanks! > > > I want to ask other question: > "If you've never submitted code before, you must add your (or your organization's) name and contact info to the AUTHORS file for Chromium or Chromium OS." > How to add my name to AUTHORS file ? thank you Just edit the `AUTHORS` file at the root of the repository, and add it to this CL. :)
Thanks! I have some comments that I've left inline. :) https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html (right): https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:1: <body> Please also adjust `LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php` to check `?` behavior, as well as a combination of query and fragment. https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:6: if(window.location.protocol=="file:") { Nit: Space between `if` and `(`. https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:8: history.pushState({}, 'dummyTitle', '#foo=bar'); Nit: Indentation. https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:9: history.pushState({}, 'dummyTitle', '?foo=bar'); Please also add `?foo#bar`. https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:16: } New tests should be written in terms of `testharness.js` assertions (because they can then be pretty easily upstreamed to Web Platform Tests, and used in other browsers). `LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php` gives you a pretty good example of what that kind of test structure might look like. https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.cpp (right): https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:86: } You can write this more simply by copy/pasting `equalIgnoringPathQueryAndFragment`'s body, then replacing `pathStart` with `pathEnd`. :)
On 2016/06/14 08:44:59, Mike West wrote: > Thanks! I have some comments that I've left inline. :) > > https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html > (right): > > https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:1: > <body> > Please also adjust > `LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php` to > check `?` behavior, as well as a combination of query and fragment. > > https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:6: > if(window.location.protocol=="file:") { > Nit: Space between `if` and `(`. > > https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:8: > history.pushState({}, 'dummyTitle', '#foo=bar'); > Nit: Indentation. > > https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:9: > history.pushState({}, 'dummyTitle', '?foo=bar'); > Please also add `?foo#bar`. > > https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/html/pushstate-ignore-query-file.html:16: } > New tests should be written in terms of `testharness.js` assertions (because > they can then be pretty easily upstreamed to Web Platform Tests, and used in > other browsers). > `LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php` gives > you a pretty good example of what that kind of test structure might look like. > > https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/History.cpp (right): > > https://codereview.chromium.org/2060093002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/History.cpp:86: } > You can write this more simply by copy/pasting > `equalIgnoringPathQueryAndFragment`'s body, then replacing `pathStart` with > `pathEnd`. :) Thank for your reviewed. i motified ,please review.
Looks good. If you can clean up the whitespace, and explain or change the length calculations, I'd be happy to land it for you. https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php (right): https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php:28: }, 'pushState ?hash in unique origin should not fail with SecurityError'); Nit: Can you add a newline between tests? Also above, where we apparently missed it in the initial patch. https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-at-unique-origin-denied.php (right): https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-at-unique-origin-denied.php:33: }, 'pushState ?hash in unique origin should not fail with SecurityError'); Nit: Newlines, as above. https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.cpp (right): https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:64: int aLength = (a.parsed().query.len >= 0) ? (a.parsed().query.begin - 1) : ((a.parsed().ref.len >= 0) ? (a.parsed().ref.begin - 1) : (a.getString().length())); Is `a.pathEnd()` not correct here? I don't understand why you need to examine the length of the query and fragment individually. https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:65: int bLength = (b.parsed().query.len >= 0) ? (b.parsed().query.begin - 1) : ((b.parsed().ref.len >= 0) ? (b.parsed().ref.begin - 1) : (b.getString().length())); Ditto. https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:77: } Nit: Newline.
On 2016/06/16 09:23:13, Mike West (OOO - BlinkOn) wrote: > Looks good. If you can clean up the whitespace, and explain or change the length > calculations, I'd be happy to land it for you. > > https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php > (right): > > https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php:28: > }, 'pushState ?hash in unique origin should not fail with SecurityError'); > Nit: Can you add a newline between tests? Also above, where we apparently missed > it in the initial patch. > > https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-at-unique-origin-denied.php > (right): > > https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-at-unique-origin-denied.php:33: > }, 'pushState ?hash in unique origin should not fail with SecurityError'); > Nit: Newlines, as above. > > https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/History.cpp (right): > > https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/History.cpp:64: int aLength = > (a.parsed().query.len >= 0) ? (a.parsed().query.begin - 1) : > ((a.parsed().ref.len >= 0) ? (a.parsed().ref.begin - 1) : > (a.getString().length())); > Is `a.pathEnd()` not correct here? I don't understand why you need to examine > the length of the query and fragment individually. > > https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/History.cpp:65: int bLength = > (b.parsed().query.len >= 0) ? (b.parsed().query.begin - 1) : > ((b.parsed().ref.len >= 0) ? (b.parsed().ref.begin - 1) : > (b.getString().length())); > Ditto. > > https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/History.cpp:77: } > Nit: Newline. Thank for your help . Mofitied ,please review!
https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php (right): https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php:28: }, 'pushState ?hash in unique origin should not fail with SecurityError'); On 2016/06/16 09:23:13, Mike West wrote: > Nit: Can you add a newline between tests? Also above, where we apparently missed > it in the initial patch. Done. https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-at-unique-origin-denied.php (right): https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-at-unique-origin-denied.php:33: }, 'pushState ?hash in unique origin should not fail with SecurityError'); On 2016/06/16 09:23:13, Mike West wrote: > Nit: Newlines, as above. Done. https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.cpp (right): https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:64: int aLength = (a.parsed().query.len >= 0) ? (a.parsed().query.begin - 1) : ((a.parsed().ref.len >= 0) ? (a.parsed().ref.begin - 1) : (a.getString().length())); On 2016/06/16 09:23:13, Mike West wrote: > Is `a.pathEnd()` not correct here? I don't understand why you need to examine > the length of the query and fragment individually. Done. You are right , i misunderstand path of URL https://codereview.chromium.org/2060093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:77: } On 2016/06/16 09:23:13, Mike West wrote: > Nit: Newline. Done.
LGTM. Thanks! I'll throw this at the bots.
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2060093002/60001
Message was sent while issue was closed.
Description was changed from ========== Allow 'history.pushState' to modify query string for unique and file origins. Add function equalIgnoringQueryAndFragment to ignore query and fragment for History. BUG=618989 ========== to ========== Allow 'history.pushState' to modify query string for unique and file origins. Add function equalIgnoringQueryAndFragment to ignore query and fragment for History. BUG=618989 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allow 'history.pushState' to modify query string for unique and file origins. Add function equalIgnoringQueryAndFragment to ignore query and fragment for History. BUG=618989 ========== to ========== Allow 'history.pushState' to modify query string for unique and file origins. Add function equalIgnoringQueryAndFragment to ignore query and fragment for History. BUG=618989 Committed: https://crrev.com/c21f0b11ac83ea970d0eaf6a0b223d48a32a4b32 Cr-Commit-Position: refs/heads/master@{#400418} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c21f0b11ac83ea970d0eaf6a0b223d48a32a4b32 Cr-Commit-Position: refs/heads/master@{#400418} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
