Bug 59369 - Convert LayoutTests/fast/events/mouseout-dead-node.html from pixel test to text-based test
Summary: Convert LayoutTests/fast/events/mouseout-dead-node.html from pixel test to te...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-25 16:01 PDT by Naoki Takano
Modified: 2011-04-28 00:15 PDT (History)
6 users (show)

See Also:


Attachments
Converted to use dumpAsText() (1.36 KB, patch)
2011-04-25 16:02 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Add ChangeLog and remove unneeded files. (10.11 KB, patch)
2011-04-25 16:30 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Remove Japanese char. (9.84 KB, patch)
2011-04-25 16:34 PDT, Naoki Takano
dbates: review-
Details | Formatted Diff | Diff
According to the review, change the test and update the expected result. (12.55 KB, patch)
2011-04-26 15:57 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (64.81 KB, patch)
2011-04-26 22:59 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (64.68 KB, patch)
2011-04-27 19:01 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (64.69 KB, patch)
2011-04-27 19:15 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (64.69 KB, patch)
2011-04-27 20:00 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Naoki Takano 2011-04-25 16:01:35 PDT
Convert to text based test.
Comment 1 Naoki Takano 2011-04-25 16:02:41 PDT
Created attachment 90975 [details]
Converted to use dumpAsText()

Please review.
Comment 2 Naoki Takano 2011-04-25 16:09:18 PDT
Oops, forgot to write ChangeLog...
Comment 3 Naoki Takano 2011-04-25 16:30:32 PDT
Created attachment 91004 [details]
Add ChangeLog and remove unneeded files.

Please review.
Comment 4 Naoki Takano 2011-04-25 16:34:17 PDT
Created attachment 91005 [details]
Remove Japanese char.

Removed Japanese chars.
Comment 5 Daniel Bates 2011-04-26 00:07:18 PDT
Comment on attachment 91005 [details]
Remove Japanese char.

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.html> 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.html> for an example of this. In particular, see <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.html#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.
Comment 6 Naoki Takano 2011-04-26 09:30:53 PDT
Thank you for your review, Daniel.

I'll do that.

