Bug 211087 – [iOS] Marked text editor state only needs to be set when layout is up-to-date

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 397707 [details]
Patch

[Daniel Bates]

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.

[Daniel Bates]

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.

[Daniel Bates]

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

[Daniel Bates]

Created attachment 397709 [details]
Patch

[Daniel Bates]

Created attachment 397710 [details]
Patch

[Darin Adler]

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.

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 397712 [details]
To Land

[Daniel Bates]

Created attachment 397739 [details]
To Land

[Daniel Bates]

Created attachment 397740 [details]
To Land

[Daniel Bates]

Created attachment 397744 [details]
To Land

[Daniel Bates]

Created attachment 397761 [details]
To Land

[Daniel Bates]

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

[Daniel Bates]

Created attachment 397762 [details]
To Land

[Daniel Bates]

Created attachment 397766 [details]
To Land

[EWS]

Committed r260826: <https://trac.webkit.org/changeset/260826>

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

[Radar WebKit Bug Importer]

<rdar://problem/62518537>