Bug 230204 - [macOS] Avoid computing post-layout EditorState data unless necessary
Summary: [macOS] Avoid computing post-layout EditorState data unless necessary
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 230238
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-12 11:51 PDT by Wenson Hsieh
Modified: 2021-09-14 15:43 PDT (History)
7 users (show)

See Also:


Attachments
Patch (14.42 KB, patch)
2021-09-13 18:29 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Just for EWS (14.51 KB, patch)
2021-09-13 18:30 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (16.91 KB, patch)
2021-09-14 08:37 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (18.21 KB, patch)
2021-09-14 13:36 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-09-12 11:51:35 PDT
.
Comment 1 Radar WebKit Bug Importer 2021-09-13 16:55:59 PDT
<rdar://problem/83078713>
Comment 2 Wenson Hsieh 2021-09-13 18:29:00 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-09-13 18:30:31 PDT Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2021-09-14 08:37:58 PDT
Created attachment 438144 [details]
For EWS
Comment 5 Devin Rousso 2021-09-14 13:13:30 PDT
Comment on attachment 438144 [details]
For EWS

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

r=me, neat stuff :)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1234
> -    result.isContentEditable = selection.isContentEditable();
> +    result.isContentEditable = selection.hasEditableStyle();

These appear to have different `UserSelectAllTreatment`.  Is that a problem?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1327
> +    auto isEditable = frame->selection().selection().hasEditableStyle() ? EditorStateIsContentEditable::Yes : EditorStateIsContentEditable::No;

ditto (:1231)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3561
>      m_hasEverFocusedElementDueToUserInteractionSincePageTransition = false;

NIT: Shouldn't this really be inside a `HAVE(TOUCH_BAR)`?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5979
> -    m_hasEverFocusedElementDueToUserInteractionSincePageTransition |= m_userIsInteracting;
> +    if (m_userIsInteracting && m_focusedElement)
> +        m_hasEverFocusedElementDueToUserInteractionSincePageTransition = true;

This seems like a actual functional difference.  Is it something we can/should test?

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:1089
> +    if (m_needsFontAttributes)

NIT: it'd be nice if we could add comments explaining why these things require computing post layout data
Comment 6 Wenson Hsieh 2021-09-14 13:27:31 PDT
Comment on attachment 438144 [details]
For EWS

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1234
>> +    result.isContentEditable = selection.hasEditableStyle();
> 
> These appear to have different `UserSelectAllTreatment`.  Is that a problem?

I don't think this will be an issue since we already use this option when grabbing the `rootEditableElement()`, computing editing style, and other adjacent codepaths.

(`isContentRichlyEditable()` also uses this option, so it would be consistent with that)

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3561
>>      m_hasEverFocusedElementDueToUserInteractionSincePageTransition = false;
> 
> NIT: Shouldn't this really be inside a `HAVE(TOUCH_BAR)`?

Moved this into `HAVE(TOUCH_BAR)` instead.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5979
>> +        m_hasEverFocusedElementDueToUserInteractionSincePageTransition = true;
> 
> This seems like a actual functional difference.  Is it something we can/should test?

It's not actually clear to me what the functional difference would be, since this should only prevent us from setting `m_hasEverFocusedElementDueToUserInteractionSincePageTransition` to true in cases where we wouldn't have tried to show the text editing controls on the Touch Bar anyways.

>> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:1089
>> +    if (m_needsFontAttributes)
> 
> NIT: it'd be nice if we could add comments explaining why these things require computing post layout data

This is because font attributes are plumbed through post-layout data. I'll add comments clarifying this.
Comment 7 Wenson Hsieh 2021-09-14 13:36:19 PDT
Created attachment 438162 [details]
For EWS
Comment 8 EWS 2021-09-14 15:43:05 PDT
Committed r282419 (241678@main): <https://commits.webkit.org/241678@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438162 [details].