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.
<rdar://problem/57438200>
Created attachment 384640 [details] 57438200.patch
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.
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.
(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.
Created attachment 384650 [details] 57438200-part-2.patch
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.
Created attachment 384654 [details] 57438200-part-3.patch Addressed the typo and newline in the imports.
Comment on attachment 384654 [details] 57438200-part-3.patch Clearing flags on attachment: 384654 Committed r253009: <https://trac.webkit.org/changeset/253009>
All reviewed patches have been landed. Closing bug.