[webkit-reviews] review denied: [Bug 207748] Null Ptr Deref @ WebCore::Node::Treescope : [Attachment 390735] Patch

Fri Feb 14 00:50:34 PST 2020

Ryosuke Niwa <rniwa at webkit.org> has denied Pinki Gyanchandani
<pgyanchandani at apple.com>'s request for review:
Bug 207748: Null Ptr Deref @ WebCore::Node::Treescope
https://bugs.webkit.org/show_bug.cgi?id=207748

Attachment 390735: Patch

https://bugs.webkit.org/attachment.cgi?id=390735&action=review

--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 390735
  --> https://bugs.webkit.org/attachment.cgi?id=390735
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390735&action=review

r- due to the issues listed below.

> Source/WebCore/ChangeLog:6
> +	   Reviewed by Ryosuke Niwa. 

This is definitely not reviewed by me yet. Please undo this and say NOBODY
(OOPS!).

> Source/WebCore/html/HTMLTextFormControlElement.cpp:307
> +	   if (!isTextField())
> +	       return;

Let's check this before getting innerText. It's weird to try to update
innerText if it's not a text field

> LayoutTests/ChangeLog:6
> +	   Reviewed by Ryosuke Niwa NOBODY.

Ditto. This is definitely not reviewed by me.

In fact, since I wrote this case, it's not appropriate for me to review this
test.

> LayoutTests/ChangeLog:8
> +	   Added a regression test to verify the fix

Missing a period at the end. Also, we need to give the test author (me in this
case) a credit in this case.
Say something like: "Added a regression test to verify the fix based on a test
case written by Ryosuke Niwa".

>
LayoutTests/editing/selection/ignore-selection-range-on-input-style-change.html
:1
> +<html>

Missing <!DOCTYPE html>.

>
LayoutTests/editing/selection/ignore-selection-range-on-input-style-change.html
:11
> +    const input3 = document.createElement('input');

Please rename this to inputWithAutofocus or something.

>
LayoutTests/editing/selection/ignore-selection-range-on-input-style-change.html
:22
> +

Please remove this blank lines.

>
LayoutTests/editing/selection/ignore-selection-range-on-input-style-change.html
:24
> +<p>Testcase passes if there is no crash </p>

"Testcase" should be "test case" but I think we should just say "This test".

>
LayoutTests/editing/selection/ignore-selection-range-on-input-style-change.html
:25
> +

Ditto.