Bug 24851 – Failures in http/tests/xhlhttprequest/web-apps on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Brian Weinstein]

As of build 3351 - there were failures in two tests in this directory - 012.html, and 013.html. These tests behave as they are supposed to, but read the results from platform/mac/http/tests/xhlhttprequest/web-apps.

These tests are currently on the skip list.

To fix this behavior (and move two bugs off of the skip list), I have put the correct results (with the updated status text) inside of platform/win.

[Brian Weinstein]

Created attachment 28980 [details]
Update of skip-list and web-apps windows results

[Adam Roben (:aroben)]

Comment on attachment 28980 [details]
Update of skip-list and web-apps windows results

You need a ChangeLog (as you know).

You should use "svn cp" to create the new files in platform/win. The following commands should do it:

svn cp LayoutTests/http/tests/xmlhttprequest/web-apps/012-expected.txt LayoutTests/platform/win/http/tests/xmlhttprequest/web-apps/
svn cp LayoutTests/http/tests/xmlhttprequest/web-apps/013-expected.txt LayoutTests/platform/win/http/tests/xmlhttprequest/web-apps/

[Brian Weinstein]

Created attachment 28992 [details]
Update to web-apps test result and skip list

[Alexey Proskuryakov]

Comment on attachment 28992 [details]
Update to web-apps test result and skip list

