Created attachment 146636 [details] Patch Found by <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/Node-replaceChild.html>. Can either of you turn this patch into something acceptable?
Created attachment 146952 [details] Patch
Comment on attachment 146952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146952&action=review > Source/WebCore/ChangeLog:8 > + Covered by existing tests with some modifications. Please explain how this bug manifested and how you fixed it. r- due to the lack of description. Also, please add URLs of relevant specifications. > LayoutTests/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description but below the bug URL.
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.
Created attachment 146981 [details] Patch Made
(Err. Missed some quotes in the shell there...) Made the requested changes and rewrote that test to be much shorter and use the shouldThrow/shouldNotThrow functions.
Comment on attachment 146981 [details] Patch Wow, that's 100x more readable now.
Comment on attachment 146981 [details] Patch Clearing flags on attachment: 146981 Committed r120037: <http://trac.webkit.org/changeset/120037>
All reviewed patches have been landed. Closing bug.