Bug 21597

Summary: Set popup's location to about:blank while it's loading
Product: WebKit Reporter: Pam Greene (IRC:pamg) <pam>
Component: DOMAssignee: Mike Belshe <mbelshe>
Severity: Normal CC: ap, darin, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Location.cpp patch with new expected results.
darin: review-
Patch w/ Darin's comments eric: review+

Description Pam Greene (IRC:pamg) 2008-10-14 13:45:42 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 fast/dom/Window/window-open-pending-url.html 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

WebKit should also report 'location: about:blank', for consistency and so Selenium works.  See bug 21419 for earlier discussion.
Comment 1 Mike Belshe 2009-03-02 16:31:56 PST
Created attachment 28203 [details]
Location.cpp patch with new expected results.
Comment 2 Alexey Proskuryakov 2009-03-04 01:43:24 PST
Where does this incorrect data in m_frame->loader()->url() come from? Could it be more appropriate to fix it, instead of tweaking results in Location?
Comment 3 Mike Belshe 2009-03-04 10:24:44 PST
I don't think there was any incorrect data; the URL was just not yet set, and when you tostring() that, you get "/".  So I think the fix is correct - if there is no URL (e.g. url.IsValid() == false), then use the about:blank for conformance with other browsers.

We could change the way frame works to initialize to about:blank from the start;  but this seems more like an API compatibility issue with Location rather than a problem with the way the frame initializes its url.
Comment 4 Alexey Proskuryakov 2009-03-06 09:30:23 PST
A test added in <http://trac.webkit.org/projects/webkit/changeset/41484> will be affected by this patch.
Comment 5 Darin Fisher (:fishd, Google) 2009-03-06 12:50:02 PST
Comment on attachment 28203 [details]
Location.cpp patch with new expected results.

>Index: WebCore/page/Location.cpp
>+    const KURL& url = m_frame->loader()->url();
>+    if (!url.isValid())
>+        return blankURL();  // Use "about:blank" while the page is still loading (before we have a frame).

isn't it possible for the URL to be invalid due to bad input from
a web page or from the user?  in those cases, wouldn't we still want
to report that possibly invalid URL as the location?

e.g.,  12345://foo.com/

hmm... though in my testing, it looks like invalid URLs are always
treated as relative URLs by webkit.
Comment 6 Darin Adler 2009-03-06 14:44:30 PST
Comment on attachment 28203 [details]
Location.cpp patch with new expected results.

This looks like a good approach. It would also be possible to change the FrameLoader itself to use the blankURL instead of an empty URL -- riskier and harder to get it right.

You'll also have to patch the test and results from the test I just added, by modifying fast/dom/resources/location-new-window-no-crash.js and updating fast/dom/location-new-window-no-crash-expected.txt with new results.

The ChangeLog mentions a function WebCore::urlWhileLoading -- this version of the patch doesn't have that so it shouldn't be in the change log.

Those change logs are no good -- they don't say anything about what you're changing and why, nor do they have the URL of this bug! The change log needs to mention the rationale about why these values need to be returned. I'd like some more data about the behavior of IE here and how you determined this behavior was required.
Comment 7 Mike Belshe 2009-03-12 17:07:17 PDT
Thanks for your comments Darin.

Regarding why I think we should do this change:  there is a selenium test framework which had a dependency on this from Firefox, and since the browsers are inconsistent, we thought this was a better approach.  I suppose we could debate more whether about:blank is the right interim value, but it does seem better than '/'.

Here are the IE8 results with your new location-new-window-no-crash which differ:
location.toString() was [object Object].
location.href was .        (empty string)
location.protocol was .
location.pathname was .

Comment 8 Mike Belshe 2009-03-12 17:08:18 PDT
Created attachment 28569 [details]
Patch w/ Darin's comments
Comment 9 Jon@Chromium 2009-03-25 13:08:17 PDT
Darin, could you please look over Mike's revision?  We are trying to clean up some layout test failures and this patch is needed.
Comment 10 Eric Seidel (no email) 2009-03-26 11:25:22 PDT
Darin was not cc'd on the bug, I've done so now.  The patch also looks fine to me, and seems to address Darin and Alexey's previous comments.
Comment 11 Eric Seidel (no email) 2009-03-26 11:27:42 PDT
Comment on attachment 28569 [details]
Patch w/ Darin's comments

I think that this change looks fine, but we should make an effort to better document the behavior of the other browsers in the test or code change for when the next person looks at this. ;)

Specifically, there are comments about what FF does location-new-window-no-crash.js we might as well add the information mike learned in his testing of IE8 to the same comment lines when landing.

I've CC'd Darin (and Alexey was already cc'd) so they have the opportunity to point out any subtlety which they had in mind, but didn't mention in the bug.
Comment 12 Eric Seidel (no email) 2009-04-29 14:31:47 PDT
I fixed up the ChangeLogs a little and landed:

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/dom/Window/window-open-pending-url-expected.txt
	M	LayoutTests/fast/dom/location-new-window-no-crash-expected.txt
	M	LayoutTests/fast/dom/resources/location-new-window-no-crash.js
	M	WebCore/ChangeLog
	M	WebCore/page/Location.cpp
Committed r43014