Bug 107449

Created attachment 183782 [details] Patch

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?

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

Created attachment 183792 [details] Patch

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

Created attachment 183793 [details] Patch

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

Created attachment 183975 [details] Patch

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.

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.