Bug 42608 - dumpAsMarkup test conversion: create-list-from-range-selection.html and insert-list-empty-div.html
Summary: dumpAsMarkup test conversion: create-list-from-range-selection.html and inser...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Blocker
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 42436
  Show dependency treegraph
 
Reported: 2010-07-19 17:42 PDT by Ryosuke Niwa
Modified: 2010-07-21 21:03 PDT (History)
3 users (show)

See Also:


Attachments
converts the tests (37.49 KB, patch)
2010-07-19 18:08 PDT, Ryosuke Niwa
ojan: review-
ojan: commit-queue-
Details | Formatted Diff | Diff
removed create-list-with-hr.html and fixed per ojan's comments (27.09 KB, patch)
2010-07-19 19:03 PDT, Ryosuke Niwa
ojan: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-07-19 17:42:33 PDT
create-list-from-range-selection.html, create-list-with-hr.html, and insert-list-empty-div.html currently uses render test but they should all be using dumpAsMarkup test since what we need to know is the final DOM tree and the selection.
Comment 1 Ryosuke Niwa 2010-07-19 18:08:55 PDT
Created attachment 62019 [details]
converts the tests
Comment 2 Ryosuke Niwa 2010-07-19 18:36:11 PDT
Will not convert create-list-with-hr.html since it demonstrates a rendering bug.
Comment 3 Ojan Vafai 2010-07-19 18:37:48 PDT
Comment on attachment 62019 [details]
converts the tests

> Index: LayoutTests/resources/dump-as-markup.js
> ===================================================================
> --- LayoutTests/resources/dump-as-markup.js	(revision 63331)
> +++ LayoutTests/resources/dump-as-markup.js	(working copy)
> @@ -17,7 +17,7 @@ Markup.dump = function(opt_node)

> +Markup.setNode = function(node)

Can we call this setNodeToDump so it's more clear what it does? 

> +{
> +  if (typeof node == "string")
> +    node = document.getElementById(node);
> +  if (node instanceof Node)

I don't think you need this if-check. If node is not a Node here, we should error out.

> +    Markup.node = node

This should be private, i.e., Markup._node.
Comment 4 Ryosuke Niwa 2010-07-19 19:03:09 PDT
Created attachment 62027 [details]
removed create-list-with-hr.html and fixed per ojan's comments
Comment 5 Ojan Vafai 2010-07-20 16:05:34 PDT
Comment on attachment 62027 [details]
removed create-list-with-hr.html and fixed per ojan's comments

Wow. It's so much easier to read these expected results!
Comment 6 WebKit Commit Bot 2010-07-20 17:50:19 PDT
Comment on attachment 62027 [details]
removed create-list-with-hr.html and fixed per ojan's comments

Rejecting patch 62027 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Ojan Vafai', u'--force']" exit_code: 1
Last 500 characters of output:
ommand/create-list-from-range-selection-expected.txt'
patching file LayoutTests/platform/qt/editing/execCommand/insert-list-empty-div-expected.txt
rm 'LayoutTests/platform/qt/editing/execCommand/insert-list-empty-div-expected.txt'
patching file LayoutTests/resources/dump-as-markup.js
Hunk #2 FAILED at 31.
Hunk #3 succeeded at 65 with fuzz 1 (offset 10 lines).
Hunk #4 succeeded at 237 (offset 12 lines).
1 out of 4 hunks FAILED -- saving rejects to file LayoutTests/resources/dump-as-markup.js.rej

Full output: http://queues.webkit.org/results/3395665
Comment 7 Ryosuke Niwa 2010-07-21 21:03:11 PDT
Landed as http://trac.webkit.org/changeset/63873.