[Webkit-unassigned] [Bug 41441] createWindow method should only do window-creating without URL navigator

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74080|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #28 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-01-07 09:00:04 PST ---
(From update of attachment 74080)
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.

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