Summary: | Add exclusion rule for text manipulation SPI to exclude based on element class | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Louie Livon-Bemel <llivonbemel> | ||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, llivonbemel, mifenton, rniwa, 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:13:57 PST
Created attachment 384666 [details]
57398802.patch
Created attachment 384723 [details]
57398802-part-2.patch
Updated match() function to not mutate the element, and therefore left the argument as const.
Comment on attachment 384723 [details] 57398802-part-2.patch View in context: https://bugs.webkit.org/attachment.cgi?id=384723&action=review > Source/WebCore/ChangeLog:9 > + Tests added as API tests. This should instead be: Tests: TextManipulation.StartTextManipulationApplyInclusionExclusionRulesForClass TextManipulation.StartTextManipulationApplyInclusionExclusionRulesForClassAndAttribute > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:248 > + RetainPtr<_WKTextManipulationConfiguration> configuration = adoptNS([[_WKTextManipulationConfiguration alloc] init]); Why not auto? > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:279 > + RetainPtr<_WKTextManipulationConfiguration> configuration = adoptNS([[_WKTextManipulationConfiguration alloc] init]); Use auto? Comment on attachment 384723 [details] 57398802-part-2.patch View in context: https://bugs.webkit.org/attachment.cgi?id=384723&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:283 > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)YES forAttribute:@"data-exclude" value:@"yes"] autorelease], > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)NO forAttribute:@"data-exclude" value:@"no"] autorelease], > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)YES forClass:@"exclude"] autorelease], Nit - I don’t think you need the (BOOL) casts here. (In reply to Ryosuke Niwa from comment #4) > Comment on attachment 384723 [details] > 57398802-part-2.patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384723&action=review > > > Source/WebCore/ChangeLog:9 > > + Tests added as API tests. > > This should instead be: > Tests: > TextManipulation.StartTextManipulationApplyInclusionExclusionRulesForClass > > TextManipulation. > StartTextManipulationApplyInclusionExclusionRulesForClassAndAttribute Fixed. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:248 > > + RetainPtr<_WKTextManipulationConfiguration> configuration = adoptNS([[_WKTextManipulationConfiguration alloc] init]); > > Why not auto? No specific reason, I'm just in the habit of declaring types since I'm used to Objective-C. I'll make these `auto`. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:279 > > + RetainPtr<_WKTextManipulationConfiguration> configuration = adoptNS([[_WKTextManipulationConfiguration alloc] init]); > > Use auto? (In reply to Wenson Hsieh from comment #5) > Comment on attachment 384723 [details] > 57398802-part-2.patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384723&action=review > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:283 > > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)YES forAttribute:@"data-exclude" value:@"yes"] autorelease], > > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)NO forAttribute:@"data-exclude" value:@"no"] autorelease], > > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)YES forClass:@"exclude"] autorelease], > > Nit - I don’t think you need the (BOOL) casts here. At one point I had a build issue where it didn't understand these values, and a typecast fixed it. I'll see if these are still required. (In reply to Louie Livon-Bemel from comment #7) > (In reply to Wenson Hsieh from comment #5) > > Comment on attachment 384723 [details] > > 57398802-part-2.patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=384723&action=review > > > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:283 > > > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)YES forAttribute:@"data-exclude" value:@"yes"] autorelease], > > > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)NO forAttribute:@"data-exclude" value:@"no"] autorelease], > > > + [[[_WKTextManipulationExclusionRule alloc] initExclusion:(BOOL)YES forClass:@"exclude"] autorelease], > > > > Nit - I don’t think you need the (BOOL) casts here. > > At one point I had a build issue where it didn't understand these values, > and a typecast fixed it. I'll see if these are still required. Testing this now, I see no issues when removing the typecast. It looks like the tests Ryosuke originally added include the typecast so that's probably why I did too. I'll remove them from my tests though. Created attachment 384820 [details]
57398802-part-3.patch
Comment on attachment 384820 [details] 57398802-part-3.patch Clearing flags on attachment: 384820 Committed r253112: <https://trac.webkit.org/changeset/253112> All reviewed patches have been landed. Closing bug. |