Bug 24851 - Failures in http/tests/xhlhttprequest/web-apps on Windows
Summary: Failures in http/tests/xhlhttprequest/web-apps on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P2 Normal
Assignee: Nobody
URL: http://build.webkit.org/results/trunk...
Keywords: LayoutTestFailure, PlatformOnly
Depends on:
Blocks:
 
Reported: 2009-03-26 13:36 PDT by Brian Weinstein
Modified: 2009-04-07 15:27 PDT (History)
3 users (show)

See Also:


Attachments
Update of skip-list and web-apps windows results (1.45 KB, patch)
2009-03-26 13:47 PDT, Brian Weinstein
aroben: review-
Details | Formatted Diff | Diff
Update to web-apps test result and skip list (3.79 KB, patch)
2009-03-26 16:08 PDT, Brian Weinstein
ap: review-
Details | Formatted Diff | Diff
Fix in ResourceResponse to remove status code from status text (5.06 KB, patch)
2009-03-27 15:40 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
ResourceResponse fix and updated skip list (5.09 KB, patch)
2009-03-27 15:50 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
ResourceResponse, updated skip list, and new platform/win results (5.28 KB, patch)
2009-03-28 10:53 PDT, Brian Weinstein
darin: review-
Details | Formatted Diff | Diff
Fix of status text in Windows and updated test results (5.49 KB, patch)
2009-03-30 19:17 PDT, Brian Weinstein
ap: review-
Details | Formatted Diff | Diff
Fix of status text in Windows and updated test results (5.29 KB, patch)
2009-03-31 10:33 PDT, Brian Weinstein
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2009-03-26 13:36:56 PDT
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.
Comment 1 Brian Weinstein 2009-03-26 13:47:01 PDT
Created attachment 28980 [details]
Update of skip-list and web-apps windows results
Comment 2 Adam Roben (:aroben) 2009-03-26 13:50:08 PDT
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/
Comment 3 Brian Weinstein 2009-03-26 16:08:12 PDT
Created attachment 28992 [details]
Update to web-apps test result and skip list
Comment 4 Alexey Proskuryakov 2009-03-26 23:30:29 PDT
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.
Comment 5 Brian Weinstein 2009-03-27 15:40:09 PDT
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.
Comment 6 Brian Weinstein 2009-03-27 15:41:20 PDT
And of course I used tabs :-(.
Comment 7 Brian Weinstein 2009-03-27 15:50:25 PDT
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)
Comment 8 Alexey Proskuryakov 2009-03-28 02:26:05 PDT
+        //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?
Comment 9 Brian Weinstein 2009-03-28 10:24:51 PDT
(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.
Comment 10 Brian Weinstein 2009-03-28 10:53:21 PDT
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)
Comment 11 Darin Adler 2009-03-28 17:28:45 PDT
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.
Comment 12 Brian Weinstein 2009-03-29 00:53:08 PDT
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)
Comment 13 Brian Weinstein 2009-03-30 19:17:45 PDT
Created attachment 29105 [details]
Fix of status text in Windows and updated test results

Implemented Darin's suggestions.
Comment 14 Alexey Proskuryakov 2009-03-31 00:06:27 PDT
Comment on attachment 29105 [details]
Fix of status text in Windows and updated test results

The comparisons to -1 still look unnecessary to me.
Comment 15 Brian Weinstein 2009-03-31 08:54:11 PDT
(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".
Comment 16 Alexey Proskuryakov 2009-03-31 09:18:38 PDT
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.
Comment 17 Brian Weinstein 2009-03-31 10:33:04 PDT
Created attachment 29123 [details]
Fix of status text in Windows and updated test results

Removed -1 checks.
Comment 18 Darin Adler 2009-03-31 10:35:42 PDT
Comment on attachment 29123 [details]
Fix of status text in Windows and updated test results

r=me
Comment 19 Adam Roben (:aroben) 2009-04-07 15:27:53 PDT
Landed in r42293