Bug 51005

Summary: Range::extractContents needs more tests
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Enhancement CC: darin, eae, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 51006    
Attachments:
Description Flags
Patch
none
Patch
none
adds a test darin: review+

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
DetailsFormatted DiffDiff
Patch (9.35 KB, patch)
2010-12-14 13:35 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
adds a test (10.04 KB, patch)
2010-12-14 15:24 PST, Ryosuke Niwa 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Emil A Eklund
Comment 1 2010-12-14 12:46:04 PST
Created attachment 76559 [details] Patch
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
Committed r74094: <http://trac.webkit.org/changeset/74094>
Note You need to log in before you can comment on or make changes to this bug.