The problem here is that XMLHttpRequest.statusText includes the status code (e.g. it's "400 Good work" instead of "Good Work"). Looking at ResourceResponse::platformLazyInit(), it has an obvious bug, removing only "HTTP/1.1" from a response such as "HTTP/1.1 400 Good Work":

        RetainPtr<CFStringRef> statusLine(AdoptCF, CFHTTPMessageCopyResponseStatusLine(httpResponse));
        String statusText(statusLine.get());
        int spacePos = statusText.find(" ");
        if (spacePos != -1)
            statusText = statusText.substring(spacePos + 1);
        m_httpStatusText = statusText;

Looks like this is easier to fix than to land failing results.

[Brian Weinstein]

Created attachment 29028 [details]
Fix in ResourceResponse to remove status code from status text

This is a code fix instead of just fixing the expected output of the tests, able to move 6 tests off of the skipped list.

[Brian Weinstein]

And of course I used tabs :-(.

[Brian Weinstein]

Created attachment 29029 [details]
ResourceResponse fix and updated skip list

Fixed formatting issues from previous patch (sorry for the spam people who are CC'd)

[Alexey Proskuryakov]

+        //Get rid of the status number from statusText

Please add a space after "//", and a period after the sentence. Also, it's "status code", not number.

+        spacePos = statusText.find(" ");
+        if (spacePos != -1)
+            statusText = statusText.substring(spacePos + 1);

You were just copying existing code, but I don't think that the check is useful - even if spacePos equals -1, it is safe to use substring, because its argument will be 0.

+        Tests in http/tests/xmlhttprequest/web-apps were taking results from platform/mac instead of LayoutTests, so added the values from LayoutTests into platform/win, and removed a few tests from the skip list that are fixed by the fix to platform/network/cf/ResourceResponseCFNet.cpp		

Please break this line at some reasonable width! Also, there are tabs at the end of it.

I'm confused by the need to make copies of expected results in platform/win. Do we really take expected results from platform/mac when testing on Windows? Is this a feature, or a bug?

[Brian Weinstein]

(In reply to comment #8)
> +        //Get rid of the status number from statusText
> 
> Please add a space after "//", and a period after the sentence. Also, it's
> "status code", not number.
> 
> +        spacePos = statusText.find(" ");
> +        if (spacePos != -1)
> +            statusText = statusText.substring(spacePos + 1);
> 
> You were just copying existing code, but I don't think that the check is useful
> - even if spacePos equals -1, it is safe to use substring, because its argument
> will be 0.
> 
> +        Tests in http/tests/xmlhttprequest/web-apps were taking results from
> platform/mac instead of LayoutTests, so added the values from LayoutTests into
> platform/win, and removed a few tests from the skip list that are fixed by the
> fix to platform/network/cf/ResourceResponseCFNet.cpp                
> 
> Please break this line at some reasonable width! Also, there are tabs at the
> end of it.
> 
> I'm confused by the need to make copies of expected results in platform/win. Do
> we really take expected results from platform/mac when testing on Windows? Is
> this a feature, or a bug?
> 

Does that mean that check is not useful in either instance of the code? Should I just get rid of it in both places? Because like you said, even if nothing is found, the start of the substring will just be 0.

Also, it is trying to match platform/mac because, as I was told, they are trying to get the results to match Leopard/Snow Leopard as closely as possible, so adding the files to platform/win is needed for the patch to take effect.

[Brian Weinstein]

Created attachment 29037 [details]
ResourceResponse, updated skip list, and new platform/win results

Made formatting fixes, removed both checks for spacePos != -1 (if one isn't necessary, I don't think either are)

[Darin Adler]

Comment on attachment 29037 [details]
ResourceResponse, updated skip list, and new platform/win results

Great that you're working on this!

>          int spacePos = statusText.find(" ");

It's crazy that this is using the string version of find instead of the faster character version. It should be changed to just do find(' ') instead of find(" ").

> -        if (spacePos != -1)
> -            statusText = statusText.substring(spacePos + 1);
> +        statusText = statusText.substring(spacePos + 1);
> +	
> +        // Remove the status code from statusText.
> +        spacePos = statusText.find(" ");
> +        statusText = statusText.substring(spacePos + 1);

If we want to skip the second space, then we can call find twice, passing the start position from the first find in to the second. We don't have to call substring twice. If we want to find the last space, then reverseFind is the function to use.

> +        Tests in http/tests/xmlhttprequest/web-apps were taking results from 
> +        platform/mac instead of LayoutTests, so added the values from LayoutTests 
> +        into platform/win, and removed a few tests from the skip list that are fixed 
> +        by the fix to platform/network/cf/ResourceResponseCFNet.cpp
> +
> +        * platform/win/Skipped:
> +        * platform/win/http: Added.
> +        * platform/win/http/tests: Added.
> +        * platform/win/http/tests/xmlhttprequest: Added.
> +        * platform/win/http/tests/xmlhttprequest/web-apps: Added.
> +        * platform/win/http/tests/xmlhttprequest/web-apps/012-expected.txt: Copied from LayoutTests/http/tests/xmlhttprequest/web-apps/012-expected.txt.
> +        * platform/win/http/tests/xmlhttprequest/web-apps/013-expected.txt: Copied from LayoutTests/http/tests/xmlhttprequest/web-apps/013-expected.txt.

I'm not entirely sure this is right. Don't we want to share results with Mac for these just like most of the other tests? What's different here? Is this something we can fix in the script instead?

review- due to the find comments above.

[Brian Weinstein]

The reason we don't want to share this with the mac is because the mac always returns "OK" instead of the status text.

http://trac.webkit.org/changeset/41665

Windows and other platforms can return the actual status text, which means that we should use it when possible, and in this case it means LayoutTests/http is the better bet than LayoutTests/platform/mac/http

And thanks for the feedback Darin, I will have an updated patch out to you hopefully early tomorrow (I'm up too late to be coding anyways)

[Brian Weinstein]

Created attachment 29105 [details]
Fix of status text in Windows and updated test results

Implemented Darin's suggestions.

[Alexey Proskuryakov]

Comment on attachment 29105 [details]
Fix of status text in Windows and updated test results

The comparisons to -1 still look unnecessary to me.

[Brian Weinstein]

(In reply to comment #14)
> (From update of attachment 29105 [details] [review])
> The comparisons to -1 still look unnecessary to me.
> 

I feel like they are necessary in this case, because in the first comparison, if there is no space in the original response text, a comparison to -1 there is much cheaper than doing another whole find call. For the second comparison, I was considering the pathological case where there is one space in the text (like "HTTP/1.1 OK"), where the first find would return 8, but then calling find(' ', 9), would return -1, and if we used that -1 result, the status text after removing the status code would be "HTTP/1.1 OK" instead of what I felt was the expected "OK".

[Alexey Proskuryakov]

Comment on attachment 29105 [details]
Fix of status text in Windows and updated test results

As discussed on IRC, there is no need to optimize the failure case.

Brian said he was going to update the patch, so marking as r- to get it out of review queue.

[Brian Weinstein]

Created attachment 29123 [details]
Fix of status text in Windows and updated test results

Removed -1 checks.

[Darin Adler]

Comment on attachment 29123 [details]
Fix of status text in Windows and updated test results

r=me

[Adam Roben (:aroben)]

Landed in r42293