Bug 175370 – [WK2] EditorState updates should be rolled into the layer update lifecycle when possible

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Wenson Hsieh]

In https://trac.webkit.org/changeset/220393/webkit, I inadvertently regressed the performance of WebPage::editorState, causing a test that thrashes DOM selections (editing/selection/move-by-word-visually-multi-space.html) to take 40-50% longer to complete, causing a timeout.

I think this can be addressed via an optimization I've been meaning to implement for a while -- rather than synchronously send EditorState updates upon selection change, use an invalidation mechanism to tell the layer tree that it should include an EditorState in the next transaction (scheduling a layer tree update if needed).

[Tim Horton]

A+ what a good plan 🙃

[Wenson Hsieh]

<rdar://problem/33799806>

[Wenson Hsieh]

Created attachment 317699 [details]
Which LayoutTests fail if I take out EditorState altogether?

[Build Bot]

Comment on attachment 317699 [details]
Which LayoutTests fail if I take out EditorState altogether?

Attachment 317699 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4284468

New failing tests:
editing/secure-input/removed-password-input.html
editing/secure-input/reset-state-on-navigation.html
editing/secure-input/password-input-focusing.html
editing/secure-input/password-input-changed-type.html

[Build Bot]

Created attachment 317710 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317699 [details]
Which LayoutTests fail if I take out EditorState altogether?

Attachment 317699 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4284489

New failing tests:
editing/inserting/insert-div-024.html
editing/inserting/insert-div-026.html
editing/style/5084241.html
editing/selection/character-granularity-rect.html
editing/style/unbold-in-bold.html

[Build Bot]

Created attachment 317712 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Wenson Hsieh]

Created attachment 317784 [details]
Initial EWS pass

[Alexey Proskuryakov]

> rather than synchronously send EditorState updates upon selection change, use an invalidation mechanism to tell the layer tree that it should include an EditorState in the next transaction

This doesn't sound right to me. We need to keep editor state in UI process and WebProcess reasonably in sync so that input methods don't get confused during multi-step operations, that's not tied to rendering in any way.

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #9)
> > rather than synchronously send EditorState updates upon selection change, use an invalidation mechanism to tell the layer tree that it should include an EditorState in the next transaction
> 
> This doesn't sound right to me. We need to keep editor state in UI process
> and WebProcess reasonably in sync so that input methods don't get confused
> during multi-step operations, that's not tied to rendering in any way.

Good catch! The plan is not to make EditorStates lazy across the board, but rather, only send EditorState updates immediately if we need to. Setting marked text should be one of these circumstances where we should flush any dirtied EditorState immediately, in the same runloop as the editing happens.

[Wenson Hsieh]

Created attachment 317994 [details]
Fix Mac/iOS API tests

[Wenson Hsieh]

Created attachment 318080 [details]
Another EWS pass (fixes internal iOS/Mac platform-specific layout tests)

[Wenson Hsieh]

Created attachment 318081 [details]
Another EWS pass (fixes internal iOS/Mac platform-specific layout tests)

[Wenson Hsieh]

Created attachment 318111 [details]
Patch

[Build Bot]

Comment on attachment 318111 [details]
Patch

Attachment 318111 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4317415

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 318119 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Wenson Hsieh]

Comment on attachment 318111 [details]
Patch

I confirmed with Ryan that these test failures on ews101 are WebKit1. Not sure what's going on with the bot, since my patch doesn't change any WebKit1 behavior.

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #9)
> > rather than synchronously send EditorState updates upon selection change, use an invalidation mechanism to tell the layer tree that it should include an EditorState in the next transaction
> 
> This doesn't sound right to me. We need to keep editor state in UI process
> and WebProcess reasonably in sync so that input methods don't get confused
> during multi-step operations, that's not tied to rendering in any way.

The point of this patch is to send it at the end of runloop. There's no point in sending multiple IPCs per a single runloop to keep input methods updated multiple times since it's async relative to UIProcess anyway.

[Alexey Proskuryakov]

> There's no point in sending multiple IPCs per a single runloop to keep input methods updated multiple times since it's async relative to UIProcess anyway.

Only a subset of methods that get called is async, many more aren't (e.g. -inputContext). AppKit is free to call any SPI methods of WKView as part of input processing.

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #19)
> > There's no point in sending multiple IPCs per a single runloop to keep input methods updated multiple times since it's async relative to UIProcess anyway.
> 
> Only a subset of methods that get called is async, many more aren't (e.g.
> -inputContext). AppKit is free to call any SPI methods of WKView as part of
> input processing.

But that's sync IPC from UI process to WebContent process. How is that relevant to sending states from WebContent process to UI process async?

Also, we're updating EditorState sync whenever composition event is involved so I don't see how new behavior could affect input methods.

What are specific scenarios in which this can affect input methods?

[Alexey Proskuryakov]

inputContext uses state from EditorState. We should never have any sync IPC from UI process to WebContent, that's just not supported. There are some icky exceptions in painting code.

[Wenson Hsieh]

Created attachment 318599 [details]
Remove targeted EditorState flushes in WebPage

[Build Bot]

