Bug 63616 - Add missing methods used by fast/notifications tests to LayoutTestController
Summary: Add missing methods used by fast/notifications tests to LayoutTestController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xan Lopez
URL:
Keywords:
Depends on:
Blocks: 66477
  Show dependency treegraph
 
Reported: 2011-06-29 02:53 PDT by Alexandre Mazari
Modified: 2011-12-04 03:56 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.67 KB, patch)
2011-06-29 03:35 PDT, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (9.86 KB, patch)
2011-07-01 02:58 PDT, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (13.50 KB, patch)
2011-08-18 09:42 PDT, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (11.59 KB, patch)
2011-09-27 01:20 PDT, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (11.69 KB, patch)
2011-10-27 07:59 PDT, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (9.48 KB, patch)
2011-11-09 11:21 PST, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (9.68 KB, patch)
2011-11-10 02:36 PST, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (10.17 KB, patch)
2011-12-04 03:34 PST, Xan Lopez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Mazari 2011-06-29 02:53:52 PDT
Add missing methods used by fast/notifications tests to LayoutTestController
Comment 1 Alexandre Mazari 2011-06-29 03:35:29 PDT
Created attachment 99065 [details]
Patch
Comment 2 Alexandre Mazari 2011-07-01 02:58:22 PDT
Created attachment 99449 [details]
Patch

Added ChangeLog entry
Comment 3 WebKit Review Bot 2011-07-01 03:01:27 PDT
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.
Comment 4 Martin Robinson 2011-07-01 07:36:49 PDT
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.
Comment 5 Alexandre Mazari 2011-08-18 09:42:36 PDT
Created attachment 104351 [details]
Patch

updated: do not provide default implementation for LTC notifications API
Comment 6 Eric Seidel (no email) 2011-09-12 17:05:17 PDT
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?
Comment 7 Philippe Normand 2011-09-23 06:14:34 PDT
Ping, Alex.
Comment 8 Alexandre Mazari 2011-09-27 01:20:02 PDT
Created attachment 108808 [details]
Patch
Comment 9 Philippe Normand 2011-09-27 09:30:57 PDT
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.
Comment 10 Martin Robinson 2011-09-27 09:39:13 PDT
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?
Comment 11 Martin Robinson 2011-09-27 09:40:38 PDT
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.
Comment 12 Alexandre Mazari 2011-10-27 07:59:22 PDT
Created attachment 112682 [details]
Patch
Comment 13 Martin Robinson 2011-10-27 08:37:11 PDT
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?
Comment 14 Philippe Normand 2011-11-03 08:15:02 PDT
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?
Comment 15 Alexandre Mazari 2011-11-03 08:48:30 PDT
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 :)
Comment 16 Alexandre Mazari 2011-11-09 11:21:14 PST
Created attachment 114323 [details]
Patch
Comment 17 Martin Robinson 2011-11-09 12:47:56 PST
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.
Comment 18 Alexandre Mazari 2011-11-10 02:36:04 PST
Created attachment 114460 [details]
Patch
Comment 19 Alexandre Mazari 2011-11-10 02:42:22 PST
(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
Comment 20 Martin Robinson 2011-11-19 18:19:46 PST
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!
Comment 21 Xan Lopez 2011-12-04 03:34:20 PST
Created attachment 117788 [details]
Patch
Comment 22 WebKit Review Bot 2011-12-04 03:56:37 PST
Comment on attachment 117788 [details]
Patch

Clearing flags on attachment: 117788

Committed r101953: <http://trac.webkit.org/changeset/101953>
Comment 23 WebKit Review Bot 2011-12-04 03:56:43 PST
All reviewed patches have been landed.  Closing bug.