Bug 107449 – [Qt][WK1] Web Notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Allan Sandfeld Jensen]

Web Notifications in WebKit1 still have a few issues, among those:

requestPermission should use securityOrigin
DumpRenderTree should support testing of web notification.
requestPermission should and not ask the user if an answer is already given
A deny reply from the user should also be remember.

[Allan Sandfeld Jensen]

Created attachment 183782 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 183782 [details]
Patch

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

r-ing until we agree on the dumpNotification part.

> Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:-361
> -    m_cachedPermissions.remove(context);
> -

Could you explain why this line isn't needed, in the changelog?

> Tools/DumpRenderTree/qt/TestRunnerQt.cpp:-48
> -    DumpRenderTreeSupportQt::dumpNotification(true);

If we remove this, it would be better to remove all the code that this triggered in NotificationPresenterClientQt.cpp and all test expected files that relied on this.
Could this be done in a different patch as well?

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (From update of attachment 183782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183782&action=review
> 
> r-ing until we agree on the dumpNotification part.
> 
> > Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:-361
> > -    m_cachedPermissions.remove(context);
> > -
> 
> Could you explain why this line isn't needed, in the changelog?
> 
> > Tools/DumpRenderTree/qt/TestRunnerQt.cpp:-48
> > -    DumpRenderTreeSupportQt::dumpNotification(true);
> 
> If we remove this, it would be better to remove all the code that this triggered in NotificationPresenterClientQt.cpp and all test expected files that relied on this.
> Could this be done in a different patch as well?

That might be a good idea. It just means all the tests would need qt-specific baselines, unless we choose not enable them yet either.

[Allan Sandfeld Jensen]

Created attachment 183792 [details]
Patch

[Early Warning System Bot]

Comment on attachment 183792 [details]
Patch

Attachment 183792 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16042091

[Allan Sandfeld Jensen]

Created attachment 183793 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183793 [details]
Patch

Attachment 183793 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16031175

New failing tests:
fast/notifications/notifications-display-close-events.html
fast/notifications/notifications-click-event.html
fast/notifications/notifications-no-icon.html
fast/notifications/notifications-replace.html
fast/notifications/notifications-with-permission.html
fast/notifications/notifications-rtl.html

[Allan Sandfeld Jensen]

Created attachment 183975 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 183975 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:381
> +    m_cachedPermissions.remove(context->securityOrigin()->toRawString());

You check context->securityOrigin() in checkPermission but not here and it seems like it is exposed to the same risk, could you verify?

> Tools/DumpRenderTree/qt/TestRunnerQt.cpp:-75
> -    m_desktopNotificationAllowedOrigins.clear();

You should probably remove the member from the header as well.

[Allan Sandfeld Jensen]

Closing bug since the split patches have landed. The last part about changing to using security-origin, is not covered by any tests, and might not even fit the existing API.