Bug 198451

Summary: Remove some unused selection code
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, megan_gardner, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 198549    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Tim Horton 2019-06-01 02:17:14 PDT 
Remove some unused selection code
Comment 1 Tim Horton 2019-06-01 02:17:39 PDT 
Created attachment 371111 [details]
Patch
Comment 2 Tim Horton 2019-06-03 23:12:36 PDT 
Created attachment 371251 [details]
Patch
Comment 3 Megan Gardner 2019-06-04 06:15:34 PDT 
Comment on attachment 371251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371251&action=review

I’m nervous about this, but the bots seem to be green....

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:-559
> -

Are we sure non of this is needed? I thought this assistant was still used in legacy web kit, though it’s mostly broken, it can still be triggered sometimes.

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:-600
> -- (void)showDictionaryFor:(NSString *)selectedTerm fromRect:(CGRect)presentationRect;

I know we use some of the above methods, I’m surprised that these are ok to remove? Are we sure this is ok?

> Source/WebKit/Shared/ios/GestureTypes.h:45
>      PhraseBoundary

Does this need to have values specified, or since It’s an enumeration class jf’s Ok? Removing one in the middle of a list makes me nervous.

> Source/WebKit/UIProcess/ios/WKPDFView.mm:526
>      auto selectionAssistant = adoptNS([[UIWKSelectionAssistant alloc] initWithView:[_hostViewController view]]);

File a radar? I would hope this would be very easy to do, and would clean up a lot.
Comment 4 Wenson Hsieh 2019-06-04 07:37:32 PDT 
Comment on attachment 371251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371251&action=review

>> Source/WebKit/Platform/spi/ios/UIKitSPI.h:-559
>> -
> 
> Are we sure non of this is needed? I thought this assistant was still used in legacy web kit, though it’s mostly broken, it can still be triggered sometimes.

This is WebKit
Comment 5 Wenson Hsieh 2019-06-04 07:39:32 PDT 
Comment on attachment 371251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371251&action=review

>>> Source/WebKit/Platform/spi/ios/UIKitSPI.h:-559
>>> -
>> 
>> Are we sure non of this is needed? I thought this assistant was still used in legacy web kit, though it’s mostly broken, it can still be triggered sometimes.
> 
> This is WebKit

Oops, half-finished comment 🤦🏻‍♂️

This is just a forward declaration in WebKit, so I don't think it will have any impact in legacy WebKit.

>> Source/WebKit/UIProcess/ios/WKPDFView.mm:526
>>      auto selectionAssistant = adoptNS([[UIWKSelectionAssistant alloc] initWithView:[_hostViewController view]]);
> 
> File a radar? I would hope this would be very easy to do, and would clean up a lot.

+1
Comment 6 WebKit Commit Bot 2019-06-04 16:27:36 PDT 
Comment on attachment 371251 [details]
Patch

Clearing flags on attachment: 371251

Committed r246086: <https://trac.webkit.org/changeset/246086>
Comment 7 WebKit Commit Bot 2019-06-04 16:27:38 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-06-04 16:28:17 PDT 
<rdar://problem/51419978>
Comment 9 WebKit Commit Bot 2019-06-04 17:21:05 PDT 
Re-opened since this is blocked by bug 198549
Comment 10 Tim Horton 2019-06-05 11:01:19 PDT 
Tried again in https://trac.webkit.org/changeset/246116/webkit