Bug 24616 - Add custom V8 bindings for DOMWindow
Summary: Add custom V8 bindings for DOMWindow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Mike Belshe
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-16 07:17 PDT by Mike Belshe
Modified: 2009-03-28 10:01 PDT (History)
2 users (show)

See Also:


Attachments
patch (25.77 KB, patch)
2009-03-25 13:01 PDT, Mike Belshe
dglazkov: review-
Details | Formatted Diff | Diff
patch w/ Dimitri's comments (25.93 KB, patch)
2009-03-26 10:05 PDT, Mike Belshe
dglazkov: review+
Details | Formatted Diff | Diff
use notImplementedByInterceptor() (25.94 KB, patch)
2009-03-26 13:59 PDT, Mike Belshe
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Belshe 2009-03-16 07:17:44 PDT
Custom bindings for the DOMWindow under v8.
Comment 1 Mike Belshe 2009-03-25 13:01:54 PDT
Created attachment 28941 [details]
patch
Comment 2 Eric Seidel (no email) 2009-03-25 16:04:41 PDT
Sam will likely be curious about this change too.
Comment 3 Dimitri Glazkov (Google) 2009-03-26 07:38:42 PDT
Comment on attachment 28941 [details]
patch

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
Comment 4 Dimitri Glazkov (Google) 2009-03-26 07:39:07 PDT
Comment on attachment 28941 [details]
patch

Whoops, I meant r-.
Comment 5 Mike Belshe 2009-03-26 10:05:01 PDT
Created attachment 28972 [details]
patch w/ Dimitri's comments
Comment 6 Dimitri Glazkov (Google) 2009-03-26 13:13:18 PDT
Comment on attachment 28972 [details]
patch w/ Dimitri's comments

Good enough. The following are nice, but not required.

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

notHandledByInterceptor()

> +    if (!name->IsString())
> +        return v8::Handle<v8::Value>();

notHandledByInterceptor()

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

notHandledByInterceptor()


> +    if (!window)
> +        return v8::Handle<v8::Value>();

notHandledByInterceptor()


> +    Frame* frame = window->frame();
> +    // window is detached from a frame.
> +    if (!frame)
> +        return v8::Handle<v8::Value>();

notHandledByInterceptor()


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

notHandledByInterceptor()
Comment 7 Mike Belshe 2009-03-26 13:59:43 PDT
Created attachment 28983 [details]
use notImplementedByInterceptor()
Comment 8 Dimitri Glazkov (Google) 2009-03-28 09:49:19 PDT
Landed as http://trac.webkit.org/changeset/42071.
Comment 9 Dimitri Glazkov (Google) 2009-03-28 09:50:29 PDT
Whoops, I just realized this file is in the wrong place. It should be in bindings/v8/custom.
Comment 10 Dimitri Glazkov (Google) 2009-03-28 10:01:23 PDT
Fix landed in http://trac.webkit.org/changeset/42072.