Bug 41724

Summary: Improve XHR access control tests
Product: WebKit Reporter: Justin Schuh <jschuh>
Component: New BugsAssignee: Justin Schuh <jschuh>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, commit-queue, jschuh, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch none

Description Justin Schuh 2010-07-06 17:17:31 PDT
Improve XHR access control tests
Comment 1 Justin Schuh 2010-07-06 17:42:27 PDT
Created attachment 60664 [details]
Patch
Comment 2 Justin Schuh 2010-07-06 20:17:03 PDT
Improving and expanding the layout tests added with the fix for bug 36483.
Comment 3 Adam Barth 2010-07-07 03:04:03 PDT
Comment on attachment 60664 [details]
Patch

LayoutTests/http/tests/xmlhttprequest/access-control-preflight-async-not-supported.html:10
 +  if (window.layoutTestController) {
No braces needed.

LayoutTests/http/tests/xmlhttprequest/access-control-preflight-async-not-supported.html:19
 +          xhr.send("");
Usually we pass null for GET (no body) instead of a body of length 0.

LayoutTests/http/tests/xmlhttprequest/access-control-preflight-async-not-supported.html:56
 +          // Eat the exception.
Why don't we log it?
Comment 4 Adam Barth 2010-07-07 03:04:41 PDT
Those comments were just drive-bys.  ap should probably review your patch since he requested it.
Comment 5 Justin Schuh 2010-07-07 07:36:46 PDT
(In reply to comment #3)
> (From update of attachment 60664 [details])
> LayoutTests/http/tests/xmlhttprequest/access-control-preflight-async-not-supported.html:56
>  +          // Eat the exception.
> Why don't we log it?

The exception here means the test probably succeeded (because we want the request to be denied). But I'm doing a more thorough result check in the onreadystatechange handler, which also validates that the server side matches what it should be. 

And I'll fix the style issues and resubmit.
Comment 6 Alexey Proskuryakov 2010-07-07 11:46:05 PDT
Comment on attachment 60664 [details]
Patch

Clearing review flag per the above comment.

Looks good to me. Do these tests pass in Firefox?

This change makes it obvious that I called "header field" a "header" at least once. I'll need to fix that eventually.
Comment 7 David Levin 2010-07-07 12:24:38 PDT
(In reply to comment #2)
> Improving and expanding the layout tests added with the fix for bug 36483.

I don't think that is the right bug number: "webkitpy: Move the commands folder into webkitpy/patch"
Comment 8 Justin Schuh 2010-07-07 13:24:30 PDT
(In reply to comment #7)
> (In reply to comment #2)
> > Improving and expanding the layout tests added with the fix for bug 36483.
> 
> I don't think that is the right bug number: "webkitpy: Move the commands folder into webkitpy/patch"

Dyslexia strikes again. Sorry, that should be bug 36843. I've finally gotten through my holiday weekend backlog so I should be posting a cleaned up patch shortly.
Comment 9 Justin Schuh 2010-07-07 17:22:52 PDT
Created attachment 60811 [details]
Patch

Cleaned up style issues and tweaked a few things so the tests will run in Firefox.
Comment 10 WebKit Commit Bot 2010-07-08 05:00:43 PDT
Comment on attachment 60811 [details]
Patch

Clearing flags on attachment: 60811

Committed r62779: <http://trac.webkit.org/changeset/62779>
Comment 11 WebKit Commit Bot 2010-07-08 05:00:49 PDT
All reviewed patches have been landed.  Closing bug.