Bug 226274

Summary: [macOS] Show context menu when clicking on data detection results in image overlays
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, akeerthi, bdakin, hi, megan_gardner, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 226267, 226292    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Rebase on trunk
ews-feeder: commit-queue-
Fix non-internal macOS build
ews-feeder: commit-queue-
Fix non-internal macOS build (2)
ews-feeder: commit-queue-
Fix non-internal build (3)
none
For EWS
ews-feeder: commit-queue-
Patch for landing none

Description Wenson Hsieh 2021-05-26 08:36:42 PDT 
rdar://75504956
Comment 1 Wenson Hsieh 2021-05-27 08:46:20 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-05-27 13:26:51 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-05-27 13:56:21 PDT Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2021-05-27 14:07:13 PDT Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2021-05-27 14:29:17 PDT 
Created attachment 429928 [details]
Fix non-internal build (3)
Comment 6 Aditya Keerthi 2021-05-27 15:12:48 PDT 
Comment on attachment 429928 [details]
Fix non-internal build (3)

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:149
> +#import <pal/cocoa/RevealSoftLink.h>

Nit: Move near the other PAL headers above?

> Source/WebKit/UIProcess/mac/WKRevealItemPresenter.h:41
> +- (instancetype)initWithWebViewImpl:(WebKit::WebViewImpl&)webViewImpl item:(RVItem *)item frame:(CGRect)frameInView menuLocation:(CGPoint)menuLocationInView;

const WebKit::WebViewImpl&?

> Source/WebKit/UIProcess/mac/WKRevealItemPresenter.mm:36
> +#import <pal/cocoa/RevealSoftLink.h>

Nit: Move PAL header before WTF header.
Comment 7 Tim Horton 2021-05-27 15:19:20 PDT 
Comment on attachment 429928 [details]
Fix non-internal build (3)

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

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:149
>> +#import <pal/cocoa/RevealSoftLink.h>
> 
> Nit: Move near the other PAL headers above?

SoftLinking headers go last, the stylebot enforces it (I think there is a non-style reason, but I don't remember what it is)
Comment 8 Wenson Hsieh 2021-05-27 15:21:36 PDT 
Comment on attachment 429928 [details]
Fix non-internal build (3)

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

Thanks for the reviews!

>>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:149
>>> +#import <pal/cocoa/RevealSoftLink.h>
>> 
>> Nit: Move near the other PAL headers above?
> 
> SoftLinking headers go last, the stylebot enforces it (I think there is a non-style reason, but I don't remember what it is)

Done!

>> Source/WebKit/UIProcess/mac/WKRevealItemPresenter.h:41
>> +- (instancetype)initWithWebViewImpl:(WebKit::WebViewImpl&)webViewImpl item:(RVItem *)item frame:(CGRect)frameInView menuLocation:(CGPoint)menuLocationInView;
> 
> const WebKit::WebViewImpl&?

👍🏻 Made this `const`

>> Source/WebKit/UIProcess/mac/WKRevealItemPresenter.mm:36
>> +#import <pal/cocoa/RevealSoftLink.h>
> 
> Nit: Move PAL header before WTF header.

So doing this results in this style failure:

```
% check-webkit-style
ERROR: Source/WebKit/UIProcess/mac/WKRevealItemPresenter.mm:35:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/mac/WKRevealItemPresenter.mm:36:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
```

…which prompted me to move the header down below the others. I'm not exactly sure why WebViewImpl.mm doesn't yield a similar error, though 🤔.

I think I'll leave this one here.
Comment 9 Wenson Hsieh 2021-05-27 15:47:20 PDT 
Created attachment 429944 [details]
For EWS
Comment 10 Wenson Hsieh 2021-05-27 20:15:26 PDT 
Created attachment 429976 [details]
Patch for landing
Comment 11 EWS 2021-05-27 20:34:52 PDT 
Patch 429944 does not build
Comment 12 EWS 2021-05-27 22:10:57 PDT 
Committed r278190 (238233@main): <https://commits.webkit.org/238233@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429976 [details].