XMLHttpRequest should have a responseURL attribute (added in recent XHR specification drafts)

[Shivakumar J M]

The XMLHttpRequest should suppourt method responseURL As per latest Spec https://xhr.spec.whatwg.org/#the-responseurl-attribute.

[Shivakumar J M]

Working for Fix and adding new tests, will submit the patch soon.

[Shivakumar J M]

Created attachment 238368 [details]
Patch

The XMLHttpRequest should suppourt method responseURL As per latest Spec.

[Darin Adler]

Comment on attachment 238368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238368&action=review

Patch is OK, but not quite right, and we need a bit more testing.

> Source/WebCore/xml/XMLHttpRequest.cpp:343
> +    if (!m_response.url().string().isNull())
> +        return m_response.url().string();
> +
> +    return "";

This check for null isn’t needed because the JavaScript binding layer should already turn null strings into empty strings unless we use keywords to tell it not to do so. It’s also possibly inefficient to call m_response.url().string() twice; the compiler might not be able to optimize the common subexpression. And further, the correct efficient idiom for an empty string is emptyString(), not "".

We need test coverage for this. The test doesn’t currently seem to cover the null case and it needs to.

> Source/WebCore/xml/XMLHttpRequest.h:135
> +    String responseURL();

I think this should be a const member function. Any reason to have it not be one?

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL-expected.txt:9
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"

This test is inadequate. In this case the response URL is exactly the same as the request URL; the primary feature of response URL is to expose cases where the two aren't the same, and we need to make a test that covers that. The cases where a response URL might be different from the request URL would at least include things like redirects. I’d like to understand the complete set of cases where the response URL could be different from the request and have coverage for those. Our HTTP tests are set up to be able to deal with things like redirects.

We also need a test for the case where the response URL is null to test our code that turns it into the empty string rather than a JavaScript null.

I also don’t understand why we get exactly 5 pieces. Is that reliable or could this test be flaky?

[Alexey Proskuryakov]

Comment on attachment 238368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238368&action=review

responseURL only exists in WhatWG version of XHR spec, <https://xhr.spec.whatwg.org/#the-responseurl-attribute>, and this spec does not appear to be very polished yet. Has it been through any kind of security review? Do other browsers implement this?

The definition of responseURL is: "The responseURL attribute must return the empty string if response's url is null and its serialization otherwise." The definition of response's url is in Fetch spec, <https://fetch.spec.whatwg.org/#concept-response-url>.

But in the Fetch spec, it's defined to always be the request URL, it doesn't change with redirects!

Another thing that I noticed is that the Fetch spec has a concept of Response object and its URL, which filters out fragment identifiers - and XHR spec does not filter them out for responseURL. This is an inconsistency that needs to be resolved.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:3
> +<script src="../../../../js-test-resources/js-test.js"></script>

The relative path is not needed, can be just "/js-test-resources/js-test.js".

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:14
> +    if (req.readyState == 3) {

We should test responseURL in every state, not just in state 3.

We should test various unusual URLs in Location field, notably:
- ones with fragments;
- ones with user credentials;
- ones that are absolute vs. relative.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:30
> +<script src="../../../../js-test-resources/js-test.js"></script>

Same comment about path.

[Shivakumar J M]

(In reply to comment #4)
> (From update of attachment 238368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238368&action=review
> 
> responseURL only exists in WhatWG version of XHR spec, <https://xhr.spec.whatwg.org/#the-responseurl-attribute>, and this spec does not appear to be very polished yet. Has it been through any kind of security review? Do other browsers implement this?
> 
> The definition of responseURL is: "The responseURL attribute must return the empty string if response's url is null and its serialization otherwise." The definition of response's url is in Fetch spec, <https://fetch.spec.whatwg.org/#concept-response-url>.
> 
> But in the Fetch spec, it's defined to always be the request URL, it doesn't change with redirects!
> 
> Another thing that I noticed is that the Fetch spec has a concept of Response object and its URL, which filters out fragment identifiers - and XHR spec does not filter them out for responseURL. This is an inconsistency that needs to be resolved.
> 
> > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:3
> > +<script src="../../../../js-test-resources/js-test.js"></script>
> 
> The relative path is not needed, can be just "/js-test-resources/js-test.js".
> 
> > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:14
> > +    if (req.readyState == 3) {
> 
> We should test responseURL in every state, not just in state 3.
> 
> We should test various unusual URLs in Location field, notably:
> - ones with fragments;
> - ones with user credentials;
> - ones that are absolute vs. relative.
> 
> > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:30
> > +<script src="../../../../js-test-resources/js-test.js"></script>
> 
> Same comment about path.

Dear Darin and Alexey,

     Thanks for the inputs, below are my observations:
	 
     1) Since null check is not needed, we can just have these code "return m_response.url().string();" in XMLHttpRequest::responseURL() API, will add const to these new XMLHttpRequest::responseURL() API.
     2) I will add more tests for response URL null case, the response URL is not exactly the same as the request URL case and redirect cases etc. 
     3) I will check why, in existing test case exactly 5 pieces are coming. Is that reliable or could this test be flaky?	 
     4) This spec still needs to be polished and undergo security review. 
