Add missing methods used by fast/notifications tests to LayoutTestController
Created attachment 99065 [details] Patch
Created attachment 99449 [details] Patch Added ChangeLog entry
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 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.
Created attachment 104351 [details] Patch updated: do not provide default implementation for LTC notifications API
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?
Ping, Alex.
Created attachment 108808 [details] Patch
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 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 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.
Created attachment 112682 [details] Patch
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 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?
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 :)
Created attachment 114323 [details] Patch
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.
Created attachment 114460 [details] Patch
(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 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!
Created attachment 117788 [details] Patch
Comment on attachment 117788 [details] Patch Clearing flags on attachment: 117788 Committed r101953: <http://trac.webkit.org/changeset/101953>
All reviewed patches have been landed. Closing bug.