Bug 64403 – Move RenderTextControl::indexForVisiblePosition to HTMLTextFormControlElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 100577 [details]
clanup

[Ryosuke Niwa]

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 :(

[Ryosuke Niwa]

Created attachment 100578 [details]
Removed unnecessarily includes and forward declarations

[Ryosuke Niwa]

Sorry, I claimed http://trac.webkit.org/changeset/90849 was the last one but I keep finding more yaks in various places.

[Hajime Morrita]

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

[Ryosuke Niwa]

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.

[Hajime Morrita]

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

[WebKit Review Bot]

Comment on attachment 100578 [details]
Removed unnecessarily includes and forward declarations

Clearing flags on attachment: 100578

Committed r90885: <http://trac.webkit.org/changeset/90885>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.