I just checked Blink latest version, responseURL() API has been added in Blink.
     5) I will add tests for various unusual URLs in Location field, ones with fragments, ones with user credentials, ones that are absolute vs. relative.
     6) I will make correction for relative path in test cases, will test responseURL in every state.

[Alexey Proskuryakov]

As the spec appears to be wrong, it seems necessary to bring this up with people who maintain it.

[Shivakumar J M]

Created attachment 238590 [details]
Patch-Updated-Review

Updated the Code and Tests as per comments, tried using async_test(), but using Promise object looks good and reduce the code, got the clue from Blink and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise.
I will mail the clarification for Spec to Anne van Kesteren (Mozilla) <[e-mail]> to get more clarity.

[Shivakumar J M]

Anne van Kesteren has updated the Specs for comments sent in mail:
https://github.com/whatwg/fetch/commits  (  https://github.com/whatwg/fetch/commit/0c96b1e11131c542877a21d486d2e5a51a361e41 and  https://github.com/whatwg/xhr/commit/9fd9bb483001231f55f380a319b00ca32bdb5222 )

[Shivakumar J M]

Dear Darin and Alexey,


     Anne Kesteren has updated the Specs, so should we need to rebase the patch or shall we wait for inputs from syoichi and xKhorasan.


Thanks
Shiva

[Alexey Proskuryakov]

I intend to look into this in the next few days.

[Shivakumar J M]

