Bug 201608 – Replace "deferred element focus" functionality with alternative solution

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

The concept or "deferred element focus" (added in bug #149567), where the UI process defers a WebProcess request to focus an element until editor state WITH post layout data arrive, does two things:

1. Prevents fixing bug #199960.
2. Reveals that SPI -supportsImagePaste depends on bug #199960 (hence causes (1)): -supportsImagePaste depends on the WebProcess sending an editor state update with isMissingPostLayoutData == false even though the editor state object COULD BE missing post layout data.

The purpose of this bug is to see if it's possible to replace this concept with some alternative solution that allows the fix for bug #199960 to move forward.

[Daniel Bates]

Created attachment 378388 [details]
First attempt - always do editor state update if recollect focus info (simple, likely suboptimal, lets see what breaks though)

Simple, yet sub-optimal. Editor state likely does not need to be updated when Web process responding to "update focus info" request. Maybe this also could regress a benchmark? 🤷‍♂️ Anyway, let's just see what functionality breaks.

[Daniel Bates]

Created attachment 393953 [details]
Patch

[Daniel Bates]

Comment on attachment 393953 [details]
Patch

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

> Source/WebKit/ChangeLog:21
> +        defocuing logic in the UI process and prevents fixing <https://bugs.webkit.org/show_bug.cgi?id=199960>.

defocuing => defocusing

[Daniel Bates]

iOS test failure, fast/hidpi/image-srcset-relative-svg-canvas-2x.html, is unrelated to this patch.

[Daniel Bates]

Comment on attachment 393953 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3055
> +    RefPtr<Document> document = m_page->focusController().focusedOrMainFrame().document();
> +    if (!document || !document->view())
>          return;
>  
> +    auto focusedElement = m_focusedElement.copyRef();
> +    bool willLayout = document->view()->needsLayout();
> +    layoutIfNeeded();
> +
> +    // Layout may have detached the document or caused a change of focus.
> +    if (!document->view() || focusedElement != m_focusedElement)
> +        return;
> +
> +    if (willLayout)
> +        sendEditorStateUpdate();
> +    else
> +        scheduleFullEditorStateUpdate();

Maybe extract this into a private member function? Could be called layoutAndSendEditorStateIfNeeded()?

[Daniel Bates]

Ran before and after this change in Speedometer 2.0 and there is 0 change.

[Wenson Hsieh]

Comment on attachment 393953 [details]
Patch

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

>> Source/WebKit/ChangeLog:21
>> +        defocuing logic in the UI process and prevents fixing <https://bugs.webkit.org/show_bug.cgi?id=199960>.
> 
> defocuing => defocusing

Yes, that, or blurring.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3055
>> +        scheduleFullEditorStateUpdate();
> 
> Maybe extract this into a private member function? Could be called layoutAndSendEditorStateIfNeeded()?

IMO, this is okay as-is.

[Wenson Hsieh]

(In reply to Daniel Bates from comment #6)
> Ran before and after this change in Speedometer 2.0 and there is 0 change.

Nice! (I assume you either tested using release builds, or tallied up the total count of editor state computations?)

[Wenson Hsieh]

Oh, you should also remove the declaration of ElementDidFocusArguments.

[Daniel Bates]

Thanks for the review.

(In reply to Wenson Hsieh from comment #8)
> (In reply to Daniel Bates from comment #6)
> > Ran before and after this change in Speedometer 2.0 and there is 0 change.
> 
> Nice! (I assume you either tested using release builds, or tallied up the
> total count of editor state computations?)

I compared the average time.

[Daniel Bates]

(In reply to Wenson Hsieh from comment #9)
> Oh, you should also remove the declaration of ElementDidFocusArguments.

Will remove.

[Daniel Bates]

Created attachment 394133 [details]
To Land

[Daniel Bates]

Comment on attachment 394133 [details]
To Land

Clearing flags on attachment: 394133

Committed r258792: <https://trac.webkit.org/changeset/258792>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60705921>