Summary: | [macOS] Avoid computing post-layout EditorState data unless necessary | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | akeerthi, bdakin, hi, megan_gardner, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 230238 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2021-09-12 11:51:35 PDT
Created attachment 438094 [details]
Patch
Created attachment 438095 [details]
Just for EWS
Created attachment 438144 [details]
For EWS
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 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. Created attachment 438162 [details]
For EWS
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]. |