Seems like a GoodFirstBug way of dipping my toes into webkit innards. Mind if I grab this?

Go for it.

Created attachment 140374 [details] Patch

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 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!

(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?

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.

> 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.

> 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.

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".

Interesting. That sounds like a similar goal to "display isolated", which is about preventing web sites from invoking these URLs.

Comment on attachment 140374 [details] Patch Clearing flags on attachment: 140374 Committed r116248: <http://trac.webkit.org/changeset/116248>

All reviewed patches have been landed. Closing bug.