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+

Description Pam Greene (IRC:pamg) 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.
Comment 1 Pam Greene (IRC:pamg) 2008-10-06 17:06:19 PDT
Created attachment 24131 [details]
Layout test and expected result
Comment 2 Sam Weinig 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!
Comment 3 Eric Seidel (no email) 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...
Comment 4 Eric Seidel (no email) 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.
Comment 5 Eric Seidel (no email) 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.
Comment 6 Pam Greene (IRC:pamg) 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.
Comment 7 Sam Weinig 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.
Comment 8 Pam Greene (IRC:pamg) 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.
Comment 9 Sam Weinig 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.
Comment 10 Pam Greene (IRC:pamg) 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.)
Comment 11 Sam Weinig 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.
Comment 12 Adam Barth 2008-10-14 01:16:00 PDT
Will land.
Comment 13 Adam Barth 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.
Comment 14 Adam Barth 2008-10-14 12:28:06 PDT
It's going to be a week before I get my Mac Mini back.  Unassigning.
Comment 15 Pam Greene (IRC:pamg) 2008-10-14 12:37:48 PDT
I'll land it, now that I have cred.
Comment 16 Pam Greene (IRC:pamg) 2008-10-14 15:49:57 PDT
Submitted r37595.