Bug 28271 – V8 Bindings for Desktop Notifications

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[John Gregg]

Desktop notifications feature needs V8 Bindings.

[John Gregg]

Created attachment 34764 [details]
V8 Bindings for Notifications

[Eric Seidel (no email)]

Comment on attachment 34764 [details]
V8 Bindings for Notifications

Why so much custom bindings code?  We'd much rather share as much bindings code with JSC as possible.  Why do all of these functions need to be custom?

[John Gregg]

The V8 code generator requires event listener accessors to be custom, which is the vast majority of that custom file.

Beyond that there are the 2 createNotification functions which I made custom, as explained in the ChangeLog, because I need them to be different in page context and worker context.  

I don't particularly like that either since the difference is so minor.  The alternative solution would be to change the code generator to be aware that V8 uses different proxy objects in worker and page contexts, and mark up the IDL with a tag that causes context-aware code to be generated.  But I consider that a more substantial change.

[John Gregg]

For completeness of my response, there is also requestPermission which was already custom in JSC for the same reason, the need to distinguish between worker and page contexts, and in a more substantial way: not executing in worker context, rather than just using different proxies.

[Eric Seidel (no email)]

Seems we need to file some bugs about making the generators better.  Adding more copy/paste code is bad news bears. :(

[John Gregg]

Eric, I agree with that and I'm happy to help with it (although the code generator is not an area of my expertise yet).  But I think that could happen in parallel with moving notifications forward for Chromium.  Would you want to wait on this patch until those changes can be made?

[Eric Seidel (no email)]

I left this r=? because I agree, it doesn't need to block this change.  That said, we need to file some bugs about the missing features.  It would be great if you could file bugs for the needed additions to the code generators, and ideally link them here. :)

[Dimitri Glazkov (Google)]

Comment on attachment 34764 [details]
V8 Bindings for Notifications

Overall, looks good. Some style nits around abbreviations:

> +    Notification* notif = V8DOMWrapper::convertToNativeObject<Notification>(V8ClassIndex::NOTIFICATION, args.Holder());

notification

> +    Notification* notif = V8DOMWrapper::convertToNativeObject<Notification>(V8ClassIndex::NOTIFICATION, args.Holder());

notification

> +    NotificationCenter* notifInterface = V8DOMWrapper::convertToNativeObject<NotificationCenter>(V8ClassIndex::NOTIFICATIONCENTER, args.Holder());

notificationCenter

> +    RefPtr<V8CustomVoidCallback> callback;
> +    if (args.Length() > 0) {
> +        if (!args[0]->IsObject()) {
> +            V8Proxy::throwError(V8Proxy::TypeError, "Callback must be of valid type.");
> +            return v8::Undefined();

return throwError("Callback must...");

[John Gregg]

Created attachment 35090 [details]
V8 Bindings for Notifications

Thanks, I've addressed those comments in this revised patch.

[Dimitri Glazkov (Google)]

Comment on attachment 35090 [details]
V8 Bindings for Notifications

r=me.

[Eric Seidel (no email)]

Comment on attachment 35090 [details]
V8 Bindings for Notifications

Rejecting patch 35090 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'rebase']"  exit_code: 1  cwd: None

[Adam Barth]

Comment on attachment 35090 [details]
V8 Bindings for Notifications

I blame the rebase bug.  Let's try again.

[Eric Seidel (no email)]

Comment on attachment 35090 [details]
V8 Bindings for Notifications

Clearing flags on attachment: 35090

Committed r47492: <http://trac.webkit.org/changeset/47492>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Peter Kasting]

(In reply to comment #13)
> (From update of attachment 35090 [details])
> Clearing flags on attachment: 35090
> 
> Committed r47492: <http://trac.webkit.org/changeset/47492>

This seems to have caused the following compile error:

WebCore\bindings\v8\WorkerContextExecutionProxy.cpp(41) : fatal error C1083: Cannot open include file: 'Notification.h': No such file or directory

[John Gregg]

http://codereview.chromium.org/173066 is ready to go to fix the chromium build, if that's what you mean.  

Or is the webkit build on its own broken?

[Peter Kasting]

(In reply to comment #16)
> http://codereview.chromium.org/173066 is ready to go to fix the chromium build,
> if that's what you mean.  

Ah, OK.  Sorry for the harassment :)