[webkit-reviews] review denied: [Bug 25463] Desktop Notifications API : [Attachment 31683] Notifications API, WebCore only

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied John Gregg <johnnyg at google.com>'s
request for review:
Bug 25463: Desktop Notifications API
https://bugs.webkit.org/show_bug.cgi?id=25463

Attachment 31683: Notifications API, WebCore only
https://bugs.webkit.org/attachment.cgi?id=31683&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
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?