Bug 226274 - [macOS] Show context menu when clicking on data detection results in image overlays
Summary: [macOS] Show context menu when clicking on data detection results in image ov...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 226267 226292
Blocks:
  Show dependency tree / graph
 
Reported: 2021-05-26 08:36 PDT by Wenson Hsieh
Modified: 2021-05-27 22:10 PDT (History)
7 users (show)

See Also:

Attachments
Patch (39.83 KB, patch)
2021-05-27 08:46 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Rebase on trunk (40.12 KB, patch)
2021-05-27 13:26 PDT, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix non-internal macOS build (42.19 KB, patch)
2021-05-27 13:56 PDT, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix non-internal macOS build (2) (42.49 KB, patch)
2021-05-27 14:07 PDT, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix non-internal build (3) (42.51 KB, patch)
2021-05-27 14:29 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
For EWS (42.70 KB, patch)
2021-05-27 15:47 PDT, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (42.78 KB, patch)
2021-05-27 20:15 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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].