[webkit-reviews] review granted: [Bug 41441] createWindow method should only do window-creating without URL navigator : [Attachment 74080] Patch V3 update 1 fixed Chromium break

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Xianzhu Wang
<phnixwxz at gmail.com>'s request for review:
Bug 41441: createWindow method should only do window-creating without URL
navigator
https://bugs.webkit.org/show_bug.cgi?id=41441

Attachment 74080: Patch V3 update 1 fixed Chromium break
https://bugs.webkit.org/attachment.cgi?id=74080&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74080&action=review

Just nits, but otherwise this LGTM.

> WebCore/bindings/generic/BindingDOMWindow.h:89
> +    KURL completedUrl = url.isEmpty() ? KURL() : completeURL(state, url);

nit: completedUrl -> completedURL

acronyms should either be lowercase or UPPERCASE depending on whether they
appear at the beginning or elsewhere in a name, respectively.

> WebKit/chromium/public/WebViewClient.h:85
>      virtual WebView* createView(WebFrame* creator,

nit: please add a FIXME comment saying that this method is DEPRECATED.

> WebKit/chromium/public/WebViewClient.h:88
> +    // Sometimes it's much better for client API if a new window starts with
a

nit: insert a new line above this comment.

> WebKit/chromium/public/WebViewClient.h:92
> +    virtual WebView* createView(WebFrame* creator,

nit: (bikeshed comment) how about re-ordering these parameters so that it is
creator, request, name, and then features?  that way it more closely resembles
window.open(url, name, features).

> WebKit/chromium/public/WebViewClient.h:96
> +	   (void)request;			 

nit: please just leave the WebURLRequest parameter unnamed so that you don't
need
to write this line.