Working on a new feature enabling workers to pop up notifications UI. Prototyping this in chromium, but WebKit needs the Javascript API (will be protected behind a compile time flag which is default off).
Created attachment 29871[details]
Patch implementing proposed API for worker notifications
Proposed patch which implements the Notification API for workers.
It requires a container such as Chromium to inject a custom Notifications interface which exposes the supported methods, and a NotificationProvider which actually does the rendering work. It's also all disabled by a compile time flag since it is a prototype.
Sure. The notifications API has been in & out of the HTML5 spec over the last 6 months or so; right now it is not in the spec but I am hoping to use a prototype implementation to motivate things again and arrive at a spec that makes the most sense.
I circulated the design doc for this particular code on WHATWG last month:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-March/019113.html
Given that we don't support Persistent Workers it doesn't seem like there is that much of a compelling use case for these since window.open() works just fine. Do we really want to give authors another way to annoy our users with popups?
That's fair, although there is more to it than window.open(): the user agent can manage the screen space on the desktop by queueing, stacking, etc., or relay to a different notification platform (e.g., Growl) if the user wants. I'm hoping letting people try out an experimental feature will reveal the compelling use cases. And I certainly agree about popups, but I think it fits into the current popup permissions world we already have.
In any case, I want to clean up some style problems with my patch (apologies, this is my first experience with webkit), so allow me to try it again and in the meantime I'm happy to discuss the motivation for the feature.
When I think of notifications from an html page, here's some use cases without persistent workers:
1. Calendar notifications about appointments. If you look at something like Google Calendar, right now they do an alert pop up which has several problems for me as a user.
2. Email notifications from a tab that has my email.
3. Potentially IM notifications from in browser im clients.
etc.
Now these notifications do have the potential for abuse, but that can be mitigated by having them only be work for something that is installed like widgets.
Timothy: agreed. 'Headless' persistent workers is the motivation, and there are good reasons to use shared workers for things like calendar notifications (don't want multiple tabs open to the same app to duplicate each other), so starting there but I would like to make this to available to pages as well.
About to submit a new patch, which makes the Notification API available to regular pages via DOMWindow as well as workers via WorkerContext.
Still the only code being proposed to add to WebKit is an interface from Javascript up to a client provider which will implement the security policy and do the display; and plumbing for event callbacks on Notification objects.
I believe without a compelling use case, we should not have this feature in WebKit. As I see it, if we don't have persistent workers (which we don't and don't even yet agree if they are good idea), it seems like every thing here could be achieved with a new parameter to the window.open() feature string.
The use cases are pretty clear: calendar reminders / new email, etc. Anything that wants to alert the user.
Here's why I think it's different from window.open() from the WebKit perspective:
- The semantics of the interface are to alert the user, not necessarily to open a window. If the user is idle, or the desktop is cluttered with other notifications, the user agent might not open a window immediately but queue it instead.
- If the user's preferences are configured so that web-app notifications go to Growl, e.g., the result is not opening a window. This I think is the real compelling use case: allow the browser to enable web applications to use external desktop notification engines according to user prefs.
Suppose we dropped the createHTMLNotification(URL) interface entirely, and just had createNotification(icon, title, text, onclick-link)? That moves it pretty far from window.open(). Would that be a reasonable place to start exploring the use cases?
(In reply to comment #14)
> I believe without a compelling use case, we should not have this feature in
> WebKit. As I see it, if we don't have persistent workers (which we don't and
> don't even yet agree if they are good idea), it seems like every thing here
> could be achieved with a new parameter to the window.open() feature string.
>
As John mentioned, one use case is for applications like Calendar that want to display notifications to the user that doesn't rely on alert(). For an example of other use cases/prior art, you can look at the HTML5 extension APIs that are provided by the Palm WebOS for their Pre devices, which expose similar functionality.
We are trying to address two cases:
1) Applications that want to display notifications with rich HTML/dynamic javascript. One simple example would be a calendar meeting notification that displays "time until meeting" via a dynamic countdown. These would be styled differently from standard popup windows, and would likely have other behavior that differs from popups (be non-focusable so they cannot receive keyboard input).
2) Some platforms have an alternate non-HTML, non-interactive notification format which the user may wish to use - Growl on the Mac is one example of this. Alternatively, some platforms may not allow HTML popups - Palm and Android phones have a specific icon + text format that they can display in their notification bar, and Ubuntu requires that all well-behaved apps use their standard libnotify APIs for displaying notifications.
The suggestion of window.open() to address use case #1 is reasonable, but it does not address the capabilities detection case (how can a web application detect that a given platform supports HTML notifications, so it can fall back to non-HTML notifications)? It is not a reasonable fallback to display a regular popup window in that case (this would especially break for mobile platforms that don't really support non-full-screen windows).
Additionally, window.open() gives the web application no way to interface with non-HTML notification systems like Growl. So window.open(), by itself, is not sufficient.
Another alternative would be to have a single notifications API that takes both an HTML URL and a fallback text+icon if HTML notifications aren't supported:
createNotification(url, altIcon, altTitle, altText);
If you could clarify your concerns further we can try to address them - there are at least a half-dozen teams within Google that are interested in using this, and we've also gotten some good feedback externally as well, so if you are looking for more use cases we can certainly provide them.
Speaking of prior art, this is similar to some of the functionality being exposed via Mozilla Jetpack for Firefox:
https://jetpack.mozillalabs.com/api.html
It sounds like ultimately they are planning on supporting full HTML notifications as well.
Comment on attachment 30562[details]
Notifications Patch
The comments in the bug seem to show a lack of consensus as to whether we want this feature and whether the API is appropriate. These design issues should be hashed out (perhaps on the mailing list) before submitting a patch for code review. Marking r- for now pending further discussion and rough consensus.
I do appreciate the concerns and I would be very happy to discuss the design issues, but putting this out on the mailing list a second time has not generated any discussion. How can we get to a rough consensus?
> // Callback methods for the provider when events occur regarding the notification.
> void display();
I think the naming convention for methods like these are something like callDisplayListener().
> ACCESSOR_SETTER(NotificationOndisplay)
> ACCESSOR_GETTER(NotificationOndisplay)
> ACCESSOR_SETTER(NotificationOnerror)
> ACCESSOR_GETTER(NotificationOnerror)
> ACCESSOR_SETTER(NotificationOnclose)
> ACCESSOR_GETTER(NotificationOnclose)
This technique of v8 bindings for event handler results in more code bloat then we really like. We end up with distinct boiler plate functions for each event attribute. There is another available to us that involves looking up the event attribute by name, resulting in one getter/setter pair for all event attributes.
It be great if the code generator just generated this gunk without intervention... but until that glorious day.
You can see that technique in use in DOMWindow (which has a good number of events), and also in these patches out for review for the DOMAppliationCache...
// see V8DOMApplicationCacheCustom.cpp
http://codereview.chromium.org/113554
// see v8_custom.h and CodeGeneratorV8.pm for the voodoo required
// to use this over the default code bloaty technique
http://codereview.chromium.org/115531
Okay, thanks. I couldn't find a good example in the code that bound event listeners in a way that worked for both document and worker contexts (which I want for notifications), but it looks like the ApplicationCache code does, so will convert to that style.
Heads up... I actually haven't worker'ified the appcache bindings yet. It *wants* to eventually work in workers too, but i haven't actually done that yet. Maybe I can pick up the details of what you do in that context :)
Comment on attachment 30728[details]
Notifications API with Permissions API
I have no idea who can review this. This does not add JSC bindings (perhaps intentionally), which means that this is useless to main-line webkit.
Why don't we just land this on a branch instead?
FYI, Maciej and Weinig, who helped workthrough the design issues, have told me offline they will be looking at this patch soon.
But to address your other points, this patch is just plumbing. Even in V8 it doesn't do the actual work, it just defines an interface for notifications. I think that's valuable to have shared in WebKit rather than added on by each browser, especially as we drive for this to be spec'd officially.
You'll see that the V8 bindings there are effectively pass-throughs into a NotificationProvider interface implemented by the containing browser, where the real work happens depending on the platform. That said, I would be happy to add JSC bindings which also pass-through, but without a NotificationProvider it wouldn't do anything. Also I'd rather do it as a follow-up given the suggestions that this patch is already unwieldy. :)
Created attachment 31655[details]
Notifications API, WebCore only
Uploading a smaller version of the patch which contains the WebCore pieces only, no bindings. This patch has no effect without the ENABLE_NOTIFICATIONS flag.
Comment on attachment 31683[details]
Notifications API, WebCore only
Thanks for the patch! This will be a neat feature. Review- for now, to address the following comments:
- Might be clearer to refer to the guard as ENABLE(NOTIFICATIONS)
> This change adds an (experimental) HTML notification API, being prototyped in Chromium. It's protected by compile-time feature flag ENABLE_NOTIFICATIONS.
- It sure is awkward having classes named "Notification" and "Notificiations". Is there any way to make the latter name more distinct, perhaps "NotificationCenter"?
- I'd suggest renaming m_HTML to m_isHTML to make it more clear that it's a flag, not content.
- m_fields is a pretty poor data member name - it doesn't really clarify what it holds. Perhaps m_parameters would be better. But then I'd want it to also hold the HTML flag and optional URL, so that show() doesn't have to contain an if statement. It seems like those are really parameters controlling display too, right?
- In Notification::show(), it seems that nothing prevents the notification from being shown twice in a row, overwriting m_id. That seems bad.
- In Notification::cancel(), nothing checks if the notification is currently showing. Since m_id is an int, this could lead to closing an entirely different notification.
- Having an integer ID for tracking the open notification seems like poor design anyway. Why not an object? This would allow double-show and double-cancel problems mentioned above to be handled in a clean way. Perhaps even the Notification object itself could be passed to the NotificationProvider's show() and cancel().
- It looks to me like Notification::dispatchDisplayEvent, dispatchErrorEvent and dispatchCloseEvent will not dispatch to the built-in event listeners in the right order - they should come between the capture phase listeners and the bubble phase listeners.
- Even more seriously, the dispatchEvent, addEventListener and removeEventListener appear to be no-ops. That's not a correct implementation of the EventTarget interface.
- As far as I can tell, display(), close() and error() are never called. A comment implies they would be called by the NotificationProvider, but the Notification object is not passed to NotificationProvider, so how can they be?
- Not that this has any practical effect, but why is the Notification IDL interface in the "threads" module?
> module threads {
- These files are cited in the ChangeLog but are missing from the patch, I should add that these sound like they would be incomplete for either JSC or V8 bindings.
* bindings/v8/custom/V8NotificationsCustom.cpp: Added.
* bindings/js/JSWorkerContextCustom.cpp:
- It's not clear from the patch why the show() and cancel() methods on Notification need to be [Custom].
- NotificationProvider seems a bit backwards as a name - as I understand it, this isn't something that provides notifications, but rather provides the cabability of displaying them, so something like NotificationListener, NotificationSink, NotificationView, NotificationClient might be a good name.
- Will this patch even compile with ENABLE(NOTIFICATIONS) turned on, since the bindings code appears to be missing?
- This is sort of an API-level question, but why is there a two-step process to create a notification and then show it? Is there a use case for creating a notification, keeping it around, and then showing it later?
- I don't think it's necessary to explicitly call m_notifications.clear in the WorkerContext destructor.
- Will the test case provided actually pass without the missing JS bindings code?
Thanks for taking the time to look at the patch and provide all the review comments! I'm working on another revision now, but wanted to respond to your comments:
> - Might be clearer to refer to the guard as ENABLE(NOTIFICATIONS)
Ok, will do.
> - It sure is awkward having classes named "Notification" and "Notificiations".
> Is there any way to make the latter name more distinct, perhaps
> "NotificationCenter"?
Fair enough. Will change.
>- Not that this has any practical effect, but why is the Notification IDL
>interface in the "threads" module?
>> module threads {
Again, because I followed WorkerContext as my model to get started. :X I'll change that.
>- It's not clear from the patch why the show() and cancel() methods on
>Notification need to be [Custom].
I'll check that.
> - I'd suggest renaming m_HTML to m_isHTML to make it more clear that it's a
> flag, not content.
> - m_fields is a pretty poor data member name - it doesn't really clarify what
> it holds. Perhaps m_parameters would be better. But then I'd want it to also
> hold the HTML flag and optional URL, so that show() doesn't have to contain an
> if statement. It seems like those are really parameters controlling display
> too, right?
I'm renaming it to NotificationContents, which I think better reflects the distinction: either the contents of the notification are in a remote URL, or they are right here in the object.
> - In Notification::show(), it seems that nothing prevents the notification from
> being shown twice in a row, overwriting m_id. That seems bad.
> - In Notification::cancel(), nothing checks if the notification is currently
> showing. Since m_id is an int, this could lead to closing an entirely different
> notification.
> - Having an integer ID for tracking the open notification seems like poor
> design anyway. Why not an object? This would allow double-show and
> double-cancel problems mentioned above to be handled in a clean way. Perhaps
> even the Notification object itself could be passed to the
> NotificationProvider's show() and cancel().
I'm removing the int and adding an m_isShowing to deal with the double-check. NotificationProvider::show() already does take the object; I will make NotificationProvider::cancel() take the object as well in order to get rid of the ID.
>- It looks to me like Notification::dispatchDisplayEvent, dispatchErrorEvent
>and dispatchCloseEvent will not dispatch to the built-in event listeners in the
>right order - they should come between the capture phase listeners and the
>bubble phase listeners.
This is the part I understand the least, however I followed the model of the way WorkerContext.onmessage events are dispatched. Is it possible that is wrong also?
>- Even more seriously, the dispatchEvent, addEventListener and
>removeEventListener appear to be no-ops. That's not a correct implementation of
>the EventTarget interface.
Yes, apologies. I guess I stubbed that out and never fixed it. Will do.
>- As far as I can tell, display(), close() and error() are never called. A
>comment implies they would be called by the NotificationProvider, but the
>Notification object is not passed to NotificationProvider, so how can they be?
Actually the Notification object _is_ a parameter of NotificationProvider::show(). I think I will make it the only parameter to simplify that API.
>- This is sort of an API-level question, but why is there a two-step process to
>create a notification and then show it? Is there a use case for creating a
>notification, keeping it around, and then showing it later?
That wasn't the use case I had in mind; it was to allow registration of event listeners (ondisplay, onclose) before showing. Sort of like XMLHttpRequest lets you set things up between creation and send().
>- NotificationProvider seems a bit backwards as a name - as I understand it,
>this isn't something that provides notifications, but rather provides the
>cabability of displaying them, so something like NotificationListener,
>NotificationSink, NotificationView, NotificationClient might be a good name.
I was thinking "the thing that provides actual notification of the user", i.e, not a provider of notification _objects_, but a provider of notification itself. :) Would NotificationPlatform be reasonable? NotificationDisplay? NotificationDisplayProvider? View would be okay but is sort of a loaded term at least in Chromium.
>- These files are cited in the ChangeLog but are missing from the patch, I
>should add that these sound like they would be incomplete for either JSC or V8
>bindings.
>* bindings/v8/custom/V8NotificationsCustom.cpp: Added.
>* bindings/js/JSWorkerContextCustom.cpp:
Yeah, the ChangeLog entry is from an earlier iteration. Originally this was only on workers and for JS bindings I returned a stub, so for a moment it time it was complete.
>- Will this patch even compile with ENABLE(NOTIFICATIONS) turned on, since the
>bindings code appears to be missing?
>- Will the test case provided actually pass without the missing JS bindings
>code?
missing bindings... so basically the answer is no. The test case belongs with bindings, which are not in this patch. I completely understand it is lame to check in a chromium-only build flag, on the other hand, the feature requires browser-level changes in addition to bindings to be valuable. So if the patch is unacceptable with a chromium-only build flag even as an intermediate step, I will add JS bindings now. If that can wait, it would be less code to maintain as the feature iterates.
>One thing I must add is that we should probably prefix the feature since it is
>not a standard, ie, WebKitNotification.
I'm okay with that.
Multiple patches was just to allow distributed reviews if it made sense. I was going to suggest that a Chromium reviewer could look at the V8 stuff, although due to V8 churn since I uploaded it I have to revise that patch anyway, so I took it out of the queue for now.
> although
> due to V8 churn since I uploaded it I have to revise that patch anyway, so I
> took it out of the queue for now.
Yeah, sorry you got hit by the DOM wrapper refactoring. Most of those APIs have moved to JSDOMWrapper from V8Proxy.
Comment on attachment 32341[details]
Notifications API, Tests and DumpRenderTree hooks
First of all, I suggest combining this with the other patch. Yes, it will make it overall a bit bigger, but it's a significant benefit to be able to land atomically with test cases. I don't see any problems with the code (well done!) but marking r- since this depends on the other patch, and since I suggest combining with it.
Created attachment 33345[details]
Notifications API, combined Core and Tests
New patch which addresses the feedback and combines the two pieces together. V8 Bindings still pending in a separate patch. One small addition to this patch allows the notification to inform its presenter when it's being destroyed, in order to detach it from future events.
(In reply to comment #45)
> Looking at this patch now. Thanks for revising.
Do we have an ETA on the review (or even partial review comments you can post)? We'd like to land the patch this week if possible, so the earlier we can respond to comments the more likely this becomes.
Comment on attachment 33346[details]
Notifications API, combined Core and Tests
Seems like we need to separate out that *massive* command line into a helper script sooner rather than later. Please consider filing a bug to do that.
\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\notifications
Is it possible to break this up? With 60 other patches in the queue, I simply don't have the stamina for this one. Tempting me to r- it for size.
You'll have the best luck if you can engage an interested reviewer directly. I'm not your man, but maybe you know someone else who is interested in seeing this added.
(In reply to comment #47)
> You'll have the best luck if you can engage an interested reviewer directly.
> I'm not your man, but maybe you know someone else who is interested in seeing
> this added.
Maciej is already looking at this. Agreed that in retrospect this should have been broken up. If we can't bring this to a rapid conclusion (ostensibly we are very close after several rounds of reviews) we may need to do this. Depends on maciej's upcoming review comments.
Please don't r- me for size! The last set of patches *were* broken it up into pieces with all the test hooks separate from the main code but the review feedback was to combine them.
Comment on attachment 33346[details]
Notifications API, combined Core and Tests
Everything seems to be in order. The event handler logic in Notification.cpp in particular looks flawless this time. Kudos! And r=me.
The Windows buildbot is failing after this commit:
http://build.webkit.org/waterfall
I don't expect we need to roll this out, but someone needs to update the windows build system...
(In reply to comment #55)
> The Windows buildbot is failing after this commit:
> http://build.webkit.org/waterfall
> I don't expect we need to roll this out, but someone needs to update the
> windows build system...
Working through build issues now. I guess I should not mark as resolved until build goes green :)
2009-04-28 17:30 PDT, John Gregg
2009-05-06 16:04 PDT, John Gregg
2009-05-20 17:13 PDT, John Gregg
2009-05-21 15:09 PDT, John Gregg
2009-05-27 23:53 PDT, John Gregg
2009-06-22 10:58 PDT, John Gregg
2009-06-22 15:45 PDT, John Gregg
2009-07-06 18:10 PDT, John Gregg
2009-07-06 18:10 PDT, John Gregg
2009-07-06 18:11 PDT, John Gregg
2009-07-06 18:14 PDT, John Gregg
2009-07-06 18:18 PDT, John Gregg
2009-07-23 10:54 PDT, John Gregg
2009-07-23 10:58 PDT, John Gregg