Audit all uses of KURL::prettyURL Requested by abarth on #webkit.
I think that it has its place when displaying a URL to the user, but not necessarily in DOM code.
Looks like prettyURL is mostly used for location.href and as a sandtrap to confuse code into having bugs: ./WebCore/page/Location.cpp: return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/"; ./WebCore/page/Location.cpp: return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/"; ./WebCore/platform/KURL.cpp:String KURL::prettyURL() const ./WebCore/platform/KURL.h: String prettyURL() const; ./WebCore/platform/KURLGoogle.cpp:String KURL::prettyURL() const ./WebCore/platform/network/soup/ResourceHandleSoup.cpp: GOwnPtr<SoupURI> soupURI(soup_uri_new(url.prettyURL().utf8().data())); ./WebCore/workers/WorkerLocation.cpp: return m_url.hasPath() ? m_url.prettyURL() : m_url.prettyURL() + "/"; ./WebCore/workers/WorkerLocation.cpp: return m_url.hasPath() ? m_url.prettyURL() : m_url.prettyURL() + "/"; ./WebKit/efl/ewk/ewk_frame.cpp: hit_test->link.url = eina_stringshare_add(result.absoluteLinkURL().prettyURL().utf8().data()); ./WebKit/efl/ewk/ewk_frame.cpp: WTF::CString uri(sd->frame->document()->url().prettyURL().utf8()); ./WebKit/efl/ewk/ewk_view.cpp: priv->settings.user_stylesheet = eina_stringshare_add(url.prettyURL().utf8().data()); ./WebKit/efl/ewk/ewk_view.cpp: url.prettyURL().utf8().data(), referrer.utf8().data()); ./WebKit/efl/ewk/ewk_view.cpp: url.prettyURL().utf8().data(), mimeType.utf8().data()); ./WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp: CString urlStr = url.prettyURL().utf8(); ./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp: CString url = coreRequest.url().prettyURL().utf8(); ./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp: CString url = coreRequest.url().prettyURL().utf8(); ./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp: char* url = strdup(resourceRequest.url().prettyURL().utf8().data()); ./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp: CString url = request.url().prettyURL().utf8(); ./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp: ResourceError error("Error", -999, request.url().prettyURL(), ./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp: return ResourceError("Error", WebKitErrorCannotUseRestrictedPort, request.url().prettyURL(), ./WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp: CString urlString = url.prettyURL().utf8(); ./WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp: priv->uri = g_strdup(core(m_frame)->document()->url().prettyURL().utf8().data()); ./WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp: priv->uri = g_strdup(core(m_frame)->loader()->activeDocumentLoader()->url().prettyURL().utf8().data()); ./WebKit/gtk/webkit/webkitwebview.cpp: String iconURL = iconDatabase().synchronousIconURLForPageURL(core(webView)->mainFrame()->document()->url().prettyURL()); ./WebKit/qt/WebCoreSupport/ChromeClientQt.cpp: emit m_webPage->linkHovered(lastHoverURL.prettyURL(), ./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp: history->addHistoryEntry(loader->urlForHistory().prettyURL()); ./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp: ResourceError error = ResourceError("QtNetwork", QNetworkReply::OperationCanceledError, request.url().prettyURL(), ./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp: return ResourceError("WebKitErrorDomain", WebKitErrorCannotUseRestrictedPort, request.url().prettyURL(), ./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp: // qDebug()<<" ++++++++++++++++ url is "<<url.prettyURL()<<", mime = "<<mimeTypeIn; ./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp: // qDebug()<<"------ Creating plugin in FrameLoaderClientQt::createPlugin for "<<url.prettyURL() << mimeType; ./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp: // qDebug()<<"------\t url = "<<url.prettyURL(); ./WebKit2/WebProcess/WebCoreSupport/qt/WebErrorsQt.cpp: ResourceError error = ResourceError("QtNetwork", QNetworkReply::OperationCanceledError, request.url().prettyURL(), ./WebKit2/WebProcess/WebCoreSupport/qt/WebErrorsQt.cpp: return ResourceError("WebKit", WebKitErrorCannotUseRestrictedPort, request.url().prettyURL(), ./WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp: return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/";
AFACT, all these callers are wrong.
Created attachment 95116 [details] Patch
Comment on attachment 95116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95116&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-1204 > - const KURL& url = frame->document()->url(); > - return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/"; What makes you sure we don’t need this “always make sure we have at least a /” logic?
Comment on attachment 95116 [details] Patch Attachment 95116 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8734961
Comment on attachment 95116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95116&action=review >> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-1204 >> - return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/"; > > What makes you sure we don’t need this “always make sure we have at least a /” logic? The trailing / isn't required by the URI specs, nor by any URL parser I'm aware of. (I also couldn't seem to actually create a document where document()->url()->string() was lacking the trailing slash.)
Created attachment 95117 [details] Patch
e.g., data:text/html,%3Ca%20href=%22http://example.com%22%3E%3Cscript%3Ealert(document.body.firstElementChild.href)%3C/script%3E has the trailing slash, as does <iframe src="http://example.com"> and reading document.documentURI inside the frame (which just grabs url().string()). Safari Version 5.0.5 (6533.21.1)
Which of these call sites (if any) is responsible for displaying a pretty URL in Safari address bar? I am unsure what is best for ResourceError URLs, which are strictly for human consumption.
> Which of these call sites (if any) is responsible for displaying a pretty URL in Safari address bar? Presumable none of them because I didn't change anything that effects WebKit1 on Mac! > I am unsure what is best for ResourceError URLs, which are strictly for human consumption. My thinking is that it's better to have these URLs work like every other URL in the web platform, but I agree that it's a matter of aesthetics.
Comment on attachment 95117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95117&action=review Yes - Safari probably relies upon code in NSURLExtras, which is independent of KURL. > Source/WebCore/page/Location.cpp:69 > + return url.hasPath() ? url.deprecatedString() : url.deprecatedString() + "/"; Why not add a FIXME link to <https://bugs.webkit.org/show_bug.cgi?id=30225> in case you don't resolve this immediately?
(In reply to comment #12) > (From update of attachment 95117 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95117&action=review > > > Source/WebCore/page/Location.cpp:69 > > + return url.hasPath() ? url.deprecatedString() : url.deprecatedString() + "/"; > > Why not add a FIXME link to <https://bugs.webkit.org/show_bug.cgi?id=30225> in case you don't resolve this immediately? Thanks. Will do.
Created attachment 95225 [details] Patch for landing
Committed r87623: <http://trac.webkit.org/changeset/87623>