Summary: | Refactoring: webkitEditableContentChangedEvent should be handled by the owner of appropriate the renderer | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||
Component: | DOM | Assignee: | 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
Hajime Morrita
2010-04-26 17:14:45 PDT
Created attachment 54364 [details] v0; a small change preparing for Bug 26526 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. Created attachment 54651 [details]
v1; took feedback
Created attachment 54652 [details]
v1; took feedback
Comment on attachment 54651 [details]
v1; took feedback
Oops, this is wrong.
Comment on attachment 54652 [details]
v1; took feedback
This is also wrong. I'm sorry to bother you ...
Created attachment 54663 [details]
v1; took feedback
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 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 Attachment 54663 [details] was posted by a committer and has review+, assigning to MORITA Hajime for commit.
Committed r58864: <http://trac.webkit.org/changeset/58864> |