[Webkit-unassigned] [Bug 46950] Some bugs of search cancel button and spin button about state change in an event handler

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46950


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69417|review?                     |review+
               Flag|                            |




--- Comment #3 from Darin Adler <darin at apple.com>  2010-11-04 12:56:57 PST ---
(From update of attachment 69417)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.