Bug 204758 – Add helper methods for description and equality to text manipulation SPI

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Louie Livon-Bemel]

Add helper methods to _WKTextManipulationItem and _WKTextManipulationToken to make working with them easier; add methods to see if two are equal (optionally ignoring the actual content on the page), and add methods for getting both a description for use in lldb, and a privacy preserving description so the object can be logged without logging any contents of the webpage.

[Louie Livon-Bemel]

<rdar://problem/57438200>

[Louie Livon-Bemel]

Created attachment 384640 [details]
57438200.patch

[David Quesada]

Comment on attachment 384640 [details]
57438200.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384640&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:57
> +- (BOOL)isEqualToTextManipulationItem:(_WKTextManipulationItem *)otherItem includingContentEquality:(BOOL)includingContentEquality

It seems like this class should maybe also have an override of -isEqual: which calls this method with some value for `includingContentEquality`.

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:83
> +- (NSString *)privacyPreservingDescription

This pattern doesn't seem common in WebKit. Is there any reason you decided not to make -description return the privacy-preserving string and implement -debugDescription to return the full string when called from the debugger?

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:29
>  @implementation _WKTextManipulationToken

Ditto my comments about the description and equality methods in this class.

[Wenson Hsieh]

Comment on attachment 384640 [details]
57438200.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384640&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:62
> +    if (![self.identifier isEqualToString:otherItem.identifier] || self.tokens.count != otherItem.tokens.count)

This would return NO in the case where both self and otherItem have nil identifiers (it’s unclear to me if that is desired).

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:687
> +    NSString *debugDescription = item.get().description;

Nit - We usually just use [item description] for this (or [item debugDescription], per David’s earlier comments).

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:694
> +    NSString *privacyPreservingDescription = item.get().privacyPreservingDescription;

Ditto.

[Louie Livon-Bemel]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 384640 [details]
> 57438200.patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384640&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:62
> > +    if (![self.identifier isEqualToString:otherItem.identifier] || self.tokens.count != otherItem.tokens.count)
> 
> This would return NO in the case where both self and otherItem have nil
> identifiers (it’s unclear to me if that is desired).

I think we would want both having nil identifiers but being otherwise equal to be considered equal. I can't re-declare `isEqualOrBothNil()` in this file because of Unified Source compilation saying it's redeclared, but I'll just add the logic directly here. And I'll add a test for this as well.
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:687
> > +    NSString *debugDescription = item.get().description;
> 
> Nit - We usually just use [item description] for this (or [item
> debugDescription], per David’s earlier comments).

I'll change this to message syntax. Per David's suggestion, I'm also making -description be the privacy preserving method, removing -privacyPreservingDescription, and adding -debugDescription which shows the full content.
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:694
> > +    NSString *privacyPreservingDescription = item.get().privacyPreservingDescription;
> 
> Ditto.

[Louie Livon-Bemel]

Created attachment 384650 [details]
57438200-part-2.patch

[Wenson Hsieh]

Comment on attachment 384650 [details]
57438200-part-2.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384650&action=review

LGTM, with a couple of very minor comments.

> Source/WebKit/ChangeLog:35
> +            The general description for should not include the webpage content.

Nit - “…description for should not…”

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:27
>  #import "_WKTextManipulationItem.h"

Nit - we typically leave a newline below the primary header.

[Louie Livon-Bemel]

Created attachment 384654 [details]
57438200-part-3.patch

Addressed the typo and newline in the imports.

[WebKit Commit Bot]

Comment on attachment 384654 [details]
57438200-part-3.patch

Clearing flags on attachment: 384654

Committed r253009: <https://trac.webkit.org/changeset/253009>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.