Bug 21597 – Set popup's location to about:blank while it's loading

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Pam Greene (IRC:pamg)]

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.

[Mike Belshe]

Created attachment 28203 [details]
Location.cpp patch with new expected results.

[Alexey Proskuryakov]

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?

[Mike Belshe]

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.

[Alexey Proskuryakov]

A test added in <http://trac.webkit.org/projects/webkit/changeset/41484> will be affected by this patch.

[Darin Fisher (:fishd, Google)]

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.

[Darin Adler]

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.

[Mike Belshe]

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 .

[Mike Belshe]

Created attachment 28569 [details]
Patch w/ Darin's comments

[Jon@Chromium]

Darin, could you please look over Mike's revision?  We are trying to clean up some layout test failures and this patch is needed.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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