[webkit-reviews] review granted: [Bug 46950] Some bugs of search cancel button and spin button about state change in an event handler : [Attachment 69417] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 46950: Some bugs of search cancel button and spin button about state change
in an event handler
https://bugs.webkit.org/show_bug.cgi?id=46950

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

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

I’m going to say review+, but I think this can be improved and I’d be willing
to review again if you post an improved version.

> WebCore/rendering/TextControlInnerElements.cpp:222
> +	   RefPtr<HTMLInputElement> protector(input);

Why not just make the local variable “input” be a RefPtr in the first place?
The protector idiom should only be used when there’s no better option, and I
think making the pointer be a RefPtr from the beginning is better.

> WebCore/rendering/TextControlInnerElements.cpp:227
> +	   // By an event handler, the input element might be removed or
hidden,
> +	   // the cancel button might be hidden, or the input type might be
changed.

The grammar here is not great.

You could say: “An event handler might remove or hide the input element, hide
the cancel button, or change the input type.”

But it would be even better to make it clearer what you mean by event handler.
You are alluding to the fact that focus and select calls can invoke JavaScript,
but this may not be clear to the person reading the code.

So you could say: “Focusing and selecting the field may run JavaScript in
response to the event. That code might remove or hide the input element, hide
the cancel button, or change the input type.”

But I’m not sure that list of specific the code can do is all that helpful, nor
is the comment pointing clearly enough to the part of this function that was
written to deal with those possibilities. I think the point here is that we
protect “this” so that we can call renderer() and get access to m_capturing,
and protect “input” so that we can call the select function even after calling
the focus function.

> WebCore/rendering/TextControlInnerElements.cpp:276
> +void SpinButtonElement::detach()

Does the test case cover this? To be specific: Does the test fail if you remove
this code?

> WebCore/rendering/TextControlInnerElements.cpp:314
>	       RefPtr<Node> protector(input);

Why not just make the local variable “input” be a RefPtr in the first place?
The protector idiom should only be used when there’s no better option, and I
think making the pointer be a RefPtr from the beginning is better.

> WebCore/rendering/TextControlInnerElements.cpp:318
> +	       // Input type might be changed by an event handler. This shadow
node might be detached.

Again, I think a much better comment can be written.