Bug 207582 – WebPage::getFocusedElementInformation should be robust when the focused element changes during layout

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/47634344>

[Wenson Hsieh]

Created attachment 390421 [details]
Speculative fix

[Tim Horton]

Comment on attachment 390421 [details]
Speculative fix

View in context: https://bugs.webkit.org/attachment.cgi?id=390421&action=review

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2988
> +    auto focusedElement = m_focusedElement;

auto makes it impossible to tell if this retains the element or not. But it needs to, right?

[Wenson Hsieh]

Comment on attachment 390421 [details]
Speculative fix

View in context: https://bugs.webkit.org/attachment.cgi?id=390421&action=review

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2988
>> +    auto focusedElement = m_focusedElement;
> 
> auto makes it impossible to tell if this retains the element or not. But it needs to, right?

Since m_focusedElement is a RefPtr<Element>, auto will just copy it (and ensure that it is reffed).

If you think it's more clear, I can change this to `auto focusedElement = m_focusedElement.copyRef();`

[Tim Horton]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 390421 [details]
> Speculative fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390421&action=review
> 
> >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2988
> >> +    auto focusedElement = m_focusedElement;
> > 
> > auto makes it impossible to tell if this retains the element or not. But it needs to, right?
> 
> Since m_focusedElement is a RefPtr<Element>, auto will just copy it (and
> ensure that it is reffed).

Right, but it's impossible to read the code above and tell that it is correct without referring to another file :D

[Ryosuke Niwa]

(In reply to Tim Horton from comment #4)
> (In reply to Wenson Hsieh from comment #3)
> > Comment on attachment 390421 [details]
> > Speculative fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=390421&action=review
> > 
> > >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2988
> > >> +    auto focusedElement = m_focusedElement;
> > > 
> > > auto makes it impossible to tell if this retains the element or not. But it needs to, right?
> > 
> > Since m_focusedElement is a RefPtr<Element>, auto will just copy it (and
> > ensure that it is reffed).
> 
> Right, but it's impossible to read the code above and tell that it is
> correct without referring to another file :D

Please do call copyRef().

[Wenson Hsieh]

Created attachment 390456 [details]
Patch for landing

[Wenson Hsieh]

Changed to call copyRef() explicitly. Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 390456 [details]
Patch for landing

Clearing flags on attachment: 390456

Committed r256401: <https://trac.webkit.org/changeset/256401>