Bug 85641 - KURL::isBlankURL would be a nicer idiom than KURL::protocolIs("about")
Summary: KURL::isBlankURL would be a nicer idiom than KURL::protocolIs("about")
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-04 10:40 PDT by WebKit Review Bot
Modified: 2012-05-06 15:56 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.36 KB, patch)
2012-05-04 19:37 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2012-05-04 10:40:43 PDT
KURL::isBlankURL would be a nicer idiom than KURL::protocolIs("about")
Requested by abarth on #webkit.
Comment 1 Mike West 2012-05-04 10:46:06 PDT
Seems like a GoodFirstBug way of dipping my toes into webkit innards. Mind if I grab this?
Comment 2 Adam Barth 2012-05-04 10:48:21 PDT
Go for it.
Comment 3 Mike West 2012-05-04 19:37:12 PDT
Created attachment 140374 [details]
Patch
Comment 4 Mike West 2012-05-04 19:39:11 PDT
Adam, I'd like to add tests for this change but I'm not sure where. Based on some random grepping, I don't see any explicit KURL tests. Can you point me in the right direction?
Comment 5 Adam Barth 2012-05-05 12:45:00 PDT
Comment on attachment 140374 [details]
Patch

I'm surprised there aren't more of these.  For example, I would have expected one in FrameLoader (or at least called from FrameLoader).  As for testing, there's no way to test a change like this in WebKit.  WebKit typically uses LayoutTests to test changes in web-visible behavior.  In this case there isn't any external behavior change that we can test.  Thanks for the patch!
Comment 6 Mike West 2012-05-05 17:23:27 PDT
(In reply to comment #5)
> For example, I would have expected one in FrameLoader (or
> at least called from FrameLoader).

I'm hopping on a plane back to Munich shortly, but I'll take a look at FrameLoader to see if I missed anything sometime tomorrow. Thanks for the pointer.

> As for testing, there's no way to test a change like this in WebKit.  WebKit
> typically uses LayoutTests to test changes in web-visible behavior.  In this
> case there isn't any external behavior change that we can test.  

Ah. Good to know.

> Thanks for the patch!

I appreciate you throwing the patch into the queue, but it looks... unhappy (http://queues.webkit.org/patch/140374). I don't see anything in the patch that would be causing the errors being flagged up. Would you mind taking a look?
Comment 7 Darin Adler 2012-05-05 17:27:42 PDT
I think the explanation for why there aren’t more protocolIs("about") calls is that there is a function called SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument that is used in the loader for that check.

We should probably think about whether any of the call sites that use protocolIs("about") should be using SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument instead, and if so, whether we need a more efficient version of shouldLoadURLSchemeAsEmptyDocument that doesn’t require calling KURL::protocol and allocating a new string each time.

Presumably that’s all beyond the scope of this patch, but worth thinking through.
Comment 8 Adam Barth 2012-05-05 19:09:28 PDT
> I appreciate you throwing the patch into the queue, but it looks... unhappy (http://queues.webkit.org/patch/140374). I don't see anything in the patch that would be causing the errors being flagged up. Would you mind taking a look?

The build is just too red to land patches:

http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29

Failed 30 failures

The patch will land when once the gardener cleans up the tree.
Comment 9 Adam Barth 2012-05-05 19:10:42 PDT
> SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument that is used in the loader for that check.

Hum...  I wonder what other schemes folks are registering.  It might be worth looking a when that was added and why.
Comment 10 Darin Adler 2012-05-05 19:20:37 PDT
The “load as empty document” is exposed only in Chromium WebKit and in WebKit2.

Safari uses the WebKit2 hook for protocols that display special pages in Safari, specifically Top Sites and the bookmarks collection.

Those are pages we don’t want to allow document.write into, so we I think we don’t want, say, the shouldInheritSecurityOriginFromOwner function in Document.cpp to treat those the same way as "about".
Comment 11 Adam Barth 2012-05-05 20:39:43 PDT
Interesting.  That sounds like a similar goal to "display isolated", which is about preventing web sites from invoking these URLs.
Comment 12 WebKit Review Bot 2012-05-06 15:56:46 PDT
Comment on attachment 140374 [details]
Patch

Clearing flags on attachment: 140374

Committed r116248: <http://trac.webkit.org/changeset/116248>
Comment 13 WebKit Review Bot 2012-05-06 15:56:52 PDT
All reviewed patches have been landed.  Closing bug.