RESOLVED FIXED 51005
Range::extractContents needs more tests
https://bugs.webkit.org/show_bug.cgi?id=51005
Summary Range::extractContents needs more tests
Ryosuke Niwa
Reported 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.
Attachments
Patch (8.58 KB, patch)
2010-12-14 12:46 PST, Emil A Eklund
no flags
Patch (9.35 KB, patch)
2010-12-14 13:35 PST, Emil A Eklund
no flags
adds a test (10.04 KB, patch)
2010-12-14 15:24 PST, Ryosuke Niwa
darin: review+
Emil A Eklund
Comment 1 2010-12-14 12:46:04 PST
Darin Adler
Comment 2 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?
Emil A Eklund
Comment 3 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.
Emil A Eklund
Comment 4 2010-12-14 13:35:01 PST
Created attachment 76565 [details] Patch Changed test to compare outerHTML instead of textContent.
Ryosuke Niwa
Comment 5 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.
Eric Seidel (no email)
Comment 6 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.
Ryosuke Niwa
Comment 7 2010-12-14 15:24:16 PST
Created attachment 76580 [details] adds a test
Ryosuke Niwa
Comment 8 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.'; + }
Ryosuke Niwa
Comment 9 2010-12-14 19:06:37 PST
Note You need to log in before you can comment on or make changes to this bug.