Bug 25869 - Upstream V8 bindings for V8DomWindow
Summary: Upstream V8 bindings for V8DomWindow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-19 10:49 PDT by Nate Chapin
Modified: 2009-05-20 22:36 PDT (History)
1 user (show)

See Also:


Attachments
patch (10.67 KB, patch)
2009-05-19 10:52 PDT, Nate Chapin
dglazkov: review-
Details | Formatted Diff | Diff
patch2 (11.40 KB, patch)
2009-05-19 16:16 PDT, Nate Chapin
dglazkov: review-
Details | Formatted Diff | Diff
patch3 (10.92 KB, patch)
2009-05-20 09:01 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch4 (10.92 KB, patch)
2009-05-20 09:14 PDT, Nate Chapin
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2009-05-19 10:49:22 PDT
See summary.
Comment 1 Nate Chapin 2009-05-19 10:52:05 PDT
Created attachment 30479 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2009-05-19 12:00:22 PDT
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
Comment 3 Nate Chapin 2009-05-19 16:16:00 PDT
Created attachment 30485 [details]
patch2
Comment 4 Nate Chapin 2009-05-19 16:17:03 PDT
All requested changes made, except that I couldn't find a suitable replacement for IsAscii().
Comment 5 Dimitri Glazkov (Google) 2009-05-19 21:17:19 PDT
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?
Comment 6 Nate Chapin 2009-05-20 09:01:44 PDT
Created attachment 30511 [details]
patch3
Comment 7 Nate Chapin 2009-05-20 09:14:36 PDT
Created attachment 30512 [details]
patch4
Comment 8 Dimitri Glazkov (Google) 2009-05-20 09:26:51 PDT
Comment on attachment 30512 [details]
patch4

Wee!
Comment 9 David Levin 2009-05-20 15:27:36 PDT
http://trac.webkit.org/changeset/43936