Summary: | [Qt][WK1] Web Notifications | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||
Component: | WebKit Qt | Assignee: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, jturcotte, webkit-ews, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 107694, 107696 | ||||||||||||
Bug Blocks: | 88186, 103747 | ||||||||||||
Attachments: |
|
Description
Allan Sandfeld Jensen
2013-01-21 07:16:33 PST
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. |