WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
57438200-part-2.patch
(37.77 KB, patch)
2019-12-02 13:19 PST
,
Louie Livon-Bemel
wenson_hsieh
: review+
Details
Formatted Diff
Diff
57438200-part-3.patch
(37.81 KB, patch)
2019-12-02 13:43 PST
,
Louie Livon-Bemel
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Louie Livon-Bemel
Comment 1
2019-12-02 10:23:00 PST
<
rdar://problem/57438200
>
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.
Top of Page
Format For Printing
XML
Clone This Bug