Bug 59369

Summary:

Convert LayoutTests/fast/events/mouseout-dead-node.html from pixel test to text-based test

Product:

WebKit

Reporter:

Naoki Takano <honten>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, dbates, eric, rniwa, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Converted to use dumpAsText()	none
Add ChangeLog and remove unneeded files.	none
Remove Japanese char.	dbates: review-
According to the review, change the test and update the expected result.	none
Patch	none
Patch	none
Patch	none
Patch	none

Naoki Takano

Reported 2011-04-25 16:01:35 PDT

Convert to text based test.

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Naoki Takano

Comment 1 2011-04-25 16:02:41 PDT

Created attachment 90975 [details] Converted to use dumpAsText() Please review.

Naoki Takano

Comment 2 2011-04-25 16:09:18 PDT

Oops, forgot to write ChangeLog...

Naoki Takano

Comment 3 2011-04-25 16:30:32 PDT

Created attachment 91004 [details] Add ChangeLog and remove unneeded files. Please review.

Naoki Takano

Comment 4 2011-04-25 16:34:17 PDT

Created attachment 91005 [details] Remove Japanese char. Removed Japanese chars.

Daniel Bates

Comment 5 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.

Naoki Takano

Comment 6 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.

Naoki Takano

Comment 7 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.

Daniel Bates

Comment 8 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!

Naoki Takano

Comment 9 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;-)

WebKit Commit Bot

Comment 10 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

Naoki Takano

Comment 11 2011-04-26 22:59:38 PDT

Created attachment 91235 [details] Patch

Naoki Takano

Comment 12 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.

Ryosuke Niwa

Comment 13 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.

Naoki Takano

Comment 14 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.

Naoki Takano

Comment 15 2011-04-27 19:01:46 PDT

Created attachment 91407 [details] Patch

Naoki Takano

Comment 16 2011-04-27 19:02:49 PDT

Comment on attachment 91407 [details] Patch Could you review again? Thanks,

Ryosuke Niwa

Comment 17 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.

Naoki Takano

Comment 18 2011-04-27 19:15:42 PDT

Created attachment 91408 [details] Patch

Naoki Takano

Comment 19 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,

WebKit Review Bot

Comment 20 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.

Naoki Takano

Comment 21 2011-04-27 20:00:12 PDT

Created attachment 91412 [details] Patch

Naoki Takano

Comment 22 2011-04-27 20:00:48 PDT

Comment on attachment 91412 [details] Patch Strange error happened. So upload my patch again.

WebKit Commit Bot

Comment 23 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>

WebKit Commit Bot

Comment 24 2011-04-27 23:16:02 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 25 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

Note You need to log in before you can comment on or make changes to this bug.