[webkit-reviews] review denied: [Bug 24851] Failures in http/tests/xhlhttprequest/web-apps on Windows : [Attachment 29037] ResourceResponse, updated skip list, and new platform/win results

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Mar 28 17:28:45 PDT 2009


Darin Adler <darin at apple.com> has denied Brian Weinstein
<bweinstein at gmail.com>'s request for review:
Bug 24851: Failures in http/tests/xhlhttprequest/web-apps on Windows
https://bugs.webkit.org/show_bug.cgi?id=24851

Attachment 29037: ResourceResponse, updated skip list, and new platform/win
results
https://bugs.webkit.org/attachment.cgi?id=29037&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list