[webkit-reviews] review denied: [Bug 59369] Convert LayoutTests/fast/events/mouseout-dead-node.html from pixel test to text-based test : [Attachment 91005] Remove Japanese char.

Tue Apr 26 00:07:18 PDT 2011

Daniel Bates <dbates at webkit.org> has denied Naoki Takano
<takano.naoki at gmail.com>'s request for review:
Bug 59369: Convert LayoutTests/fast/events/mouseout-dead-node.html from pixel
test to text-based test
https://bugs.webkit.org/show_bug.cgi?id=59369

Attachment 91005: Remove Japanese char.
https://bugs.webkit.org/attachment.cgi?id=91005&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91005&action=review

We can further improve this patch.

I suggest we use the LayoutTests/fast/js/resources/js-test-pre.js and its
testPassed(), testFailed() functions instead of out(). See
<http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.htm
l> for an example of using js-test-pre.js.

Also, the paths in the this patch aren't from the the top-level project
directory. It looks like the patch was created from within the LayoutTests
directory. As you can extrapolate from the EWS bot output (e.g.
<https://webkit-commit-queue.appspot.com/results/8505750>), the commit queue is
not smart enough to apply such a patch. So, it would need to be landed by hand.
Instead, I suggest you generate your patch from the top-level WebKit directory.

> fast/events/mouseout-dead-node-expected.txt:3
> +you should see PASS below
> +you should see PASS below

Can we remove this text when this test is run using Dump Render Tree so as to
simplify the result output?

See
<http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.htm
l> for an example of this. In particular, see
<http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.htm
l#L181> where we remove the test container so that we only leave the test
description and test results in the output.

> fast/events/mouseout-dead-node.html:1
> -<body onload='test()'>
> +<body onload='autoTest()'>

No need to rename this. This name of the function is sufficiently descriptive
as-is.

> fast/events/mouseout-dead-node.html:-26
> -    if (window.eventSender) {

We shouldn't change this to layoutTestController since the code within this
block uses aspects of the EventSender object. Although, in practice the
existence of the EventSender object can be seen as equivalent to the presence
of the LayoutTestController object ;=> we are running the test under Dump
Render Tree, I suggest we instead only use LayoutTestController and EventSender
functionality when we know such objects exist, respectively. For example, we
should check for the presence of the EventSender object before using
eventSender.mouseMoveTo().

> fast/events/mouseout-dead-node.html:26
> +    if (window.layoutTestController) {
> +	   layoutTestController.dumpAsText();

I would hoist this if-statement outside of this function so that it's at the
top of the <script> body.