Bug 38150

Summary: Refactoring: webkitEditableContentChangedEvent should be handled by the owner of appropriate the renderer
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
v0; a small change preparing for Bug 26526
none
v1; took feedback
none
v1; took feedback
none
v1; took feedback none

Description Hajime Morrita 2010-04-26 17:14:45 PDT
A Refactoring to tackle Bug 26526.
Comment 1 Hajime Morrita 2010-04-26 18:17:06 PDT
Created attachment 54364 [details]
v0; a small change preparing for Bug 26526
Comment 2 Darin Adler 2010-04-27 12:47:28 PDT
Comment on attachment 54364 [details]
v0; a small change preparing for Bug 26526

WebCore/html/HTMLFormControlElement.h:160
 +      virtual void defaultEventHandler(Event*);
This should be as private as possible. I don't see any good reason to make it public. Either protected or private should work. Similarly, I think HTMLInputElement and HTMLTextAreaElement could now have private defaultEventHandler functions since there is no longer anyone calling directly.

WebCore/rendering/TextControlInnerElements.cpp:119
 +      if (shadowAncestor && shadowAncestor->renderer()) {
 +          ASSERT(shadowAncestor->renderer()->isTextControl());
We should not fetch the renderer() any more or assert that it is a text control.

review- because I think it's worth refining this a tiny bit more, since its entire purpose is the refactoring.
Comment 3 Hajime Morrita 2010-04-28 18:02:45 PDT
Created attachment 54651 [details]
v1; took feedback
Comment 4 Hajime Morrita 2010-04-28 18:02:48 PDT
Created attachment 54652 [details]
v1; took feedback
Comment 5 Hajime Morrita 2010-04-28 18:04:07 PDT
Comment on attachment 54651 [details]
v1; took feedback

Oops, this is wrong.
Comment 6 Hajime Morrita 2010-04-28 18:05:43 PDT
Comment on attachment 54652 [details]
v1; took feedback

This is also wrong. I'm sorry to bother you ...
Comment 7 Hajime Morrita 2010-04-28 19:14:28 PDT
Created attachment 54663 [details]
v1; took feedback
Comment 8 Hajime Morrita 2010-04-28 19:17:38 PDT
Hi Darin, thank you for reviewing and sorry for noisy attachments.

(In reply to comment #2)
> (From update of attachment 54364 [details])
> WebCore/html/HTMLFormControlElement.h:160
>  +      virtual void defaultEventHandler(Event*);
> This should be as private as possible. I don't see any good reason to make it
> public. Either protected or private should work. Similarly, I think
> HTMLInputElement and HTMLTextAreaElement could now have private
> defaultEventHandler functions since there is no longer anyone calling directly.
Fixed moving defaultEventHandler() declarations
from public to protected or private (based on inheritance relation)

> WebCore/rendering/TextControlInnerElements.cpp:119
>  +      if (shadowAncestor && shadowAncestor->renderer()) {
>  +          ASSERT(shadowAncestor->renderer()->isTextControl());
> We should not fetch the renderer() any more or assert that it is a text
> control.
Fixed.
Comment 9 Darin Adler 2010-04-30 13:17:13 PDT
Comment on attachment 54663 [details]
v1; took feedback

> +        ASSERT(!shadowAncestor->renderer() || shadowAncestor->renderer()->isTextControl());

I'm not sure this assertion is helpful or needed. This code doesn't have any reason to care about the type of the renderer.

r=me regardless
Comment 10 Eric Seidel (no email) 2010-05-02 19:28:03 PDT
Attachment 54663 [details] was posted by a committer and has review+, assigning to MORITA Hajime for commit.
Comment 11 Hajime Morrita 2010-05-05 23:03:45 PDT
Committed r58864: <http://trac.webkit.org/changeset/58864>