[webkit-reviews] review granted: [Bug 175370] [WK2] EditorState updates should be rolled into the layer update lifecycle when possible : [Attachment 318756] Fix GTK/WPE builds

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 175370: [WK2] EditorState updates should be rolled into the layer update
lifecycle when possible
https://bugs.webkit.org/show_bug.cgi?id=175370

Attachment 318756: Fix GTK/WPE builds

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




--- Comment #28 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 318756
  --> https://bugs.webkit.org/attachment.cgi?id=318756
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.