Bug 25869 – Upstream V8 bindings for V8DomWindow

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

See summary.

[Nate Chapin]

Created attachment 30479 [details]
patch

[Dimitri Glazkov (Google)]

Comment on attachment 30479 [details]
patch

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

[Nate Chapin]

Created attachment 30485 [details]
patch2

[Nate Chapin]

All requested changes made, except that I couldn't find a suitable replacement for IsAscii().

[Dimitri Glazkov (Google)]

Comment on attachment 30485 [details]
patch2

Darn it! One more round of laundering :-\

> +            V8Proxy::ThrowError(V8Proxy::GENERAL_ERROR, "Cannot decode base64");
> +            return v8::Undefined();

One more! Use throwError helper from V8Proxy.h

> +    if (args.Length() < 1) {
> +        V8Proxy::ThrowError(V8Proxy::SYNTAX_ERROR, "Not enough arguments");
> +        return v8::Undefined();
> +    }

Same here.

> +    if (args.Length() < 1) {
> +        V8Proxy::ThrowError(V8Proxy::SYNTAX_ERROR, "Not enough arguments");
> +        return v8::Undefined();
> +    }

And here.

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

toWebCoreString

> +    return Base64Convert(str, true);

Whoops. Functions should be camelCase. How did I miss this earlier? convertBase64?

[Nate Chapin]

Created attachment 30511 [details]
patch3

[Nate Chapin]

Created attachment 30512 [details]
patch4

[Dimitri Glazkov (Google)]

Comment on attachment 30512 [details]
patch4

Wee!

[David Levin]

http://trac.webkit.org/changeset/43936