[webkit-reviews] review granted: [Bug 24616] Add custom V8 bindings for DOMWindow : [Attachment 28941] patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has granted Mike Belshe
<mike at belshe.com>'s request for review:
Bug 24616: Add custom V8 bindings for DOMWindow
https://bugs.webkit.org/show_bug.cgi?id=24616

Attachment 28941: patch
https://bugs.webkit.org/attachment.cgi?id=28941&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
These are all style-nits, obviously the code works and is good otherwise.

Bug URL in ChangeLog.

> +    WindowSetLocation(imp, ToWebCoreString(value));

toWebCoreString

> +	   String eventType = ToWebCoreString(args[0]);

toWebCoreString


> +	   String eventType = ToWebCoreString(args[0]);

toWebCoreString


> +    String message = ToWebCoreString(args[0]);

toWebCoreString

> +    WindowFeatures wargs;

wargs -> windowFeatures? No strong feelings here, just wargs sounds a bit
overcondensed.

> +	       (!parseURL(urlString).startsWith("javascript:", false) ||
> +		ScriptController::isSafeScript(frame))) {

|| goes on new line or you could just remove line break

> +	   return v8::Handle<v8::Value>();

notHandledByInterceptor() here and elsewhere in this method.

> +    String propName = ToWebCoreString(name);

toWebCoreString

> +    static HashMap<String, String> kLazyInitMap;

kLazyInitMap -> lazyInitMap

> +	   if (context.IsEmpty()) return v8::Handle<v8::Value>();

return on new line.

> +    if (!parseURL(v).startsWith("javascript:", false) ||

|| on new line or remove line break

> +    int handle = ToInt32(args[0]);

toInt32