Summary: | Add helper methods for description and equality to text manipulation SPI | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Louie Livon-Bemel <llivonbemel> | ||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, david_quesada, llivonbemel, webkit-bug-importer, wenson_hsieh | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Louie Livon-Bemel
2019-12-02 10:22:27 PST
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. |