[webkit-reviews] review denied: [Bug 25869] Upstream V8 bindings for V8DomWindow. : [Attachment 30479] patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Nate Chapin
<japhet at google.com>'s request for review:
Bug 25869: Upstream V8 bindings for V8DomWindow.
https://bugs.webkit.org/show_bug.cgi?id=25869

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Lots of style nits. Please make sure this still compiles after addressing.

> +v8::Handle<v8::Value> V8Custom::WindowSetTimeoutImpl(const v8::Arguments&
args, bool single_shot)

singleShot

> +{
> +    int num_arguments = args.Length();

argumentCount?

> +    ScriptExecutionContext* script_context =
static_cast<ScriptExecutionContext*>(imp->frame()->document());

scriptContext

> +	   WebCore::String string_function = ToWebCoreString(function);

functionString

> +	   int param_count = num_arguments >= 2 ? num_arguments - 2 : 0;

paramCount

> +static bool IsAscii(const String& str)
> +{
> +    for (size_t i = 0; i < str.length(); i++) {
> +	   if (str[i] > 0xFF)
> +	       return false;
> +    }
> +    return true;
> +}

I believe there's already an isAscii helper somewhere. Please look for it and
reuse. At the very least, put a FIXME to do that :)

> +    Vector<char> in(str.length());

inputCharacters

> +    Vector<char> out;

outputCharacters

> +// TODO(fqian): returning string is cheating, and we should

FIXME: is the WebKit-speak for TODO(name):

> +static String EventNameFromAttributeName(const String& name)

eventNameFromAttributeName

> +    String event_type = name.substring(2);

eventType

> +    String event_type = EventNameFromAttributeName(key);

eventType

> +    if (value->IsNull())
> +	   // Clear the event listener
> +	   imp->clearAttributeEventListener(event_type);

Need braces. I know this is the opposite of what Darin said, but it's actually
an amendment to the rule. No braces on one-liners UNLESS there are comments.

> +    String event_type = EventNameFromAttributeName(key);

eventType

> +    DOMWindow* target_win =
V8Proxy::ToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, window);

targetWindow

> +    DOMWindow* target_win =
V8Proxy::ToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, window);

targetWindow