[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