[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 bugzilla-daemon at webkit.org
Fri Nov 5 00:48:41 PDT 2010


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





--- Comment #5 from Kent Tamura <tkent at chromium.org>  2010-11-05 00:48:41 PST ---
(From update of attachment 69417)
View in context: https://bugs.webkit.org/attachment.cgi?id=69417&action=review

>> 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.

Oh, right.  "input' should be a RefPtr.  It's simpler.

>> WebCore/rendering/TextControlInnerElements.cpp:227
>> +        // 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.

I changed a way to fix the issue.
I moved focus() and select() because I wanted to make sure the cancel button was visible before starting event capturing.
The updated patch removes the visibility check in mouseup event, and doesn't change the mousedown event handling.  It is much simpler.
I removed the comment.

>> 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?

Ah, no.  I have added a test case for this.

>> 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.

I have improved the comment.

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


More information about the webkit-unassigned mailing list