[Webkit-unassigned] [Bug 88681] WebKit doesn't allow replacing the document element with a DocumentFragment containing one element

Mon Jun 11 16:35:04 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=88681

--- Comment #3 from Ojan Vafai <ojan at chromium.org>  2012-06-11 16:35:03 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=146952&action=review

> Source/WebCore/ChangeLog:8
> +        Covered by existing tests with some modifications.

No need to spell this out if the test is clearly testing the new code. You only need to justify leaving out tests if it's not obvious that existing tests cover the code (e.g. if no tests output changes or if you're making a big change where the changed test output only seems to cover part of the change).

> Source/WebCore/ChangeLog:11
> +        (WebCore::Document::canReplaceChild):

Can use a line describing the fix, e.g. "When the new child was a DocumentFragment we were incorrectly iterating over the Document's children instead of the DocumentFragment's children."

Part of the point of these change descriptions is so people can quickly decide if they want to take the time to look at the patch in detail, so even though that's obvious by a quick look at the patch, the change description is useful.

> LayoutTests/fast/dom/Document/replace-child-expected.txt:1
> +ALERT: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

I think this is out of date from what you uploaded.

> LayoutTests/fast/dom/Document/replace-child.html:41
> +        debug('replacing element with element in fragment')
> +        try {
> +            var doc = parser.parseFromString('<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"><foo/>', "text/xml")
> +            var fragment = doc.createDocumentFragment();
> +            fragment.appendChild(doc.createElement('bar'));
> +            doc.replaceChild(fragment, doc.documentElement);
> +        } catch (ex) {
> +            debug('FAILED: ' + ex)
> +            return;
> +        }
> +        debug('SUCCESS: ' + serializer.serializeToString(doc));
> +        successCount++;

Can you use shouldNotThrow from js-test-pre.js here and shouldThrow below? Then you wouldn't need the successCount stuff either.

I know the test is already doing this, but the test is dumb. :)

> LayoutTests/fast/dom/Document/replace-child.html:47
> +            var doc = parser.parseFromString('<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"><foo/>', "text/xml")

Here and above, can you just use the HTML doctype of <!DOCTYPE html>. I don't see any benefit from using this long doctype just to be consistent with the rest of the uses in this test. Feel free to update the other cases in this test too if you want.

> LayoutTests/fast/dom/Document/replace-child.html:50
> +            fragment.appendChild(doc.createElement('bar'));
> +            fragment.appendChild(doc.createElement('baz'));

It's really weird to me that this test uses unknown tagnames, but I suppose it's fine given that the other test-cases here do that.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.