[webkit-reviews] review denied: [Bug 41441] Add a target URL parameter to the createView and createWindow method : [Attachment 60230] patch v1 with cleaning style warning and using KURL instead of String to pass target URL

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Johnny Ding <jnd at chromium.org>'s
request for review:
Bug 41441: Add a target URL parameter to the createView and createWindow method
https://bugs.webkit.org/show_bug.cgi?id=41441

Attachment 60230: patch v1 with cleaning style warning and using KURL instead
of String to pass target URL
https://bugs.webkit.org/attachment.cgi?id=60230&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    KURL completedUrl =
> +	   url.isEmpty() ? KURL(ParsedURLString, "") : completeURL(url);

It should be completedURL, not completedUrl. See the coding style document and
note what you yourself did below with targetURL.

I know you just moved the code, but is KURL(ParsedURLString, "") better than
KURL()? Is it different at all?

>      Frame* createWindow(FrameLoader* frameLoaderForFrameLookup, const
FrameLoadRequest&, const WindowFeatures&, bool& created);
> +    // Another version of createWindow which passes the target URL where the
created window will be navigated to.
> +    Frame* createWindow(FrameLoader* frameLoaderForFrameLookup, const
FrameLoadRequest&, const WindowFeatures&, const KURL& targetURL, bool&
created);

Do we really need to have both of these? Can we change the callers to all call
the new one? I don't like having an extra function here.

>	   Page* createWindow(Frame*, const FrameLoadRequest&, const
WindowFeatures&) const;
> +	   Page* createWindow(Frame*, const FrameLoadRequest&, const
WindowFeatures&, const KURL&) const;

Do we really need to have both of these? Can we change the callers to all call
the new one? I don't like having an extra function here.

Otherwise seems fine.