[Webkit-unassigned] [Bug 41441] Add a target URL parameter to the createView and createWindow method

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41441





--- Comment #4 from Johnny Ding <jnd at chromium.org>  2010-06-30 20:55:33 PST ---
Adam, Thanks for review.

For the comments of using String not URL, that is my fault, I thought using String is easy way since we have different URL class implementation like KURL, WebURL, GURL, etc...  I will change them to corresponding URL class.


(In reply to comment #2)
> (From update of attachment 60188 [details])
> WebCore/bindings/generic/BindingDOMWindow.h:99
>  +      Frame* newFrame = callingFrame->loader()->createWindow(openerFrame->loader(), frameRequest, windowFeatures, completeUrl.prettyURL(), created);
> What is a prettyURL?
> 
> WebCore/loader/FrameLoader.cpp:261
>  +  Frame* FrameLoader::createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest& request, const WindowFeatures& features, const String& targetURL, bool& created)
> Why is this a String and not a KURL?
> 
> WebCore/loader/FrameLoader.cpp:328
>  +      return createWindow(frameLoaderForFrameLookup, request, features, "", created);
> Why isn't the URL plumbed here?
> 
> WebCore/page/Chrome.cpp:166
>  +  Page* Chrome::createWindow(Frame* frame, const FrameLoadRequest& request, const WindowFeatures& features, const String& targetURL) const
> Isn't there a URL in FrameLoadRequest?  Is that different?
> 
> WebKit/chromium/public/WebViewClient.h:86
>  +          // The default implementation is to call the old version of createView.
> Are we going to remove this after we do the roll?

Yes, definitely. After finishing the code on chromium side, we will remove the old version.

> 
> WebKit/chromium/src/ChromeClientImpl.cpp:236
>  +          m_webView->client()->createView(WebFrameImpl::fromFrame(frame), features, r.frameName(), KURL(ParsedURLString, targetURL)));
> Sad that we have to parse the URL again here.
> 
> WebKit/chromium/src/ChromeClientImpl.cpp:251
>  +      Frame* frame, const FrameLoadRequest& r, const WindowFeatures& features)
> One line please.  |r| is not a very descriptive variable name.
> 
> WebKit/chromium/src/ChromeClientImpl.cpp:253
>  +      return createWindow(frame, r, features, "");
> More empty string sadness.

(

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.