Bug 95093

<rdar://problem/12179649>

Created attachment 160734 [details] Patch

Could we do this in window.internals instead? so we only have to implement it once instead of for each DRT?

(In reply to comment #3) > Could we do this in window.internals instead? so we only have to implement it once instead of for each DRT? It would be great if we can only do this once, but I do not think it is possible with this feature. There are three sets of APIs involved: 1) Permissions management: retrieving and requesting from JS. 2) Notification management: showing and closing from JS. 3) Platform events: mimicking a user clicking on and closing a notification from the platform. The first two sets are handled at the WK1/2 layer through port-specific implementations of NotificationClient, which I think implies different solutions for each DRT/WTR port. The last set is needed only by DRT/WTR, so I think it should remain in testRunner.

Comment on attachment 160734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160734&action=review I going to say r+, but I think that naming needs to be clarified. > Tools/ChangeLog:22 > + (simulateLegacyWebNotificationClickCallback): Renamed. Unsure if you are talking about "legacy web notifications", or a "legacy way to simulate working with them". From the ChangeLog it sounds like the latter, but function name shouts the former. > Tools/DumpRenderTree/TestRunner.h:421 > - bool m_areDesktopNotificationPermissionRequestsIgnored; > + bool m_areLegacyWebNotificationPermissionRequestsIgnored; I could have used a comment here explaining what "legacy" means in this context. Specifically, how will one determine that it's time to remove this code?

(In reply to comment #5) > (From update of attachment 160734 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160734&action=review > > I going to say r+, but I think that naming needs to be clarified. > > > Tools/ChangeLog:22 > > + (simulateLegacyWebNotificationClickCallback): Renamed. > > Unsure if you are talking about "legacy web notifications", or a "legacy way to simulate working with them". From the ChangeLog it sounds like the latter, but function name shouts the former. You're right. I mean to say the latter. I'll make a comment about this. I hope we can deprecate it soon enough when the tests get migrated to http/tests. > > > Tools/DumpRenderTree/TestRunner.h:421 > > - bool m_areDesktopNotificationPermissionRequestsIgnored; > > + bool m_areLegacyWebNotificationPermissionRequestsIgnored; > > I could have used a comment here explaining what "legacy" means in this context. Specifically, how will one determine that it's time to remove this code? Added.

Looks like in my fervor to check in 95099 (r126909) I ended up accidentally including this patch! I'll still check in another patch to add comments as ap suggested.

Committed r127018: <http://trac.webkit.org/changeset/127018>