RESOLVED FIXED 204758
Add helper methods for description and equality to text manipulation SPI
https://bugs.webkit.org/show_bug.cgi?id=204758
Summary Add helper methods for description and equality to text manipulation SPI
Louie Livon-Bemel
Reported 2019-12-02 10:22:27 PST
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.
Attachments
57438200.patch (33.45 KB, patch)
2019-12-02 10:56 PST, Louie Livon-Bemel
no flags
57438200-part-2.patch (37.77 KB, patch)
2019-12-02 13:19 PST, Louie Livon-Bemel
wenson_hsieh: review+
57438200-part-3.patch (37.81 KB, patch)
2019-12-02 13:43 PST, Louie Livon-Bemel
no flags
Louie Livon-Bemel
Comment 1 2019-12-02 10:23:00 PST
Louie Livon-Bemel
Comment 2 2019-12-02 10:56:15 PST
Created attachment 384640 [details] 57438200.patch
David Quesada
Comment 3 2019-12-02 11:13:07 PST
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 4 2019-12-02 11:24:14 PST
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
Comment 5 2019-12-02 13:04:35 PST
(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
Comment 6 2019-12-02 13:19:18 PST
Created attachment 384650 [details] 57438200-part-2.patch
Wenson Hsieh
Comment 7 2019-12-02 13:32:14 PST
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
Comment 8 2019-12-02 13:43:32 PST
Created attachment 384654 [details] 57438200-part-3.patch Addressed the typo and newline in the imports.
WebKit Commit Bot
Comment 9 2019-12-02 14:31:06 PST
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
Comment 10 2019-12-02 14:31:07 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.