Bug 95093 – Update TestRunner API for web notifications

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jon Lee]

Remove some unused legacy API, add some new API, and rename the functions from "desktop notifications" to "web notifications", since that is the official name of the spec.

[Radar WebKit Bug Importer]

<rdar://problem/12179649>

[Jon Lee]

Created attachment 160734 [details]
Patch

[Eric Seidel (no email)]

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

[Jon Lee]

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

[Alexey Proskuryakov]

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?

[Jon Lee]

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

[Jon Lee]

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.

[Jon Lee]

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