[webkit-reviews] review granted: [Bug 67767] Crashed in WebCore::AppendNodeCommand::create(WTF::PassRefPtr<WebCore::ContainerNode>, WTF::PassRefPtr<WebCore::Node>) : [Attachment 106728] Patch

Thu Sep 8 08:39:21 PDT 2011

Darin Adler <darin at apple.com> has granted Shinya Kawanaka
<shinyak at google.com>'s request for review:
Bug 67767: Crashed in
WebCore::AppendNodeCommand::create(WTF::PassRefPtr<WebCore::ContainerNode>,
WTF::PassRefPtr<WebCore::Node>)
https://bugs.webkit.org/show_bug.cgi?id=67767

Attachment 106728: Patch
https://bugs.webkit.org/attachment.cgi?id=106728&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106728&action=review

This looks OK. It's a little strange to just do nothing if the node is not an
element and the patch would be better if it explained why that is the correct
thing to do, not just that it won’t crash.

> LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:9
> +    document.writeln("This test ensures WebKit does not
crash.<br><br>PASS");

The wording here is wrong. The test does not ensure that WebKit does not crash.
The test has no effect on WebKit.

It's good to say that the test has passed if it doesn’t crash, and also
probably good to say as clearly as possible why there was a time when the test
would crash.

> LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:12
> +<meta content="2"/><body onLoad="runTest();">

What is the significance of this "meta" element?