Bug 80477 - Add Notification constructor
Summary: Add Notification constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on: 83472
Blocks: 80472
  Show dependency treegraph
 
Reported: 2012-03-06 20:38 PST by Jon Lee
Modified: 2012-04-23 09:17 PDT (History)
11 users (show)

See Also:


Attachments
Patch (11.87 KB, patch)
2012-03-09 21:56 PST, Jon Lee
no flags Details | Formatted Diff | Diff
new Notification(title, body, iconURL) (11.83 KB, patch)
2012-03-09 22:59 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (34.26 KB, patch)
2012-03-12 16:29 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (27.15 KB, patch)
2012-04-18 03:09 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (28.43 KB, patch)
2012-04-18 10:20 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (30.87 KB, patch)
2012-04-19 00:05 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (33.73 KB, patch)
2012-04-19 13:26 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (34.72 KB, patch)
2012-04-19 23:29 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (34.66 KB, patch)
2012-04-20 00:31 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (35.80 KB, patch)
2012-04-20 14:17 PDT, Jon Lee
jianli: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-03-06 20:38:38 PST
Add constructor function to create Notification, with optional icon URL.

<rdar://problem/10912431>
Comment 1 Jon Lee 2012-03-09 21:56:16 PST
Created attachment 131152 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-09 21:58:41 PST
Attachment 131152 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/notifications/Notification.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gyuyoung Kim 2012-03-09 22:18:31 PST
Comment on attachment 131152 [details]
Patch

Attachment 131152 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11898981
Comment 4 Jon Lee 2012-03-09 22:59:39 PST
Created attachment 131155 [details]
new Notification(title, body, iconURL)
Comment 5 Kentaro Hara 2012-03-10 03:23:32 PST
Comment on attachment 131155 [details]
new Notification(title, body, iconURL)

View in context: https://bugs.webkit.org/attachment.cgi?id=131155&action=review

> Source/WebCore/ChangeLog:8
> +

Would you please add the link to the spec that supports this change?

> Source/WebCore/notifications/Notification.idl:38
> +        Constructor(in DOMString title, in DOMString body, in [Optional=DefaultIsUndefined] DOMString iconUrl),

This constructor signature is different from the spec: http://www.w3.org/TR/notifications/#notification-constructor
Comment 6 Jon Lee 2012-03-12 16:29:33 PDT
Created attachment 131443 [details]
Patch
Comment 7 Adam Barth 2012-03-12 16:52:49 PDT
Comment on attachment 131443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131443&action=review

Why does this constructor need custom code?  It seems like we should be able to handle this constructor with the code generator.

> Source/WebCore/page/DOMWindow.idl:175
> +        attribute NotificationConstructor Notification;

