[webkit-reviews] review denied: [Bug 56401] hover then un-hover makes state change : [Attachment 97761] Patch

Mon Jul 4 23:15:16 PDT 2011

MORITA Hajime <morrita at google.com> has denied Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 56401: hover then un-hover makes state change
https://bugs.webkit.org/show_bug.cgi?id=56401

Attachment 97761: Patch
https://bugs.webkit.org/attachment.cgi?id=97761&action=review

------- Additional Comments from MORITA Hajime <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=97761&action=review

Thank you for doing this!
Unfortunately I'm not an expert in this area. So I'd like to have some prep
review before calling the experts ;-)
That being said, the test strategy itself looks fine.

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:1
> +<!DOCTYPE html>

We need <html>

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:9
> +	       function getElementTop(elementId) {

Webkit tests usually have one <script> block.
You can use layoutTestcontroller.waitUntilDone() and notifyDone() for keep DRT
running during the exercise.

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:26
> +	       if (window.layoutTestController) {

Could you make this test runnable without layoutTestController?
WebKit devs prefer to keep tests runnable in Safari (or other browsers) as
possible. 

An idiom is:
if (!window.layoutTestController)
    return;
inside a test function.

> Source/WebCore/ChangeLog:8
> +	   https://bugs.webkit.org/show_bug.cgi?id=56401

Please follow the standard changelog format.
- You need a bug summary line before the URL line
- The detailed description should come after the URL.