Bug 175370

Summary: [WK2] EditorState updates should be rolled into the layer update lifecycle when possible
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit2Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bdakin, buildbot, ossy, rniwa, thorton
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176052
Bug Depends on:    
Bug Blocks: 175116    
Attachments:
Description Flags
Which LayoutTests fail if I take out EditorState altogether?
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Initial EWS pass
none
Fix Mac/iOS API tests
none
Another EWS pass (fixes internal iOS/Mac platform-specific layout tests)
none
Another EWS pass (fixes internal iOS/Mac platform-specific layout tests)
none
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Remove targeted EditorState flushes in WebPage
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Remove targeted EditorState flushes in WebPage
none
EWS test run
none
Fix GTK/WPE builds
rniwa: review+
Patch for landing none

Description Wenson Hsieh 2017-08-08 23:07:21 PDT
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).
Comment 1 Tim Horton 2017-08-09 00:45:53 PDT
A+ what a good plan 🙃
Comment 2 Wenson Hsieh 2017-08-09 07:44:50 PDT
<rdar://problem/33799806>
Comment 3 Wenson Hsieh 2017-08-09 07:58:35 PDT
Created attachment 317699 [details]
Which LayoutTests fail if I take out EditorState altogether?
Comment 4 Build Bot 2017-08-09 09:13:38 PDT
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
Comment 5 Build Bot 2017-08-09 09:13:39 PDT
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
Comment 6 Build Bot 2017-08-09 09:37:50 PDT
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
Comment 7 Build Bot 2017-08-09 09:37:52 PDT
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
Comment 8 Wenson Hsieh 2017-08-09 22:55:27 PDT
Created attachment 317784 [details]
Initial EWS pass
Comment 9 Alexey Proskuryakov 2017-08-10 16:05:17 PDT
> 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.
Comment 10 Wenson Hsieh 2017-08-10 16:38:05 PDT
(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.
Comment 11 Wenson Hsieh 2017-08-12 03:28:24 PDT
Created attachment 317994 [details]
Fix Mac/iOS API tests
Comment 12 Wenson Hsieh 2017-08-14 16:18:46 PDT
Created attachment 318080 [details]
Another EWS pass (fixes internal iOS/Mac platform-specific layout tests)
Comment 13 Wenson Hsieh 2017-08-14 16:27:24 PDT
Created attachment 318081 [details]
Another EWS pass (fixes internal iOS/Mac platform-specific layout tests)
Comment 14 Wenson Hsieh 2017-08-14 22:13:27 PDT
Created attachment 318111 [details]
Patch
Comment 15 Build Bot 2017-08-15 08:54:57 PDT
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.
Comment 16 Build Bot 2017-08-15 08:54:59 PDT
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
Comment 17 Wenson Hsieh 2017-08-15 09:38:11 PDT
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.
Comment 18 Ryosuke Niwa 2017-08-18 13:50:54 PDT
(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.
Comment 19 Alexey Proskuryakov 2017-08-18 15:13:01 PDT
> 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.
Comment 20 Ryosuke Niwa 2017-08-18 15:37:03 PDT
(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?
Comment 21 Alexey Proskuryakov 2017-08-18 16:16:28 PDT
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.
Comment 22 Wenson Hsieh 2017-08-20 02:11:22 PDT
Created attachment 318599 [details]
Remove targeted EditorState flushes in WebPage
Comment 23 Build Bot 2017-08-20 03:26:12 PDT
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
Comment 24 Build Bot 2017-08-20 03:26:13 PDT
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
Comment 25 Wenson Hsieh 2017-08-20 05:19:43 PDT
Created attachment 318602 [details]
Remove targeted EditorState flushes in WebPage
Comment 26 Wenson Hsieh 2017-08-21 23:45:26 PDT
Created attachment 318742 [details]
EWS test run
Comment 27 Wenson Hsieh 2017-08-22 07:35:00 PDT
Created attachment 318756 [details]
Fix GTK/WPE builds
Comment 28 Ryosuke Niwa 2017-08-22 14:32:29 PDT
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.
Comment 29 Wenson Hsieh 2017-08-22 15:53:09 PDT
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.

👍🏻
Comment 30 Wenson Hsieh 2017-08-22 17:15:18 PDT
Created attachment 318830 [details]
Patch for landing
Comment 31 Csaba Osztrogonác 2017-08-22 20:55:23 PDT
It broke the Apple Windows build, see build.webkit.org for details'
Comment 32 Csaba Osztrogonác 2017-08-23 08:47:53 PDT
just to document, fix landed in https://trac.webkit.org/changeset/221066/webkit
Comment 33 Wenson Hsieh 2017-08-23 08:49:53 PDT
(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)
Comment 34 Alexey Proskuryakov 2019-04-19 14:43:12 PDT
Has this been landed? It looks like it, but the bug is still open.