Bug 211087 - [iOS] Marked text editor state only needs to be set when layout is up-to-date
Summary: [iOS] Marked text editor state only needs to be set when layout is up-to-date
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 13
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-27 11:13 PDT by Daniel Bates
Modified: 2020-04-28 07:55 PDT (History)
2 users (show)

See Also:


Attachments
Patch (13.29 KB, patch)
2020-04-27 11:46 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (12.08 KB, patch)
2020-04-27 11:57 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (12.08 KB, patch)
2020-04-27 11:58 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (12.12 KB, patch)
2020-04-27 12:21 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (16.75 KB, patch)
2020-04-27 14:42 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (16.76 KB, patch)
2020-04-27 14:43 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (16.79 KB, patch)
2020-04-27 14:47 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (16.81 KB, patch)
2020-04-27 16:20 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (16.81 KB, patch)
2020-04-27 16:41 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (18.04 KB, patch)
2020-04-27 16:51 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-04-27 11:13:42 PDT
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.
Comment 1 Daniel Bates 2020-04-27 11:46:33 PDT
Created attachment 397707 [details]
Patch
Comment 2 Daniel Bates 2020-04-27 11:50:40 PDT
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 3 Daniel Bates 2020-04-27 11:52:52 PDT
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 4 Daniel Bates 2020-04-27 11:55:36 PDT
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
Comment 5 Daniel Bates 2020-04-27 11:57:26 PDT
Created attachment 397709 [details]
Patch
Comment 6 Daniel Bates 2020-04-27 11:58:44 PDT
Created attachment 397710 [details]
Patch
Comment 7 Darin Adler 2020-04-27 12:06:36 PDT
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 8 Daniel Bates 2020-04-27 12:18:44 PDT
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.
Comment 9 Daniel Bates 2020-04-27 12:21:36 PDT
Created attachment 397712 [details]
To Land
Comment 10 Daniel Bates 2020-04-27 14:42:12 PDT
Created attachment 397739 [details]
To Land
Comment 11 Daniel Bates 2020-04-27 14:43:06 PDT
Created attachment 397740 [details]
To Land
Comment 12 Daniel Bates 2020-04-27 14:47:28 PDT
Created attachment 397744 [details]
To Land
Comment 13 Daniel Bates 2020-04-27 16:20:31 PDT
Created attachment 397761 [details]
To Land
Comment 14 Daniel Bates 2020-04-27 16:21:16 PDT
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
Comment 15 Daniel Bates 2020-04-27 16:41:20 PDT
Created attachment 397762 [details]
To Land
Comment 16 Daniel Bates 2020-04-27 16:51:14 PDT
Created attachment 397766 [details]
To Land
Comment 17 EWS 2020-04-28 07:54:56 PDT
Committed r260826: <https://trac.webkit.org/changeset/260826>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397766 [details].
Comment 18 Radar WebKit Bug Importer 2020-04-28 07:55:15 PDT
<rdar://problem/62518537>