I'm slightly confused by these patches.  Suppose Chromium is going to cut a release branch the day after this patch lands.  What set of ENABLE flags should we set on that branch?  The doesn't seem to be a way to enable the LEGACY_NOTIFICATIONS without also enabling this half-baked implementation of the new notifications API.
Comment 8 Jon Lee 2012-03-12 17:29:34 PDT
(In reply to comment #7)
> (From update of attachment 131443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131443&action=review
> 
> Why does this constructor need custom code?  It seems like we should be able to handle this constructor with the code generator.

Can you point me to an .idl that does this only with the code generator? I was looking at geolocation, and there is custom code there to parse the options. But maybe in that case it was because the functions provided are just callback functions, but here we're dealing with event listeners.

> 
> > Source/WebCore/page/DOMWindow.idl:175
> > +        attribute NotificationConstructor Notification;
> 
> I'm slightly confused by these patches.  Suppose Chromium is going to cut a release branch the day after this patch lands.  What set of ENABLE flags should we set on that branch?  The doesn't seem to be a way to enable the LEGACY_NOTIFICATIONS without also enabling this half-baked implementation of the new notifications API.

I will remove the dependency of LEGACY_NOTIFICATIONS and NOTIFICATIONS flags, and make them separate. So ports that wish to support the legacy API can use LEGACY_NOTIFICATIONS, and those that wish to support the new one can use NOTIFICATIONS. A new patch is on the way to reflect this.
Comment 9 Adam Barth 2012-03-12 17:40:11 PDT
> Can you point me to an .idl that does this only with the code generator? I was looking at geolocation, and there is custom code there to parse the options. But maybe in that case it was because the functions provided are just callback functions, but here we're dealing with event listeners.

Maybe I'm unclear about what "this" is.  Is this the latest version of the spec that you're implementing?

http://dev.w3.org/2006/webapi/WebNotifications/publish/Notifications.html

It lists the constructor as follows:

  Notification Notification(in DOMString iconUrl, in DOMString title, in DOMString body);

which you should be able to implement with https://trac.webkit.org/wiki/WebKitIDL#Constructor

    interface [
        Constructor(in DOMString iconUrl, in DOMString title, in DOMString body),
        CallWith=ScriptExecutionContext
    ] XXX {
        [...]
    };

The code in your patch doesn't seem to match the spec, however.  You seem to be doing something with JSDictionary.  I suspect you might need to flesh out the support for OptionsObject in JSC, but it's hard to know without seeing the specification that you're trying to implement.
Comment 10 Kentaro Hara 2012-03-12 17:56:46 PDT
(In reply to comment #9)
> The code in your patch doesn't seem to match the spec, however.  You seem to be doing something with JSDictionary.  I suspect you might need to flesh out the support for OptionsObject in JSC, but it's hard to know without seeing the specification that you're trying to implement.

FYI: OptionsObject in JSC is going to land in https://bugs.webkit.org/show_bug.cgi?id=80207
Comment 11 Jon Lee 2012-03-12 18:43:09 PDT
(In reply to comment #9)
> > Can you point me to an .idl that does this only with the code generator? I was looking at geolocation, and there is custom code there to parse the options. But maybe in that case it was because the functions provided are just callback functions, but here we're dealing with event listeners.
> 
> Maybe I'm unclear about what "this" is.  Is this the latest version of the spec that you're implementing?
> 
> http://dev.w3.org/2006/webapi/WebNotifications/publish/Notifications.html

No, it is not. If you look at the change log there's a link to a recent thread in the WG, where there was consensus to alter the constructor signature to use an options object. The spec has not been updated with the changes yet.
Comment 12 Adam Barth 2012-03-12 18:46:55 PDT
> No, it is not. If you look at the change log there's a link to a recent thread in the WG, where there was consensus to alter the constructor signature to use an options object. The spec has not been updated with the changes yet.

Thanks.  From that email:

> Notification(DOMString title, DOMString body, NotificationOptions optionsDict);

It definitely sounds like you want the OptionsObject support in JSC from Bug 80207.
Comment 13 Jon Lee 2012-03-12 20:05:22 PDT
(In reply to comment #12)
> > No, it is not. If you look at the change log there's a link to a recent thread in the WG, where there was consensus to alter the constructor signature to use an options object. The spec has not been updated with the changes yet.
> 
> Thanks.  From that email:
> 
> > Notification(DOMString title, DOMString body, NotificationOptions optionsDict);
> 
> It definitely sounds like you want the OptionsObject support in JSC from Bug 80207.

It looks like bug 80802 is just renaming OptionsObject to V8Dictionary, which is the analog to JSDictionary that I use here.
Comment 14 Kentaro Hara 2012-03-12 20:09:29 PDT
(In reply to comment #13)
> > It definitely sounds like you want the OptionsObject support in JSC from Bug 80207.
> 
> It looks like bug 80802 is just renaming OptionsObject to V8Dictionary, which is the analog to JSDictionary that I use here.

No. We are trying to unify the interface between JSC and V8. The unified interface will be named OptionsObject after the bug 80207 is fixed. After that, the bug 80802 will rename OptionsObject to Dictionary for clarification. Anyway, what is important is to use the interface of (not JSDictionary but) OptionsObject in your patch, because OptionsObject will become the unified interface.
Comment 15 Jon Lee 2012-04-18 03:09:21 PDT
Created attachment 137664 [details]
Patch
Comment 16 Early Warning System Bot 2012-04-18 03:23:11 PDT
Comment on attachment 137664 [details]
Patch

Attachment 137664 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12424409
Comment 17 Early Warning System Bot 2012-04-18 03:27:38 PDT
Comment on attachment 137664 [details]
Patch

Attachment 137664 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12428121
Comment 18 WebKit Review Bot 2012-04-18 03:30:58 PDT
Comment on attachment 137664 [details]
Patch

Attachment 137664 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12428122
Comment 19 Jon Lee 2012-04-18 10:20:26 PDT
Created attachment 137717 [details]
Patch
Comment 20 WebKit Review Bot 2012-04-18 11:14:05 PDT
Comment on attachment 137717 [details]
Patch

Attachment 137717 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12423924
Comment 21 Early Warning System Bot 2012-04-18 11:36:53 PDT
Comment on attachment 137717 [details]
Patch

Attachment 137717 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12425507
Comment 22 Early Warning System Bot 2012-04-18 12:28:53 PDT
Comment on attachment 137717 [details]
Patch

Attachment 137717 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12426435
Comment 23 Jon Lee 2012-04-19 00:05:18 PDT
Created attachment 137855 [details]
Patch
Comment 24 WebKit Review Bot 2012-04-19 00:33:58 PDT
Comment on attachment 137855 [details]
Patch

Attachment 137855 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12437144
Comment 25 Jon Lee 2012-04-19 13:26:01 PDT
Created attachment 137966 [details]
Patch
Comment 26 Jian Li 2012-04-19 17:21:10 PDT
Comment on attachment 137966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137966&action=review

> Source/WebCore/bindings/js/Dictionary.h:60
> +    bool isUndefinedOrNull() const { return m_dictionary.isValid(); }

Should this method return true only if m_dictionary is not valid or empty?

> Source/WebCore/notifications/DOMWindowNotifications.idl:34
> +        attribute NotificationConstructor Notification;

Should this be only exposed to ENABLE(NOTIFICATIONS)?

> Source/WebCore/notifications/Notification.cpp:188
> +        setPendingActivity(this);

Should this be needed for all other platforms?

> Source/WebCore/notifications/Notification.cpp:292
>      unsetPendingActivity(this);

This seems to be unbalanced for non-QT platform. startLoading is only called for QT while finishloading is called for all platforms. I know this is not caused by this patch, but could you please fix this? Thanks.
Comment 27 Jon Lee 2012-04-19 18:18:54 PDT
Comment on attachment 137966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137966&action=review

>> Source/WebCore/bindings/js/Dictionary.h:60
>> +    bool isUndefinedOrNull() const { return m_dictionary.isValid(); }
> 
> Should this method return true only if m_dictionary is not valid or empty?

Yeah that implementation is completely wrong! Missing a "!". But, having an empty dictionary should return false, because it is a defined object.

>> Source/WebCore/notifications/DOMWindowNotifications.idl:34
>> +        attribute NotificationConstructor Notification;
> 
> Should this be only exposed to ENABLE(NOTIFICATIONS)?

Yes, will fix.

>> Source/WebCore/notifications/Notification.cpp:188
>> +        setPendingActivity(this);
> 
> Should this be needed for all other platforms?

This could be applied to all platforms. Might be easier to just simplify the pending activity for Qt.

>> Source/WebCore/notifications/Notification.cpp:292
>>      unsetPendingActivity(this);
> 
> This seems to be unbalanced for non-QT platform. startLoading is only called for QT while finishloading is called for all platforms. I know this is not caused by this patch, but could you please fix this? Thanks.

I can just remove this call, so that it is only handled via the show() -> finalize() loop
Comment 28 Jon Lee 2012-04-19 23:29:18 PDT
Created attachment 138049 [details]
Patch
Comment 29 Early Warning System Bot 2012-04-19 23:44:53 PDT
Comment on attachment 138049 [details]
Patch

Attachment 138049 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12460145
Comment 30 Early Warning System Bot 2012-04-19 23:48:47 PDT
Comment on attachment 138049 [details]
Patch

Attachment 138049 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12480081
Comment 31 Jon Lee 2012-04-20 00:31:32 PDT
Created attachment 138055 [details]
Patch
Comment 32 Jian Li 2012-04-20 11:04:24 PDT
Comment on attachment 138055 [details]
Patch

I notice that there're a lot of QT-specific icon-loading logic in Notification.*. Some of them are guarded while other are not. This makes Notification implementation quite complex and fragmented. I am thinking probably we can move all icon-loading logic out of Notification class and put it into something like NotificationIconLoader. Certainly, this is not part of this patch. Could you please file a bug on this and assign it to QT guy?


View in context: https://bugs.webkit.org/attachment.cgi?id=138055&action=review

> Source/WebCore/notifications/Notification.cpp:188
>      if (m_state == Idle && m_notificationCenter->client()) {

It seems that we can merge the logic for MAC platform and other non-QT platform. Since NotificationClient::show could return false for an error, why not also checking it before setting state to Showing, as what we do in non-QT platform.

> Source/WebCore/notifications/Notification.cpp:304
>      unsetPendingActivity(this);

For non-mac platform, where do we call unsetPendingActivity?

> Source/WebCore/notifications/Notification.h:165
>      void finishLoading();

finishLoading is also only related to icon loading in QT. Could you please also suffix it with Icon?
Comment 33 Jon Lee 2012-04-20 11:54:50 PDT
Comment on attachment 138055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138055&action=review

>> Source/WebCore/notifications/Notification.cpp:188
>>      if (m_state == Idle && m_notificationCenter->client()) {
> 
> It seems that we can merge the logic for MAC platform and other non-QT platform. Since NotificationClient::show could return false for an error, why not also checking it before setting state to Showing, as what we do in non-QT platform.

I don't understand what the boolean represents. Does is simply represent whether the notification got queued rather than shown, or that something went wrong on the platform side?

>> Source/WebCore/notifications/Notification.cpp:304
>>      unsetPendingActivity(this);
> 
> For non-mac platform, where do we call unsetPendingActivity?

The guards need to disappear.

>> Source/WebCore/notifications/Notification.h:165
>>      void finishLoading();
> 
> finishLoading is also only related to icon loading in QT. Could you please also suffix it with Icon?

You mentioned in the previous patch that the unsetPendingActivity() in finishLoading() is used by non-Qt ports. So I was reluctant to rename this to finishLoadingIcon since other ports are not loading the icon.

Did I misunderstand your comment? Is it only Qt that executes this?
Comment 34 Jon Lee 2012-04-20 11:54:56 PDT
Comment on attachment 138055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138055&action=review

>> Source/WebCore/notifications/Notification.cpp:188
>>      if (m_state == Idle && m_notificationCenter->client()) {
> 
> It seems that we can merge the logic for MAC platform and other non-QT platform. Since NotificationClient::show could return false for an error, why not also checking it before setting state to Showing, as what we do in non-QT platform.

I don't understand what the boolean represents. Does is simply represent whether the notification got queued rather than shown, or that something went wrong on the platform side?

>> Source/WebCore/notifications/Notification.cpp:304
>>      unsetPendingActivity(this);
> 
> For non-mac platform, where do we call unsetPendingActivity?

The guards need to disappear.

>> Source/WebCore/notifications/Notification.h:165
>>      void finishLoading();
> 
> finishLoading is also only related to icon loading in QT. Could you please also suffix it with Icon?

You mentioned in the previous patch that the unsetPendingActivity() in finishLoading() is used by non-Qt ports. So I was reluctant to rename this to finishLoadingIcon since other ports are not loading the icon.

Did I misunderstand your comment? Is it only Qt that executes this?
Comment 35 Jian Li 2012-04-20 12:08:25 PDT
(In reply to comment #34)
> (From update of attachment 138055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138055&action=review
> 
> >> Source/WebCore/notifications/Notification.cpp:188
> >>      if (m_state == Idle && m_notificationCenter->client()) {
> > 
> > It seems that we can merge the logic for MAC platform and other non-QT platform. Since NotificationClient::show could return false for an error, why not also checking it before setting state to Showing, as what we do in non-QT platform.
> 
> I don't understand what the boolean represents. Does is simply represent whether the notification got queued rather than shown, or that something went wrong on the platform side?

I just checked the implementation for NotificationClient::show on Chromium and Mac platforms, it seems that we mean something went wrong on the platform side. Currently on Mac, it does not check the return value of show().

> 
> >> Source/WebCore/notifications/Notification.cpp:304
> >>      unsetPendingActivity(this);
> > 
> > For non-mac platform, where do we call unsetPendingActivity?
> 
> The guards need to disappear.
> 
> >> Source/WebCore/notifications/Notification.h:165
> >>      void finishLoading();
> > 
> > finishLoading is also only related to icon loading in QT. Could you please also suffix it with Icon?
> 
> You mentioned in the previous patch that the unsetPendingActivity() in finishLoading() is used by non-Qt ports. So I was reluctant to rename this to finishLoadingIcon since other ports are not loading the icon.
> 
> Did I misunderstand your comment? Is it only Qt that executes this?

My previous comment about "finishLoading() is used by non-Qt ports" is wrong after I read the code more carefully. Only QT port does the icon loading and all the callers of finishLoading(), did***, are solely executed by QT port. I am fine that you leave the name unchanged since I do feel that we need to refactor this complicated icon-loading logic for QT port.
Comment 36 Adenilson Cavalcanti Silva 2012-04-20 12:20:50 PDT
Probably relevant to Jian's comment about Qt notification & icon code: https://bugs.webkit.org/show_bug.cgi?id=80700
Comment 37 Jon Lee 2012-04-20 13:05:19 PDT
(In reply to comment #32)
> (From update of attachment 138055 [details])
> I notice that there're a lot of QT-specific icon-loading logic in Notification.*. Some of them are guarded while other are not. This makes Notification implementation quite complex and fragmented. I am thinking probably we can move all icon-loading logic out of Notification class and put it into something like NotificationIconLoader. Certainly, this is not part of this patch. Could you please file a bug on this and assign it to QT guy?

Filed bug 84484 .
Comment 38 Jon Lee 2012-04-20 14:17:18 PDT
Created attachment 138163 [details]
Patch
Comment 39 Jon Lee 2012-04-21 17:18:33 PDT
Committed r114855: <http://trac.webkit.org/changeset/114855>
Comment 40 Jer Noble 2012-04-23 09:17:12 PDT
The following tests started failing on Mountain Lion Debug Tests in r114855:

    fast/js/global-constructors.html
    fast/dom/Window/window-properties.html
    fast/dom/prototype-inheritance-2.html

Filed <https://bugs.webkit.org/show_bug.cgi?id=84604> to track this test failure.