(In reply to comment #5)
> (From update of attachment 91005 [details])
> 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.html> 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.html> for an example of this. In particular, see <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.html#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.
Comment 7 Naoki Takano 2011-04-26 15:57:40 PDT
Created attachment 91175 [details]
According to the review, change the test and update the expected result.

Please review again.
Comment 8 Daniel Bates 2011-04-26 16:03:47 PDT
Comment on attachment 91175 [details]
According to the review, change the test and update the expected result.

Looks good. Thanks Naoki for the fix!
Comment 9 Naoki Takano 2011-04-26 16:08:05 PDT
Comment on attachment 91175 [details]
According to the review, change the test and update the expected result.

Could you commit?

I'm not yet committer;-)
Comment 10 WebKit Commit Bot 2011-04-26 22:15:47 PDT
Comment on attachment 91175 [details]
According to the review, change the test and update the expected result.

Rejecting attachment 91175 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2

Last 500 characters of output:
hing file LayoutTests/platform/chromium-win/fast/events/mouseout-dead-node-expected.txt
rm 'LayoutTests/platform/chromium-win/fast/events/mouseout-dead-node-expected.txt'
patching file LayoutTests/fast/events/mouseout-dead-node-expected.txt
patching file LayoutTests/fast/events/mouseout-dead-node.html
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Daniel Bates', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8514095
Comment 11 Naoki Takano 2011-04-26 22:59:38 PDT
Created attachment 91235 [details]
Patch
Comment 12 Naoki Takano 2011-04-26 23:03:24 PDT
Comment on attachment 91235 [details]
Patch

Hmm...

It looks applying patch failed.

I made the previous patch with svn manually.
The reason of the failure was to try to remove checksum files.

So I make new patch again with webkit-patch without those files.

Please review and commit again.

Please commit again.
Comment 13 Ryosuke Niwa 2011-04-27 12:42:19 PDT
Comment on attachment 91235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91235&action=review

> LayoutTests/fast/events/mouseout-dead-node.html:1
> +<link rel="stylesheet" href="../js/resources/js-test-style.css">

No DOCTYPE / head?

> LayoutTests/fast/events/mouseout-dead-node.html:4
>  <body onload='test()'>

I don't think there's any need to wait until page load.

> LayoutTests/fast/events/mouseout-dead-node.html:15
> +    <div id=d0 style='border:2px solid red'>
> +        <div onmouseout='testFailed("mouseout")'  onmouseover='document.getElementById("d0").innerHTML ="you should see PASS below"'>
> +            <div onmouseout='testFailed("mouseout")'>
> +                <span id=target1 onmouseout='testPassed("mouseout")' >
> +                    mouse over me
> +                </span>
> +            </div>

I don't think we need to indent each element like this.

> LayoutTests/fast/events/mouseout-dead-node.html:32
>  function test() {

You don't need this function. You can just run the script as it's parsed.

> LayoutTests/fast/events/mouseout-dead-node.html:54
> +description("Test that if node dies under mouse it receives mouseout event but that the event does not propagate.");

Please move this above the test code so that people reading the test code can understand the intent of this test before reading the code.
Comment 14 Naoki Takano 2011-04-27 12:47:29 PDT
Thank you for your review every time!!

(In reply to comment #13)
> (From update of attachment 91235 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91235&action=review
> 
> > LayoutTests/fast/events/mouseout-dead-node.html:1
> > +<link rel="stylesheet" href="../js/resources/js-test-style.css">
> 
> No DOCTYPE / head?
I'll add it.
 
> > LayoutTests/fast/events/mouseout-dead-node.html:4
> >  <body onload='test()'>
> 
> I don't think there's any need to wait until page load.
> 
> > LayoutTests/fast/events/mouseout-dead-node.html:15
> > +    <div id=d0 style='border:2px solid red'>
> > +        <div onmouseout='testFailed("mouseout")'  onmouseover='document.getElementById("d0").innerHTML ="you should see PASS below"'>
> > +            <div onmouseout='testFailed("mouseout")'>
> > +                <span id=target1 onmouseout='testPassed("mouseout")' >
> > +                    mouse over me
> > +                </span>
> > +            </div>
> 
> I don't think we need to indent each element like this.
Ok, so do I have to erase whole indent for html element?

> > LayoutTests/fast/events/mouseout-dead-node.html:32
> >  function test() {
> 
> You don't need this function. You can just run the script as it's parsed.
I will erase onload.

> > LayoutTests/fast/events/mouseout-dead-node.html:54
> > +description("Test that if node dies under mouse it receives mouseout event but that the event does not propagate.");
> 
> Please move this above the test code so that people reading the test code can understand the intent of this test before reading the code.
Sure.
Comment 15 Naoki Takano 2011-04-27 19:01:46 PDT
Created attachment 91407 [details]
Patch
Comment 16 Naoki Takano 2011-04-27 19:02:49 PDT
Comment on attachment 91407 [details]
Patch

Could you review again?

Thanks,
Comment 17 Ryosuke Niwa 2011-04-27 19:07:58 PDT
Comment on attachment 91407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91407&action=review

> LayoutTests/fast/events/mouseout-dead-node.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use HTML5 style <!DOCTYPE html>

> LayoutTests/fast/events/mouseout-dead-node.html:11
> +<div onmouseout='testFailed("mouseout")'  onmouseover='document.getElementById("d0").innerHTML ="you should see PASS below"'>

I would have done: this.parent.innerHTML="~~".

> LayoutTests/fast/events/mouseout-dead-node.html:33
> +if (!window.eventSender)
> +    return;

This won't work. I guess you'll have to put everything in an if clause.
Comment 18 Naoki Takano 2011-04-27 19:15:42 PDT
Created attachment 91408 [details]
Patch
Comment 19 Naoki Takano 2011-04-27 19:17:27 PDT
Comment on attachment 91408 [details]
Patch

Actually this.parent.innerText="" doesn't work.

Except that, I follow your suggestion.

Please review again.

Thanks,
Comment 20 WebKit Review Bot 2011-04-27 19:18:05 PDT
Attachment 91408 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Updating OpenSource
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 146.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Naoki Takano 2011-04-27 20:00:12 PDT
Created attachment 91412 [details]
Patch
Comment 22 Naoki Takano 2011-04-27 20:00:48 PDT
Comment on attachment 91412 [details]
Patch

Strange error happened.

So upload my patch again.
Comment 23 WebKit Commit Bot 2011-04-27 23:15:55 PDT
Comment on attachment 91412 [details]
Patch

Clearing flags on attachment: 91412

Committed r85163: <http://trac.webkit.org/changeset/85163>
Comment 24 WebKit Commit Bot 2011-04-27 23:16:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2011-04-28 00:15:01 PDT
http://trac.webkit.org/changeset/85163 might have broken Qt Linux Release
The following tests are not passing:
fast/events/mouseout-dead-node.html