Bug 21419

Summary: Add a layout test to verify popup properties while they're opening
Product: WebKit Reporter: Pam Greene (IRC:pamg) <pam>
Component: DOMAssignee: Pam Greene (IRC:pamg) <pam>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Layout test and expected result eric: review+

Pam Greene (IRC:pamg)
Reported 2008-10-06 17:01:14 PDT
Different browsers return different things for the properties of a popup window while the window is opening. In particular, the behavior of window.location, window.href, and window.pathname vary. The Selenium web app test system -- http://selenium.openqa.org -- depends on this behavior. When running the attached test, popup.location reports as follows: Safari ToT: location: / Firefox: location: about:blank Chrome: location: about:blank IE7: location: file:///C:/<path to>/fast/dom/Window/resources/destination.html My goal is to contribute all of Chromium's new layout tests promptly for everyone's benefit, so even if it's decided that WebKit will change its behavior, I'd still vote for submitting the attached files as they are for now, filing a new bug, and updating the result when it changes.
Attachments
Layout test and expected result (1.62 KB, patch)
2008-10-06 17:06 PDT, Pam Greene (IRC:pamg)
eric: review+
Pam Greene (IRC:pamg)
Comment 1 2008-10-06 17:06:19 PDT
Created attachment 24131 [details] Layout test and expected result
Sam Weinig
Comment 2 2008-10-07 00:20:31 PDT
Comment on attachment 24131 [details] Layout test and expected result r=me. We are trying to move to more tests where the expected results are clear, so in the future, that would be a good thing to do. But this looks good. Thanks!
Eric Seidel (no email)
Comment 3 2008-10-07 00:53:00 PDT
What Sam meant, was that we're trying to move towards more tests where the test itself contains the expected results. You can see this in all the "shouldBe" based tests which use the scripts in fast/js/resources and the TEMPLATE.html system with make-js-test-wrappers generated test files. the fast/js tests aren't very well documented, but they're pretty simple to write. Any time you find a resources/ directory containing a TEMPLATE.html file, all of the .js files in that directory are examples of the fast/js testing system, which uses shouldBe. This test could then be re-written as something like: description("Test to make sure that popup's have no properties while opening to match other browsers https://bugs.webkit.org/show_bug.cgi?id=21419"); // dumpAsText is handled for you in fast/js style tests if (window.layoutTestController) { layoutTestController.setCanOpenWindows(); layoutTestController.waitUntilDone(); } var popup = window.open('resources/destination.html'); shouldBe("popup.location", "/"); shouldBe("popup.location['href']", "/"); shouldBe("popup.location['pathname']", "/"); shouldBe("popup.location['hash']", ""); shouldBe("popup.location['port']", ""); shouldBe("popup.location['protocol']", ""); shouldBe("popup.location['search']", ""); shouldBe("popup.location['pathname']", ""); Wait... actually this test never calls "notifyDone()" so it will just hit the timeout every time! That's bad. :( I have to change review to r-, unless I missed something...
Eric Seidel (no email)
Comment 4 2008-10-07 00:54:07 PDT
Comment on attachment 24131 [details] Layout test and expected result Yeah, this needs to call notifyDone() or not call waitUntilDone(), otherwise this test just takes longer than necessary to run. Consider looking at the new fancy fast/js testing system if you have interest.
Eric Seidel (no email)
Comment 5 2008-10-07 00:56:34 PDT
Comment on attachment 24131 [details] Layout test and expected result Sam tells me notifyDone() is part of destination.html... so this is OK.
Pam Greene (IRC:pamg)
Comment 6 2008-10-07 11:27:10 PDT
(In reply to comment #3) > What Sam meant, was that we're trying to move towards more tests where the test > itself contains the expected results. I'd agree in general, but that does cause problems for tests like this one where different embedders may legitimately want different results.
Sam Weinig
Comment 7 2008-10-07 12:44:49 PDT
(In reply to comment #6) > I'd agree in general, but that does cause problems for tests like this one > where different embedders may legitimately want different results. I can see no reason why different ports would want different results here.
Pam Greene (IRC:pamg)
Comment 8 2008-10-07 12:49:15 PDT
Well, I know Chrome reports "location: about:blank" so Selenium works. I don't know if there's a specific reason that Safari reports "location: /" instead, or if it's just that nobody had a reason to care before.
Sam Weinig
Comment 9 2008-10-07 12:54:16 PDT
(In reply to comment #8) > Well, I know Chrome reports "location: about:blank" so Selenium works. I don't > know if there's a specific reason that Safari reports "location: /" instead, or > if it's just that nobody had a reason to care before. > Presumably it is a bug.
Pam Greene (IRC:pamg)
Comment 10 2008-10-07 13:05:21 PDT
Is it acceptable to submit this test and expected result as they are, and file a separate bug to fix it? Or should I morph this report and keep the test pending until the behavior is fixed? (I'd selfishly prefer the former, but I recognize that the latter would be better practice.)
Sam Weinig
Comment 11 2008-10-07 13:23:57 PDT
(In reply to comment #10) > Is it acceptable to submit this test and expected result as they are, and file > a separate bug to fix it? Or should I morph this report and keep the test > pending until the behavior is fixed? (I'd selfishly prefer the former, but I > recognize that the latter would be better practice.) Oh, no, please land as is and file a follow up bug. The main point of tests is to test for changes in behavior, so having them in the trees is the most important thing. My point was only that any new tests would be preferred in the other format, to solve a dual purpose.
Adam Barth
Comment 12 2008-10-14 01:16:00 PDT
Will land.
Adam Barth
Comment 13 2008-10-14 03:01:20 PDT
My Mac Mini died in the middle of testing this patch. I'm going to take it into the store tomorrow.
Adam Barth
Comment 14 2008-10-14 12:28:06 PDT
It's going to be a week before I get my Mac Mini back. Unassigning.
Pam Greene (IRC:pamg)
Comment 15 2008-10-14 12:37:48 PDT
I'll land it, now that I have cred.
Pam Greene (IRC:pamg)
Comment 16 2008-10-14 15:49:57 PDT
Submitted r37595.
Note You need to log in before you can comment on or make changes to this bug.