(In reply to comment #10)
> I intend to look into this in the next few days.


Ok fine, will rebase the patch based on your inputs.

[Alexey Proskuryakov]

Comment on attachment 238590 [details]
Patch-Updated-Review

View in context: https://bugs.webkit.org/attachment.cgi?id=238590&action=review

Thank you for updating the patch, and sorry that it took me long to review. I think that test coverage still needs to be substantially extended, and another round of review would help.

You did not add any tests for fragment in URL. Please add them. I suspect that a code change may be needed to make that work correctly.

Please add a test for what happens for a cross-origin redirect - both successful an unsuccessful cases. Please add a test for redirecting to a URL with credentials, too.

It is not necessary to add all test cases in in file - in fact, it is often preferable to split them, because then a file name serves an an explanation of what exactly is being tested, and because it's easier to debug small tests when something breaks.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:13
> +    return new Promise(function(resolve, reject) {
> +    var req = new XMLHttpRequest();

Style nitpick: code inside the promise handler should be indented.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:14
> +    req.open('GET', url);

We need to test what responseURL does in UNSENT state, i.e. before calling open().

We also need to test what it does immediately after calling open().

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:21
> +    req.send();

We need to test what responseURL does after calling send(). This doesn't change XHR state, so onreadystatechange is not called.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:28
> +window.responseURL = req.responseURL;
> +shouldBeEqualToString('responseURL', 'http://127.0.0.1:8000/xmlhttprequest/resources/reply.txt');
> +return runTest('resources/redirect_methods.php?url=reply.xml', 'document');

Please indent function bodies.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:40
> +return runTest('resources/network-simulator.php?command=connect/', 'text');

What specifically does this test? Changing network simulator state here will probably not break anything, but it's surprising anyway.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:46
> + testFailed(String(reason));

Please use 4 spaces to indent.

[Shivakumar J M]

Created attachment 239712 [details]
Patch-Updated-Review2

Updated the test cases as per comments, fragments in URL are filtered.

[Build Bot]

Comment on attachment 239712 [details]
Patch-Updated-Review2

Attachment 239712 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4585270334717952

New failing tests:
http/tests/xmlhttprequest/xmlhttprequest-responseURL.html
http/tests/xmlhttprequest/cross-origin-redirect-responseURL.html

[Build Bot]

Created attachment 239716 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239712 [details]
Patch-Updated-Review2

Attachment 239712 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6398840005459968

New failing tests:
http/tests/xmlhttprequest/xmlhttprequest-responseURL.html
http/tests/xmlhttprequest/cross-origin-redirect-responseURL.html
http/tests/xmlhttprequest/redirect-credentials-responseURL.html

[Build Bot]

Created attachment 239717 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 239712 [details]
Patch-Updated-Review2

View in context: https://bugs.webkit.org/attachment.cgi?id=239712&action=review

While there are other problems on EWS, the tests do show both of the new tests failing:

Regressions: Unexpected text-only failures (2)
  http/tests/xmlhttprequest/cross-origin-redirect-responseURL.html [ Failure ]
  http/tests/xmlhttprequest/xmlhttprequest-responseURL.html [ Failure ]

We will have to deal with that before we can commit this to the server. review- just because of the test failures; otherwise looks fine

> Source/WebCore/ChangeLog:3
> +        The XMLHttpRequest should suppourt method responseURL As per latest Spec.

Grammar: XMLHttpRequest does not need a The
Typo: support
Capitalization: as
Capitalize: spec.

responseURL is an attribute, not a method

> Source/WebCore/ChangeLog:12
> +        The XMLHttpRequest should suppourt method responseURL.

This just repeats the title above and should thus be omitted.

[Shivakumar J M]

Created attachment 239777 [details]
Patch-Updated-Review3

Updated the test cases expected files, but these tests are passed in my local build machine for EFL port.
As per build bot test results, it looks fragments in URL are not filtered.

[Build Bot]

Comment on attachment 239777 [details]
Patch-Updated-Review3

Attachment 239777 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4744600065409024

New failing tests:
http/tests/xmlhttprequest/redirect-credentials-responseURL.html

[Build Bot]

Created attachment 239779 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Shivakumar J M]

Created attachment 239783 [details]
Patch-Updated-Review4

Updated the test cases expected files, since mac-wk2 failed for previous patch, but mac-wk passed.

[Build Bot]

Comment on attachment 239783 [details]
Patch-Updated-Review4

Attachment 239783 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5123868226224128

New failing tests:
http/tests/xmlhttprequest/redirect-credentials-responseURL.html

[Build Bot]

Created attachment 239786 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Shivakumar J M]

Created attachment 239793 [details]
Patch-Updated-Review5

Updated the test cases expected files, Also moved auth related tests to basic-auth.html, since mac-wk, mac-wk2 bot was giving different results.
As per build bot test results, it looks fragments in URL are not filtered.

[Alexey Proskuryakov]

> As per build bot test results, it looks fragments in URL are not filtered.

Should they be? How did you determine that?

[Shivakumar J M]

(In reply to comment #26)
> > As per build bot test results, it looks fragments in URL are not filtered.
> 
> Should they be? How did you determine that?


Dear Alexey,

     Below are my observations:

     After testing with fragment URL: "resources/supported-xml-content-types.cgi?type=foo#bar/baz+xml" using xmlhttprequest-responseURL.html test file in my local Linux build setup, the returned response URL was "http://127.0.0.1:8000/xmlhttprequest/resources/supported-xml-content-types.cgi?type=foo", so i felt the fragment in URL is getting filtered.

     But when these tests ran in build bot, i got the returned response URL was "http://127.0.0.1:8000/xmlhttprequest/resources/supported-xml-content-types.cgi?type=foo#bar/baz+xml", so i felt fragment in URL are not filtered.


     As per https://url.spec.whatwg.org/#concept-url-serializer ( "If the exclude fragment flag is unset and url's fragment is non-null, append "#" concatenated with url's fragment to output"), if exclude fragment flag is set then only the fragment in URL will be filtered. But i saw URL.cpp, where fragments are addressed, but could not get based on fragment flag how the filtering is happening.

     pls share your inputs, if my understanding is not correct.

Thanks

[Alexey Proskuryakov]

The WhatWG spec got updated, and now it says this: "The responseURL attribute must return the empty string if response's url is null and its serialization with the exclude fragment flag set otherwise."

So, latest Anne's thinking is that the fragment needs to be omitted.

There is one more thing I noticed in the spec, which also appears to be a change since after you started working on this bug. The type of responseURL is now USVString, not DOMString. I don't understand why it's so, or even what the difference really is, but we need to capture this quirk in a regression test, too.

[Alexey Proskuryakov]

Comment on attachment 239793 [details]
Patch-Updated-Review5

View in context: https://bugs.webkit.org/attachment.cgi?id=239793&action=review

r- for the fragment issue.

I also looked over test syntax and output but didn't deeply think about test semantics yet.

> LayoutTests/http/tests/xmlhttprequest/basic-auth-expected.txt:4
> +TEST COMPLETE
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async"

The test should only say "TEST COMPLETE" once it's actually complete.

To achieve this, you should switch from waitUntilDone/notifyDone to jsTestIsAsync=true/finishJSTest().

> LayoutTests/http/tests/xmlhttprequest/basic-auth-expected.txt:10
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async2"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async3"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async4"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async5"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async6"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async7"

This output is out of order. The test should use a single mechanism for output to avoid that.

[Shivakumar J M]

(In reply to comment #29)
> (From update of attachment 239793 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239793&action=review
> 
> r- for the fragment issue.
> 
> I also looked over test syntax and output but didn't deeply think about test semantics yet.
> 
> > LayoutTests/http/tests/xmlhttprequest/basic-auth-expected.txt:4
> > +TEST COMPLETE
> > +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async"
> 
> The test should only say "TEST COMPLETE" once it's actually complete.
> 
> To achieve this, you should switch from waitUntilDone/notifyDone to jsTestIsAsync=true/finishJSTest().
> 
> > LayoutTests/http/tests/xmlhttprequest/basic-auth-expected.txt:10
> > +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async2"
> > +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async3"
> > +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async4"
> > +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async5"
> > +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async6"
> > +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=async7"
> 
> This output is out of order. The test should use a single mechanism for output to avoid that.


Yes right, so fragment needs to be omitted based on whether fragment flag is set or unset, so do we need to address these in separate bug?.

The type of responseURL is now USVString, so which regression tests we need to update?. i did not get these comment correctly.

I will updated the basic-auth.html tests or will move these tests to new file and resubmit the patch again.

[Darin Adler]

(In reply to comment #30)
> Yes right, so fragment needs to be omitted based on whether fragment flag is set or unset, so do we need to address these in separate bug?.

No. We should not introduce this new attribute with incorrect behavior and then write a new bug to correct what's wrong with it. Note that the fragment flag is a specification concept to help us understand the behavior of URL serialization in various contexts. I’m not sure we literally need a flag at runtime to implement this attribute.

> The type of responseURL is now USVString, so which regression tests we need to update?

I don’t understand your question. The type is a string (and the specifications are now using USVString as the name for the string type); but what does that type have to do with updating tests?

[Alexey Proskuryakov]

> > The type of responseURL is now USVString, so which regression tests we need to update?
> 
> I don’t understand your question. The type is a string (and the specifications are now using USVString as the name for the string type); but what does that type have to do with updating tests?

I asked to add a new test case that captures a difference between DOMString and USVString. The patch was originally written when the spec said that the type was DOMString, and now the type changed to USVString. Is that observable?

If it observable, then the patch disagrees with either old or new version of the spec, but there is nothing in our tests that would catch that. And I do not know if the patch agrees with the new spec; having a test would help understand the difference.

[Shivakumar J M]

Created attachment 240115 [details]
Patch-Updated-Review6

Updated the patch with tests and added suppourt to handle fragments.

[Shivakumar J M]

Created attachment 240116 [details]
Annes-conversation

As Discussed with Spec author Anne, USVString is the new suppourt in WebIDL ( http://heycam.github.io/webidl/#idl-USVString).
Anne is still not in sure in using USVString, so in IDL file we can use DOMString, as confired in mail conversation attached (Annes-conversation.txt).
Also fragment flag is set in responseURL case, so we need to filter fragments in responseURL, as confired in mail conversation attached ( Annes-conversation.txt).

[Alexey Proskuryakov]

Comment on attachment 240115 [details]
Patch-Updated-Review6

View in context: https://bugs.webkit.org/attachment.cgi?id=240115&action=review

http/tests/xmlhttprequest/xmlhttprequest-responseURL.html fails on EWS.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:153
> +    m_url.removeFragmentIdentifier();

By doing it at this level, you are effectively saying that response URL can never have a fragment identifier. How did you determine that this is correct? This change goes way beyond the purpose of this patch.

I do not have any easy way to determine whether this change is OK. It looks like it may not break anything at the moment, but only because of bugs in other places, and fragment actually needs to be respected - see bug 24175.

> Source/WebCore/xml/XMLHttpRequest.cpp:340
> +    return m_response.url().string();

The right place to remove fragment is in this accessor.

[Shivakumar J M]

Created attachment 240188 [details]
Patch-Updated-Review7

Updated the patch with tests and added suppourt to handle fragments.

[Shivakumar J M]

(In reply to comment #35)
> Comment on attachment 240115 [details]
> Patch-Updated-Review6
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240115&action=review
> 
> http/tests/xmlhttprequest/xmlhttprequest-responseURL.html fails on EWS.
> 
> > Source/WebCore/platform/network/ResourceResponseBase.cpp:153
> > +    m_url.removeFragmentIdentifier();
> 
> By doing it at this level, you are effectively saying that response URL can
> never have a fragment identifier. How did you determine that this is
> correct? This change goes way beyond the purpose of this patch.
> 
> I do not have any easy way to determine whether this change is OK. It looks
> like it may not break anything at the moment, but only because of bugs in
> other places, and fragment actually needs to be respected - see bug 24175.
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:340
> > +    return m_response.url().string();
> 
> The right place to remove fragment is in this accessor.

Dear Alexey,

Thanks for inputs, actually i had doubt in mind whether to filter fragments in XMLHttpRequest.cpp or in ResourceResponseBase.cpp, since ResourceResponseBase::url() was returning const URL object, i felt instead of filtering the fragments in upper XMLHttpRequest class, lets filter in core side and set the filtered URL, so that it will be done once. 
I think sa per bug 24175 and spec, it should be done in XMLHttpRequest class.

[Alexey Proskuryakov]

Comment on attachment 240188 [details]
Patch-Updated-Review7

View in context: https://bugs.webkit.org/attachment.cgi?id=240188&action=review

Looked over the tests, which look good. Please just get rid of the const_cast.

> Source/WebCore/xml/XMLHttpRequest.cpp:340
> +    const_cast<URL&>(m_response.url()).removeFragmentIdentifier();

This modifies a URL inside m_response, which is definitely a wrong thing to do here (as the constness implies). You need to make a temporary copy, and remove the fragment from it.

[Shivakumar J M]

Created attachment 240249 [details]
Patch-Updated-Review8

Updated the patch with new review comments.

[Alexey Proskuryakov]

Comment on attachment 240249 [details]
Patch-Updated-Review8

View in context: https://bugs.webkit.org/attachment.cgi?id=240249&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:340
> +    URL responseUrl(m_response.url());

WebKit coding style says that this variable should be named "responseURL", not responseUrl.

[Shivakumar J M]

ok will updated the patch with new name. When i ran check-webkit-style, it did not capture these, may be we need o updated script.

[Shivakumar J M]

Created attachment 240252 [details]
Patch-Updated-Review9

updated patch for coding style check.

[Alexey Proskuryakov]

Comment on attachment 240252 [details]
Patch-Updated-Review9

Thank you for tirelessly iterating on this patch!

> When i ran check-webkit-style, it did not capture these, may be we need o updated script.

This script does not check everything, and is not even always correct when it complains about something. Variable naming is something that is particularly hard for a computer to check.

However, this particular case of Url of URL is so common that it may be worth automating the check for it indeed. I filed bug 137964 for that.

[WebKit Commit Bot]

Comment on attachment 240252 [details]
Patch-Updated-Review9

Clearing flags on attachment: 240252

Committed r175053: <http://trac.webkit.org/changeset/175053>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.