Comment on attachment 318599 [details]
Remove targeted EditorState flushes in WebPage

Attachment 318599 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4347506

New failing tests:
editing/style/unbold-in-bold.html

[Build Bot]

Created attachment 318600 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Wenson Hsieh]

Created attachment 318602 [details]
Remove targeted EditorState flushes in WebPage

[Wenson Hsieh]

Created attachment 318742 [details]
EWS test run

[Wenson Hsieh]

Created attachment 318756 [details]
Fix GTK/WPE builds

[Ryosuke Niwa]

Comment on attachment 318756 [details]
Fix GTK/WPE builds

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

> Source/WebCore/editing/FrameSelection.cpp:174
> +    UserTriggeredSelectionChangeScope userTriggeredScope(*this, userTriggered);
> +
>      setSelection(VisibleSelection(pos.deepEquivalent(), pos.deepEquivalent(), pos.affinity(), m_selection.isDirectional()),
>          defaultSetSelectionOptions(userTriggered), AXTextStateChangeIntent(), align);

Instead of annotating everywhere setSelection is called, can we modify setSelection to take userTriggered
or a new method like setSelectionByUser and do this work there instead?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3443
> +        layerTransaction.setEditorState(editorState());

We should really rename editorState() to computeEditorState() given making the editor state is quite expensive.
It's not like we're accessing a member variable here.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5615
> +void WebPage::immediatelySendPostLayoutEditorStateUpdate()

I think it's a bit misleading to call this as immediatelySendPostLayoutEditorStateUpdate since we're not sending post-layout data here.
Maybe just sendEditorStateUpdate?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5638
> +    Frame& frame = m_page->focusController().focusedOrMainFrame();
> +    if (frame.editor().ignoreSelectionChanges())

It's a bit strange that we're checking this state here but I can't think of a better alternative.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3204
> +    VisiblePosition(selection.start()).absoluteCaretBounds(&isInsideFixedPosition);

You should use selection.visibleStart() instead.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3208
> +    VisiblePosition(selection.end()).absoluteCaretBounds(&isInsideFixedPosition);

Ditto for selection.visibleEnd()

> LayoutTests/ChangeLog:28
> +        Refactor and simplify these tests.

I think it's good to mention that these tests are not ran on open source iOS bots.

[Wenson Hsieh]

Comment on attachment 318756 [details]
Fix GTK/WPE builds

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

>> Source/WebCore/editing/FrameSelection.cpp:174
>>          defaultSetSelectionOptions(userTriggered), AXTextStateChangeIntent(), align);
> 
> Instead of annotating everywhere setSelection is called, can we modify setSelection to take userTriggered
> or a new method like setSelectionByUser and do this work there instead?

Sounds good. I added a SetSelectionOptions flag to indicate UserTriggered, and call out to the editing delegate from setSelection if the flag is set.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3443
>> +        layerTransaction.setEditorState(editorState());
> 
> We should really rename editorState() to computeEditorState() given making the editor state is quite expensive.
> It's not like we're accessing a member variable here.

I agree! But I think I'll do this as a followup (the patch is getting quite bloated as is :/)

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5615
>> +void WebPage::immediatelySendPostLayoutEditorStateUpdate()
> 
> I think it's a bit misleading to call this as immediatelySendPostLayoutEditorStateUpdate since we're not sending post-layout data here.
> Maybe just sendEditorStateUpdate?

True, the name I chose here is a bit misleading. Renamed to sendEditorStateUpdate.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5638
>> +    if (frame.editor().ignoreSelectionChanges())
> 
> It's a bit strange that we're checking this state here but I can't think of a better alternative.

It is :/

Perhaps it would make more sense to have a flag in WebPage instead, like m_isIgnoringEditorStateUpdates.
Then, when calling Editor::setIgnoreSelectionChanges(), we notify the client (WebEditorClient and WebPage) that we're ignoring selection changes, and the client then sets this flag to the appropriate value.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3204
>> +    VisiblePosition(selection.start()).absoluteCaretBounds(&isInsideFixedPosition);
> 
> You should use selection.visibleStart() instead.

Good call. Fixed!

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3208
>> +    VisiblePosition(selection.end()).absoluteCaretBounds(&isInsideFixedPosition);
> 
> Ditto for selection.visibleEnd()

Fixed.

>> LayoutTests/ChangeLog:28
>> +        Refactor and simplify these tests.
> 
> I think it's good to mention that these tests are not ran on open source iOS bots.

👍🏻

[Wenson Hsieh]

Created attachment 318830 [details]
Patch for landing

[Csaba Osztrogonác]

It broke the Apple Windows build, see build.webkit.org for details'

[Csaba Osztrogonác]

just to document, fix landed in https://trac.webkit.org/changeset/221066/webkit

[Wenson Hsieh]

(In reply to Csaba Osztrogonác from comment #32)
> just to document, fix landed in
> https://trac.webkit.org/changeset/221066/webkit

(Also: original commit was in https://trac.webkit.org/changeset/221064/webkit)

[Alexey Proskuryakov]

Has this been landed? It looks like it, but the bug is still open.