Marked text editor state only needs to be set when layout is up-to-date. The code already ensures this: the caller WebPage::editorState() forces a layout whenever platformNeedsLayoutForEditorState() returns true, which it is if there is a composition. So, this bug is just about categorizing the marked text editor state fields to match what happens today: they are always updated post layout. So, they should be grouped under post-layout data.
Created attachment 397707 [details] Patch
Comment on attachment 397707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397707&action=review > Source/WebKit/Shared/EditorState.cpp:287 > - if (editorState.postLayoutData().caretRectAtEnd != IntRect()) > + if (!editorState.postLayoutData().firstMarkedRect.isEmpty()) Note isEmpty() is good enough. It's NOT equivalent to != IntRect(), which compares rect location + rect size.
Comment on attachment 397707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397707&action=review > Source/WebKit/ChangeLog:23 > + am here also replace checks of != IntRect() with IntRect::isEmpty(), which is more canonical. Note isEmpty() is good enough. It's NOT equivalent to != IntRect(), which compares rect location + rect size.
Comment on attachment 397707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397707&action=review >> Source/WebKit/ChangeLog:23 >> + am here also replace checks of != IntRect() with IntRect::isEmpty(), which is more canonical. > > Note isEmpty() is good enough. It's NOT equivalent to != IntRect(), which compares rect location + rect size. Actually, I'm thinking this change is wrong. will revert
Created attachment 397709 [details] Patch
Created attachment 397710 [details] Patch
Comment on attachment 397710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397710&action=review > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:-246 > + // We only set the remaining EditorState entries if layout is done as a performance optimization > + // to avoid the need to force a synchronous layout here to compute these entries. > + if (!view || view->needsLayout() || result.isMissingPostLayoutData) > return; > - } Seems like this only needs to check isMissingPostLayoutData. The other conditions are already guaranteed by the editorState function before this is called. Maybe even change the interface to getPlatformEditorState to make this clearer. Also seems like isMissingPostLayoutData should eventually be dumped and instead we should use Optional for m_postLayoutData. That way the two can’t get out of sync.
Comment on attachment 397710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397710&action=review Thanks for the review. >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:-246 >> - } > > Seems like this only needs to check isMissingPostLayoutData. The other conditions are already guaranteed by the editorState function before this is called. Maybe even change the interface to getPlatformEditorState to make this clearer. > > Also seems like isMissingPostLayoutData should eventually be dumped and instead we should use Optional for m_postLayoutData. That way the two can’t get out of sync. Yep and yep. I'll make m_postLayoutData optional in the next patch.
Created attachment 397712 [details] To Land
Created attachment 397739 [details] To Land
Created attachment 397740 [details] To Land
Created attachment 397744 [details] To Land
Created attachment 397761 [details] To Land
Comment on attachment 397761 [details] To Land View in context: https://bugs.webkit.org/attachment.cgi?id=397761&action=review > Source/WebKit/ChangeLog:34 > + could cause this, but the code currently covers this case. So, taking a RefPtr is about future proofing. RefPtr => Ref
Created attachment 397762 [details] To Land
Created attachment 397766 [details] To Land
Committed r260826: <https://trac.webkit.org/changeset/260826> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397766 [details].
<rdar://problem/62518537>