Created attachment 131152 [details] Patch

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 on attachment 131152 [details] Patch Attachment 131152 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11898981

Created attachment 131155 [details] new Notification(title, body, iconURL)

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

Created attachment 131443 [details] Patch

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.

(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.

> 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.

(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

(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.

> 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.

(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.

(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.

Created attachment 137664 [details] Patch

Comment on attachment 137664 [details] Patch Attachment 137664 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12424409

Comment on attachment 137664 [details] Patch Attachment 137664 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12428121

Comment on attachment 137664 [details] Patch Attachment 137664 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12428122

Created attachment 137717 [details] Patch

Comment on attachment 137717 [details] Patch Attachment 137717 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12423924

Comment on attachment 137717 [details] Patch Attachment 137717 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12425507

Comment on attachment 137717 [details] Patch Attachment 137717 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12426435

Created attachment 137855 [details] Patch

Comment on attachment 137855 [details] Patch Attachment 137855 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12437144

Created attachment 137966 [details] Patch

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 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

Created attachment 138049 [details] Patch

Comment on attachment 138049 [details] Patch Attachment 138049 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12460145

Comment on attachment 138049 [details] Patch Attachment 138049 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12480081

Created attachment 138055 [details] Patch

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 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 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?

(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.

Probably relevant to Jian's comment about Qt notification & icon code: https://bugs.webkit.org/show_bug.cgi?id=80700

(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 .

Created attachment 138163 [details] Patch

Committed r114855: <http://trac.webkit.org/changeset/114855>

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.