[webkit-reviews] review denied: [Bug 57755] REGRESSION(r81328): Null pointer crash in canAppendNewLineFeed when selection isn't inside an editable element : [Attachment 88353] Patch

Tue Apr 5 23:22:20 PDT 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Naoki Takano
<takano.naoki at gmail.com>'s request for review:
Bug 57755: REGRESSION(r81328): Null pointer crash in canAppendNewLineFeed when
selection isn't inside an editable element
https://bugs.webkit.org/show_bug.cgi?id=57755

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88353&action=review

> LayoutTests/editing/execCommand/insert-line-break-onload.html:1
> +<body onload="go()">

Missing DOCTYPE.

> LayoutTests/editing/execCommand/insert-line-break-onload.html:2
> +Test InsertLineBreak is called correctly without any exception.

Mn... I'd expect that adding this line would stop reproducing the crash.  Did
you make sure this test case cause a crash?

> LayoutTests/editing/execCommand/insert-line-break-onload.html:10
> +    var result = "Calling InsertLineBreak succeeded without any exception.";

I'd just print 'PASS' with the test description because that's what other tests
do.

> LayoutTests/editing/execCommand/insert-line-break-onload.html:15
> +    } else
> +	   alert(result);

Why do you need an alert here?	I'd expect for document.write to work here as
well.

> Source/WebCore/editing/TypingCommand.cpp:52
> +    if (Node* node = selection.rootEditableElement()) {

We normally do an early exit in these cases.