Summary: | Returns Blank Page for all "about" protocols | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Genevieve Mak <gen> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | manyoso, staikos, zimmermann | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | Linux | ||||||||
Attachments: |
|
Description
Genevieve Mak
2009-01-23 12:47:02 PST
Created attachment 26980 [details]
Patch and logfile
Proposed patch to return a blank page for only "about:blank" urls and empty urls instead of all "about:*" urls.
Comment on attachment 26980 [details]
Patch and logfile
Looks good.
1) The check for "about" should be using the protocolIs function, which is more efficient than calling .protocol() and then calling equalIgnoringCase. You're not changing that aspect of the function, but I'm surprised to see any code still doing it this old-fashioned way.
2) To limit this to "about:blank", then I think the normal idiom would be "equalIgnoringRef(url, blankURL())". This would be more efficient than the code you wrote. Your code would also allow strings like "about: blank " and "about:blank?notsoblank"; maybe that additional behavior is good, maybe bad. You
3) SecurityOrigin.cpp assumes that the "about" protocol has certain security properties. If you implement something that makes some about URLs return something other than an empty page, you could create security problems. I don't know anything more than that.
4) Safari and other Mac OS X applications may well rely on all "about:" URLs giving you a blank document, not just "about:blank", so I think this change may not be safe to make cross-platform.
So I'm going to say review- until you either put this inside some kind of #if or provide some evidence that clients don't rely on this behavior on Mac OS X.
Created attachment 27051 [details]
Updated Patch
Thanks, fixed issues #1 and #2. For issue #4 I've also put an "#if PLATFORM() around the fix in order to preserve the status quo. For #3 wouldn't it be up to whomever implements the browser to handle? I could see why you might want default blank pages for the "about" protocol but I'm not sure MainResourceLoader is the right place. When using Qt, MainResourceLoader gets a hold of the request before QNetworkReplyHandler even gets a chance to let the browser look at it.
Comment on attachment 27051 [details]
Updated Patch
Seems to have addressed the issues. Looks okay to me.
|