Bug 63616 – Add missing methods used by fast/notifications tests to LayoutTestController

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexandre Mazari]

Add missing methods used by fast/notifications tests to LayoutTestController

[Alexandre Mazari]

Created attachment 99065 [details]
Patch

[Alexandre Mazari]

Created attachment 99449 [details]
Patch

Added ChangeLog entry

[WebKit Review Bot]

Attachment 99449 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/L..." exit_code: 1

Tools/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 99449 [details]
Patch

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

This looks pretty good, but please fix the issues I mention below.

> Tools/ChangeLog:8
> +        fast/notifications and http/tests/notifications uses LayoutTestController methods that only the chromium and qt port implemented. This patch add those missing methods to the common interface and provides empty implementations for the port-specific code.
> +        https://bugs.webkit.org/show_bug.cgi?id=63616

The ChangeLog generation is a bit wacky at the moment, I think. Please take a look at the other ChangeLogs in the file and ensure that this ChangeLog follows the same format. I think the fix description usually follows the bug link. Please also shorten you line length to match the rest of the ChangeLog.

>> Tools/ChangeLog:12
>> +	(LayoutTestController::LayoutTestController): initialize notifications permission requests ignoring to false.
> 
> Line contains tab character.  [whitespace/tab] [5]

Please fix!

> Tools/DumpRenderTree/LayoutTestController.cpp:91
> +    , m_expectedPixelHash(expectedPixelHash),
> +      m_areDesktopNotificationPermissionsIgnored(false)

The comma should be at the start of the line here. Very screwy, but that's how we do it. :)

For a boolean you don't need to have the are and the name can just be m_ignoreDesktopNotificationPermissionRequests.

[Alexandre Mazari]

Created attachment 104351 [details]
Patch

updated: do not provide default implementation for LTC notifications API

[Eric Seidel (no email)]

Comment on attachment 104351 [details]
Patch

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

> Source/WebCore/bindings/js/JSDesktopNotificationsCustom.cpp:66
> +      impl()->requestPermission(0);

Indent.

> Tools/DumpRenderTree/LayoutTestController.cpp:2196
> +     LayoutTestController* controller = static_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject));
> +     controller->ignoreDesktopNotificationPermissionRequests();
> +     return JSValueMakeUndefined(context);

Strange indent?

[Philippe Normand]

Ping, Alex.

[Alexandre Mazari]

Created attachment 108808 [details]
Patch

[Philippe Normand]

Comment on attachment 108808 [details]
Patch

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

Looks good, just a small issue to fix.

> Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:51
> +#include <wtf/text/CString.h>

I think this include is not necessary.

[Martin Robinson]

Comment on attachment 108808 [details]
Patch

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

> Tools/DumpRenderTree/LayoutTestController.h:145
> +    bool areDesktopNotificationPermissionRequestsIgnored();

This seems to be unused now?

[Martin Robinson]

Comment on attachment 108808 [details]
Patch

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

> Tools/DumpRenderTree/LayoutTestController.cpp:-2496
> -void LayoutTestController::grantDesktopNotificationPermission(JSStringRef origin)
> -{
> -    m_desktopNotificationAllowedOrigins.push_back(JSStringRetain(origin));
> -}
> -
> -bool LayoutTestController::checkDesktopNotificationPermission(JSStringRef origin)
> -{
> -    std::vector<JSStringRef>::iterator i;
> -    for (i = m_desktopNotificationAllowedOrigins.begin();
> -         i != m_desktopNotificationAllowedOrigins.end();
> -         ++i) {
> -        if (JSStringIsEqual(*i, origin))
> -            return true;
> -    }
> -    return false;
> -}
> -

It might be good to explain in the ChangeLog why you remove the platform-independent implementation for these methods.

[Alexandre Mazari]

Created attachment 112682 [details]
Patch

[Martin Robinson]

Comment on attachment 112682 [details]
Patch

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

> Tools/ChangeLog:12
> +        needs to be modified from DRT in order to pass
> +        fast/notifications tests, at least in GTK case.
> +        Also the current codepath wasn't used AFAIK.

I'm pretty sure that you can call into the LayoutTestController from the DRT.  Wouldn't it be better to keep the code cross platform and just expose an interface for mucking with it?

[Philippe Normand]

Comment on attachment 112682 [details]
Patch

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

Please correct me if I'm wrong but checkDesktopNotificationPermission() and areDesktopNotificationPermissionRequestsIgnored() are not needed at all.

> Tools/DumpRenderTree/LayoutTestController.h:143
>      bool checkDesktopNotificationPermission(JSStringRef origin);

This doesn't seem to be used by any test. Might be worth just removing it all together from the LayoutTestController.

> Tools/DumpRenderTree/LayoutTestController.h:145
> +    bool areDesktopNotificationPermissionRequestsIgnored();

This doesn't seem to be used by any LayoutTest. Why adding it?

[Alexandre Mazari]

Philippe, Martin,

Thanks for the reviews!

The current form of this patch is obsolete. As suggested by martin, I wont remove the existing code dealing with the list of authorized origins.
The current approach was to delegate it to DumpRenderTreeSupportGtk in order to share that state between LTC and the gtk notifications api signals handlers.
It is a bit twisted and Philippe gave me the solution about the ENABLE macro within the Tools/DumpRenderTree/gtk tree.

So I am planning to:
- not remove the list of authorized origins and related methods from LTC
- add the list of pending requests in LTC with related methods
- use those from notifications-related callback in GTK's DumpRenderTree

After that remains the case of simulateDesktopNotificationClick (JString) used by
fast/notifications/notifications-click-event.html

As its name suggests, it is used to simulate a click event on the native representation of the notification named after the passed string.
Obviously, LayoutTestController must have access to the list of notifications that is managed by DRT testing code. I have no idea on how to achieve that apart from delegating to DRTSupportGtk which is both accessible from LayouTestControllerGtk and gtk's DRT. I'd love to be proven wrong :)

[Alexandre Mazari]

Created attachment 114323 [details]
Patch

[Martin Robinson]

Comment on attachment 114323 [details]
Patch

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

This looks good to me with one small change. Other than that, I would r+ this patch.

> Tools/DumpRenderTree/LayoutTestController.h:146
> +    void simulateDesktopNotificationClick(JSStringRef);

You should include "title" here since JSStringRef is a generic type.

[Alexandre Mazari]

Created attachment 114460 [details]
Patch

[Alexandre Mazari]

(In reply to comment #18)
> Created an attachment (id=114460) [details]
> Patch

Updated:
- named simulate... argument as 'title'
- Made git log and ChangeLog entry clearer and describing the current state of the patch

[Martin Robinson]

Comment on attachment 114460 [details]
Patch

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

This looks fine to me, but we cannot use the cq here since there's a merge boundary still in the patch. If you re-upload without that I can cq it.

> Tools/DumpRenderTree/wx/LayoutTestControllerWx.cpp:649
> +<<<<<<< HEAD

Looks like this snuck in!

[Xan Lopez]

Created attachment 117788 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117788 [details]
Patch

Clearing flags on attachment: 117788

Committed r101953: <http://trac.webkit.org/changeset/101953>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.