Bug 38150 – Refactoring: webkitEditableContentChangedEvent should be handled by the owner of appropriate the renderer

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

A Refactoring to tackle Bug 26526.

[Hajime Morrita]

Created attachment 54364 [details]
v0; a small change preparing for Bug 26526

[Darin Adler]

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.

[Hajime Morrita]

Created attachment 54651 [details]
v1; took feedback

[Hajime Morrita]

Created attachment 54652 [details]
v1; took feedback

[Hajime Morrita]

Comment on attachment 54651 [details]
v1; took feedback

Oops, this is wrong.

[Hajime Morrita]

Comment on attachment 54652 [details]
v1; took feedback

This is also wrong. I'm sorry to bother you ...

[Hajime Morrita]

Created attachment 54663 [details]
v1; took feedback

[Hajime Morrita]

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.

[Darin Adler]

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

[Eric Seidel (no email)]

Attachment 54663 [details] was posted by a committer and has review+, assigning to MORITA Hajime for commit.

[Hajime Morrita]

Committed r58864: <http://trac.webkit.org/changeset/58864>