Description
Wenson Hsieh
2017-08-08 23:07:21 PDT
A+ what a good plan 🙃 Created attachment 317699 [details]
Which LayoutTests fail if I take out EditorState altogether?
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 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 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 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
Created attachment 317784 [details]
Initial EWS pass
> 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.
(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. Created attachment 317994 [details]
Fix Mac/iOS API tests
Created attachment 318080 [details]
Another EWS pass (fixes internal iOS/Mac platform-specific layout tests)
Created attachment 318081 [details]
Another EWS pass (fixes internal iOS/Mac platform-specific layout tests)
Created attachment 318111 [details]
Patch
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. 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 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.
(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. > 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.
(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? 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. Created attachment 318599 [details]
Remove targeted EditorState flushes in WebPage
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 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
Created attachment 318602 [details]
Remove targeted EditorState flushes in WebPage
Created attachment 318742 [details]
EWS test run
Created attachment 318756 [details]
Fix GTK/WPE builds
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 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. 👍🏻 Created attachment 318830 [details]
Patch for landing
It broke the Apple Windows build, see build.webkit.org for details' just to document, fix landed in https://trac.webkit.org/changeset/221066/webkit (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) Has this been landed? It looks like it, but the bug is still open. |