[webkit-reviews] review denied: [Bug 63616] Add missing methods used by fast/notifications tests to LayoutTestController : [Attachment 99449] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 1 07:36:48 PDT 2011
Martin Robinson <mrobinson at webkit.org> has denied Alexandre Mazari
<scaroo at gmail.com>'s request for review:
Bug 63616: Add missing methods used by fast/notifications tests to
LayoutTestController
https://bugs.webkit.org/show_bug.cgi?id=63616
Attachment 99449: Patch
https://bugs.webkit.org/attachment.cgi?id=99449&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.
More information about the webkit-reviews
mailing list