Bug 226274 – [macOS] Show context menu when clicking on data detection results in image overlays

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

rdar://75504956

[Wenson Hsieh]

Created attachment 429881 [details]
Patch

[Wenson Hsieh]

Created attachment 429918 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 429923 [details]
Fix non-internal macOS build

[Wenson Hsieh]

Created attachment 429926 [details]
Fix non-internal macOS build (2)

[Wenson Hsieh]

Created attachment 429928 [details]
Fix non-internal build (3)

[Aditya Keerthi]

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.

[Tim Horton]

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)

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 429944 [details]
For EWS

[Wenson Hsieh]

Created attachment 429976 [details]
Patch for landing

[EWS]

Patch 429944 does not build

[EWS]

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].