Desktop notifications feature needs V8 Bindings.
Created attachment 34764 [details] V8 Bindings for Notifications
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?
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.
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.
Seems we need to file some bugs about making the generators better. Adding more copy/paste code is bad news bears. :(
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?
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. :)
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...");
Created attachment 35090 [details] V8 Bindings for Notifications Thanks, I've addressed those comments in this revised patch.
Comment on attachment 35090 [details] V8 Bindings for Notifications r=me.
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
Comment on attachment 35090 [details] V8 Bindings for Notifications I blame the rebase bug. Let's try again.
Comment on attachment 35090 [details] V8 Bindings for Notifications Clearing flags on attachment: 35090 Committed r47492: <http://trac.webkit.org/changeset/47492>
All reviewed patches have been landed. Closing bug.
(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
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?
(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 :)