RenderTextControl has two versions of indexForVisiblePosition, one inherited from RenderObject and another one that takes InnerTextElement in addition to VisiblePosition and used primarily in HTMLTextFormControlElement. We've had quite few security vulnerabilities due to this function being called at undesirable timing in RenderTextControl. Since only call sites of this function outside of HTMLTextFormControlElement is in accessibility, we should move this function to HTMLTextFormControlElement. This will prevent people from inadvertently introducing a similar security vulnerabilities.
Created attachment 100577 [details] clanup
Comment on attachment 100577 [details] clanup View in context: https://bugs.webkit.org/attachment.cgi?id=100577&action=review > Source/WebCore/html/HTMLTextFormControlElement.cpp:41 > +#include "htmlediting.h" // for enclosingTextFormControl Oops, I should have removed this include :(
Created attachment 100578 [details] Removed unnecessarily includes and forward declarations
Sorry, I claimed http://trac.webkit.org/changeset/90849 was the last one but I keep finding more yaks in various places.
Comment on attachment 100578 [details] Removed unnecessarily includes and forward declarations View in context: https://bugs.webkit.org/attachment.cgi?id=100578&action=review > Source/WebCore/editing/htmlediting.cpp:-868 > - return ancestor != container ? toTextFormControl(ancestor) : 0; This means |container| has a shadow host, right? I think "container->sahdowAncestorNode() != container" idiom should represented as a Node's method (or a local variable at least). I need to go back and read the Node.cpp to that intention. (I agree that shadowAncestorNode() is a ugly name. But it's another story...)
Comment on attachment 100578 [details] Removed unnecessarily includes and forward declarations View in context: https://bugs.webkit.org/attachment.cgi?id=100578&action=review >> Source/WebCore/editing/htmlediting.cpp:-868 >> - return ancestor != container ? toTextFormControl(ancestor) : 0; > > This means |container| has a shadow host, right? > I think "container->sahdowAncestorNode() != container" idiom should represented as a Node's method (or a local variable at least). > I need to go back and read the Node.cpp to that intention. > (I agree that shadowAncestorNode() is a ugly name. But it's another story...) Extracting it as a Node's method (maybe inline?) make a sense but we should probably do that in a separate patch.
> Extracting it as a Node's method (maybe inline?) make a sense but we should probably do that in a separate patch. Sounds fine for me.
Comment on attachment 100578 [details] Removed unnecessarily includes and forward declarations Clearing flags on attachment: 100578 Committed r90885: <http://trac.webkit.org/changeset/90885>
All reviewed patches have been landed. Closing bug.