Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
347653003:
XMLHttpRequest must build a request using the given method as-is for non-standard types (Closed)
DescriptionXMLHttpRequest must build a request using the given method as-is for non-standard types
As per step-5 of http://xhr.spec.whatwg.org/#the-open()-method only DELETE, PUT, GET, POST, OPTIONS, HEAD are standard. Use other methods as is without case change.
We now pass all cases from w3c-test : http://w3c-test.org/XMLHttpRequest/open-method-case-sensitive.htm.
BUG=386347
TEST=http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176592
Patch Set 1 #
Total comments: 12
Patch Set 2 : Write async tests #
Total comments: 8
Patch Set 3 : update review comments and rebase #
Messages
Total messages: 27 (0 generated)
PTAL
What do other browsers do? It's not clear to me that this will be web-compatible.
On 2014/06/18 23:20:18, eseidel wrote: > What do other browsers do? It's not clear to me that this will be > web-compatible. Both Firefox and IE10+ passes all cases.
I can't imagine this has a measureable impact on performance. :) I worry that if this could break sites. The risk of doing so it lessened by IE/FF sharing this behavior, but mostly lessened for desktop content since IE/FF are used very little for mobile web browsing as far as I understand.
lgtm OK, I see now. This is only about whether to uppercase the type name. If that's true then this would require a bad server to be case-sensitive about one of these types, which seems unlikely.
On 2014/06/19 00:11:39, eseidel wrote: > I can't imagine this has a measureable impact on performance. :) > > I worry that if this could break sites. The risk of doing so it lessened by > IE/FF sharing this behavior, but mostly lessened for desktop content since IE/FF > are used very little for mobile web browsing as far as I understand. Agree :) removed the message from description.
On 2014/06/19 00:13:45, eseidel wrote: > lgtm > > OK, I see now. This is only about whether to uppercase the type name. If that's > true then this would require a bad server to be case-sensitive about one of > these types, which seems unlikely. Thanks! yes that is true, very unlikely servers to have case sensitive method checks. However Cl will pass four more from w3c-test suites which other browsers pass already.
The CQ bit was checked by mahesh.kk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/347653003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/12617)
The CQ bit was checked by mahesh.kk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/347653003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/12632)
https://codereview.chromium.org/262173002/ wasn't carried forward for some reason, but it makes the same compatibility improvement. You may want to have a look at it -- its test is async, I notice. https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... File LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html (right): https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:10: if (window.testRunner) I would have preferred if this (new) test used js-test.js (or testharness.js) https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:17: function method(method) { How about 'verb' (or methodName) as the argument name? https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:19: client.open(method, "resources/request-method.php", false); You could run into test timeout problems here, sync XHR is slow and flaky on our trybots (esp on Windows.) Please keep an eye out for flakiness (and if so, I think you need to split this test up a bit.) https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:24: method("PUT") Add semicolons to these calls. https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... File LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html (right): https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html:17: function method(method) { method => verb https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html:18: var client = new XMLHttpRequest() Use ";" consistently
Thanks for the review sof! On 2014/06/19 06:19:50, sof wrote: > https://codereview.chromium.org/262173002/ wasn't carried forward for some > reason, but it makes the same compatibility improvement. You may want to have a > look at it -- its test is async, I notice. > Ah, I had no idea. One thing review tool misses is search option! Thanks for digging that out. I missed the async part of method-names.html, seems to do xml sync and sync layout testcase. > https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... > File > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html > (right): > > https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:10: > if (window.testRunner) > I would have preferred if this (new) test used js-test.js (or testharness.js) > Ok, this test is mostly as is per w3c-test suite from link above in description. > https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:17: > function method(method) { > How about 'verb' (or methodName) as the argument name? > > https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:19: > client.open(method, "resources/request-method.php", false); > You could run into test timeout problems here, sync XHR is slow and flaky on our > trybots (esp on Windows.) Please keep an eye out for flakiness (and if so, I > think you need to split this test up a bit.) > Ah good to know, I can move it to async test if thats more stable. > https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:24: > method("PUT") > Add semicolons to these calls. > > https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... > File > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html > (right): > > https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html:17: > function method(method) { > method => verb > > https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html:18: > var client = new XMLHttpRequest() > Use ";" consistently sure.
Let's update the subject and the first line of the CL description to be more specific. For example, XMLHttpRequest must build a request using the given method as-is (no case conversion).
Please use the interface to associate your reply with the original review comments as sof suggested at https://codereview.chromium.org/329323002/#msg21 On side by side page, when you click each comment, it opens to show complete message and also shows anchors "Reply" and "Done". If you reply to the comment by clicking "Reply" and writing your reply in the opened text box, your reply will be associated with the original comment you replied. Then, you can see whole conversation history easily.
PTAL. https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... File LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html (right): https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:10: if (window.testRunner) On 2014/06/19 06:19:50, sof wrote: > I would have preferred if this (new) test used js-test.js (or testharness.js) Done. However dumpAsText is still required though! https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:17: function method(method) { On 2014/06/19 06:19:50, sof wrote: > How about 'verb' (or methodName) as the argument name? Ok sure. This code is mostly used as is per w3c-test suite from http://w3c-test.org/XMLHttpRequest/open-method-case-sensitive.htm. https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:19: client.open(method, "resources/request-method.php", false); On 2014/06/19 06:19:49, sof wrote: > You could run into test timeout problems here, sync XHR is slow and flaky on our > trybots (esp on Windows.) Please keep an eye out for flakiness (and if so, I > think you need to split this test up a bit.) Done. https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:24: method("PUT") On 2014/06/19 06:19:50, sof wrote: > Add semicolons to these calls. Done. https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... File LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html (right): https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html:17: function method(method) { On 2014/06/19 06:19:50, sof wrote: > method => verb Done. https://codereview.chromium.org/347653003/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html:18: var client = new XMLHttpRequest() On 2014/06/19 06:19:50, sof wrote: > Use ";" consistently Done.
On 2014/06/19 16:55:44, tyoshino wrote: > Let's update the subject and the first line of the CL description to be more > specific. > > For example, > > XMLHttpRequest must build a request using the given method as-is (no case > conversion). You are right, we do build request using given method as-is except for upper-casing standard names per spec, DELETE, PUT, GET, POST, OPTIONS, HEAD. How does this sound? XMLHttpRequest must build a request using the given method as-is for non-standard types
On 2014/06/19 19:36:36, maheshkk wrote: > How does this sound? > XMLHttpRequest must build a request using the given method as-is for > non-standard types Looks good!
lgtm https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... File LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html (right): https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:12: window.testRunner.dumpAsText(); this is included in testharnessreport.js https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:14: function testMethod(methodName, lastTest) { lastTest is not used? https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:16: client.open(methodName, "resources/request-method.php"); how about naming this echo-request-method.php? https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... File LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html (right): https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html:12: window.testRunner.dumpAsText(); unnecessary
Thanks for the review! https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... File LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html (right): https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:12: window.testRunner.dumpAsText(); On 2014/06/20 02:13:34, tyoshino wrote: > this is included in testharnessreport.js Done. https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:14: function testMethod(methodName, lastTest) { On 2014/06/20 02:13:34, tyoshino wrote: > lastTest is not used? Done. Had it for async_test usage but got it working with test() of testharness.js. https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-allowed.html:16: client.open(methodName, "resources/request-method.php"); On 2014/06/20 02:13:34, tyoshino wrote: > how about naming this echo-request-method.php? Done. https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... File LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html (right): https://codereview.chromium.org/347653003/diff/20001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-open-method-case-insensitive.html:12: window.testRunner.dumpAsText(); On 2014/06/20 02:13:34, tyoshino wrote: > unnecessary Done.
The CQ bit was checked by mahesh.kk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/347653003/40001
Message was sent while issue was closed.
Change committed as 176592 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
