Summary: | [iOS] Marked text editor state only needs to be set when layout is up-to-date | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Daniel Bates <dbates> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | darin, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||||||||||
Hardware: | iPhone / iPad | ||||||||||||||||||||||||
OS: | iOS 13 | ||||||||||||||||||||||||
Attachments: |
|
Description
Daniel Bates
2020-04-27 11:13:42 PDT
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]. |