Bug 51005 - Range::extractContents needs more tests
Summary: Range::extractContents needs more tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 51006
  Show dependency treegraph
 
Reported: 2010-12-13 21:08 PST by Ryosuke Niwa
Modified: 2010-12-14 19:07 PST (History)
3 users (show)

See Also:


Attachments
Patch (8.58 KB, patch)
2010-12-14 12:46 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2010-12-14 13:35 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
adds a test (10.04 KB, patch)
2010-12-14 15:24 PST, Ryosuke Niwa
darin: review+
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-12-13 21:08:40 PST
Currently, editing/execCommand/4920742-2.html is the only layout test that calls extractContents (as far as I checked on Finder).  However, the feature is far from trivial and requires much more rigorous testing.
Comment 1 Emil A Eklund 2010-12-14 12:46:04 PST
Created attachment 76559 [details]
Patch
Comment 2 Darin Adler 2010-12-14 13:02:01 PST
Comment on attachment 76559 [details]
Patch

It’s great to have more coverage.

However, textContent is an imprecise way to log the extracted fragment. These tests would be stronger if they used something that shows the details of the DOM nodes of the created fragment rather than just the concatenated text.

I think we have something for dumping markup that Ojan worked on a while back. Or even outerHTML might be better?
Comment 3 Emil A Eklund 2010-12-14 13:05:36 PST
That's a good idea, I'll see how other tests deal with it and rework it. Thanks.
Comment 4 Emil A Eklund 2010-12-14 13:35:01 PST
Created attachment 76565 [details]
Patch

Changed test to compare outerHTML instead of textContent.
Comment 5 Ryosuke Niwa 2010-12-14 14:00:18 PST
(In reply to comment #4)
> Created an attachment (id=76565) [details]
> Patch
> 
> Changed test to compare outerHTML instead of textContent.

We should just use dump-as-markup.js here.
Comment 6 Eric Seidel (no email) 2010-12-14 15:21:07 PST
Comment on attachment 76559 [details]
Patch

Cleared Darin Adler's review+ from obsolete attachment 76559 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 7 Ryosuke Niwa 2010-12-14 15:24:16 PST
Created attachment 76580 [details]
adds a test
Comment 8 Ryosuke Niwa 2010-12-14 19:03:37 PST
Thanks for the review, Darin.

(In reply to comment #7)
> Created an attachment (id=76580) [details]
> adds a test

Per discussion with Ojan, I'm adding a comment to dump-as-markup.js as in:
     // FIXME: Have this respect layoutTestController.dumpChildFramesAsText?
     // FIXME: Should we care about framesets?
-    var iframes = node.getElementsByTagName('iframe');
-    for (var i = 0; i < iframes.length; i++) {
-        markup += '\n\nFRAME ' + i + ':\n'
-        try {
-            markup += Markup.get(iframes[i].contentDocument.body.parentElement);
-        } catch (e) {
-            markup += 'FIXME: Add method to layout test controller to get access to cross-origin frames.';
+    // DocumentFragment doesn't have a getElementsByTagName method.
+    if (node.getElementsByTagName) {
+        var iframes = node.getElementsByTagName('iframe');
+        for (var i = 0; i < iframes.length; i++) {
+            markup += '\n\nFRAME ' + i + ':\n'
+            try {
+                markup += Markup.get(iframes[i].contentDocument.body.parentElement);
+            } catch (e) {
+                markup += 'FIXME: Add method to layout test controller to get access to cross-origin frames.';
+            }
Comment 9 Ryosuke Niwa 2010-12-14 19:06:37 PST
Committed r74094: <http://trac.webkit.org/changeset/74094>