Bug 224126 - [macOS] Image preview context menu action should be shown conditionally
Summary: [macOS] Image preview context menu action should be shown conditionally
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:
Blocks:
 
Reported: 2021-04-02 13:08 PDT by Wenson Hsieh
Modified: 2021-04-06 16:59 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.35 KB, patch)
2021-04-02 14:01 PDT, Wenson Hsieh
drousso: review+
Details | Formatted Diff | Diff
Patch for landing (15.47 KB, patch)
2021-04-05 20:32 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Address some comments (3.96 KB, patch)
2021-04-06 16:14 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-04-02 13:08:52 PDT
.
Comment 1 Radar WebKit Bug Importer 2021-04-02 13:21:20 PDT
<rdar://problem/76162272>
Comment 2 Wenson Hsieh 2021-04-02 14:01:45 PDT
Created attachment 425051 [details]
Patch
Comment 3 Wenson Hsieh 2021-04-02 17:25:03 PDT
Comment on attachment 425051 [details]
Patch

The test failure (media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag-is-prevented-over-button.html) does not seem related.
Comment 4 Devin Rousso 2021-04-05 19:18:01 PDT
Comment on attachment 425051 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:537
> +    auto insertMenuItem = makeBlockPtr([protectedThis = makeRef(*this), weakPage = makeWeakPtr(page()), imageURL, imageBitmap, completionHandler = WTFMove(completionHandler), itemsRemaining = filteredItems.size(), menu = WTFMove(menu), sparseMenuItems, revealImageItem](NSMenuItem *item, NSUInteger index) mutable {

NIT: Should this have `imageURL = WTFMove(imageURL)` and `revealImageItem = WTFMove(revealImageItem)`?  Maybe even `imageBitmap = WTFMove(imageBitmap)` too (dunno if it's needed on the `WebHitTestResultData` still)?

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:549
> +            page->computeCanRevealImage(imageURL, *imageBitmap, [protectedThis = WTFMove(protectedThis), menuItem = WTFMove(*revealImageItem)] (bool canRevealImage) mutable {

NIT: Can we keep this named `revealImageItem`?  Or does it complain about the same name having a different type?
Comment 5 Wenson Hsieh 2021-04-05 20:17:40 PDT
Comment on attachment 425051 [details]
Patch

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

Thanks for the review!

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:537
>> +    auto insertMenuItem = makeBlockPtr([protectedThis = makeRef(*this), weakPage = makeWeakPtr(page()), imageURL, imageBitmap, completionHandler = WTFMove(completionHandler), itemsRemaining = filteredItems.size(), menu = WTFMove(menu), sparseMenuItems, revealImageItem](NSMenuItem *item, NSUInteger index) mutable {
> 
> NIT: Should this have `imageURL = WTFMove(imageURL)` and `revealImageItem = WTFMove(revealImageItem)`?  Maybe even `imageBitmap = WTFMove(imageBitmap)` too (dunno if it's needed on the `WebHitTestResultData` still)?

Sounds good! Made these use move semantics.

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:549
>> +            page->computeCanRevealImage(imageURL, *imageBitmap, [protectedThis = WTFMove(protectedThis), menuItem = WTFMove(*revealImageItem)] (bool canRevealImage) mutable {
> 
> NIT: Can we keep this named `revealImageItem`?  Or does it complain about the same name having a different type?

Yes, we can keep the name — renamed to `revealImageItem`.
Comment 6 Wenson Hsieh 2021-04-05 20:32:28 PDT
Created attachment 425236 [details]
Patch for landing
Comment 7 EWS 2021-04-05 21:31:17 PDT
Committed r275491: <https://commits.webkit.org/r275491>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425236 [details].
Comment 8 Darin Adler 2021-04-06 15:18:57 PDT
Comment on attachment 425236 [details]
Patch for landing

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

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:485
> +        auto wrapper = adoptNS([[WKUserDataWrapper alloc] initWithUserData:item.userData()]);
> +        [menuItem setRepresentedObject:wrapper.get()];

Seems like this reads better as a one-liner without the local variable.

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:533
> +    auto imageURL = URL([NSURL _web_URLWithWTFString:m_context.webHitTestResultData().absoluteImageURL]);

This could be written like this:

    auto imageURL = URL { URL { }, m_context.webHitTestResultData().absoluteImageURL };

No reason to involve NSURL.

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:536
> +    auto sparseMenuItems = retainPtr([NSPointerArray strongObjectsPointerArray]);

Not sure why we do retainPtr here.

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:554
> +                auto nsMenuItem = createMenuActionItem(revealImageItem);
> +                [protectedThis->m_menu addItem:nsMenuItem.get()];

Seems like this reads better as a one-liner without the local variable.

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:585
> +        auto nsMenuItem = createMenuActionItem(item);
> +        completionHandler(nsMenuItem.get());

Seems like this reads better as a one-liner without the local variable. Then we also don’t need the braces.
Comment 9 Wenson Hsieh 2021-04-06 15:26:22 PDT
Comment on attachment 425236 [details]
Patch for landing

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

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:485
>> +        [menuItem setRepresentedObject:wrapper.get()];
> 
> Seems like this reads better as a one-liner without the local variable.

Changed to a one-liner.

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:533
>> +    auto imageURL = URL([NSURL _web_URLWithWTFString:m_context.webHitTestResultData().absoluteImageURL]);
> 
> This could be written like this:
> 
>     auto imageURL = URL { URL { }, m_context.webHitTestResultData().absoluteImageURL };
> 
> No reason to involve NSURL.

Sounds good, changed to use URL only (this means we can also avoid importing `WKNSURLExtras.h`).

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:536
>> +    auto sparseMenuItems = retainPtr([NSPointerArray strongObjectsPointerArray]);
> 
> Not sure why we do retainPtr here.

This code was only moved, but I assume it's because the completion handler below needs to keep this alive while context menu items are being added. Are you suggesting that we should just do `RetainPtr<NSPointerArray>` instead of `auto` + `retainPtr`?

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:554
>> +                [protectedThis->m_menu addItem:nsMenuItem.get()];
> 
> Seems like this reads better as a one-liner without the local variable.

Changed to a one-liner.

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:585
>> +        completionHandler(nsMenuItem.get());
> 
> Seems like this reads better as a one-liner without the local variable. Then we also don’t need the braces.

Changed to a one-liner.
Comment 10 Darin Adler 2021-04-06 15:42:01 PDT
Comment on attachment 425236 [details]
Patch for landing

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

>>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:536
>>> +    auto sparseMenuItems = retainPtr([NSPointerArray strongObjectsPointerArray]);
>> 
>> Not sure why we do retainPtr here.
> 
> This code was only moved, but I assume it's because the completion handler below needs to keep this alive while context menu items are being added. Are you suggesting that we should just do `RetainPtr<NSPointerArray>` instead of `auto` + `retainPtr`?

No, I do not think the completion handler needs to use a RetainPtr to keep this object alive. The object is autoreleased and the function hasn’t returned yet. These are synchronous callbacks. The code relies on that, doesn’t it?
Comment 11 Wenson Hsieh 2021-04-06 15:51:46 PDT
Comment on attachment 425236 [details]
Patch for landing

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

>>>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:536
>>>> +    auto sparseMenuItems = retainPtr([NSPointerArray strongObjectsPointerArray]);
>>> 
>>> Not sure why we do retainPtr here.
>> 
>> This code was only moved, but I assume it's because the completion handler below needs to keep this alive while context menu items are being added. Are you suggesting that we should just do `RetainPtr<NSPointerArray>` instead of `auto` + `retainPtr`?
> 
> No, I do not think the completion handler needs to use a RetainPtr to keep this object alive. The object is autoreleased and the function hasn’t returned yet. These are synchronous callbacks. The code relies on that, doesn’t it?

While most of the default items yield results synchronously, there appear to be other items (e.g. the share menu item) that may call the completion handler asynchronously, since it uses `-[NSSharingServicePicker getMenuWithCompletion:]`.
Comment 12 Darin Adler 2021-04-06 16:01:06 PDT
Comment on attachment 425236 [details]
Patch for landing

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

>>>>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:536
>>>>> +    auto sparseMenuItems = retainPtr([NSPointerArray strongObjectsPointerArray]);
>>>> 
>>>> Not sure why we do retainPtr here.
>>> 
>>> This code was only moved, but I assume it's because the completion handler below needs to keep this alive while context menu items are being added. Are you suggesting that we should just do `RetainPtr<NSPointerArray>` instead of `auto` + `retainPtr`?
>> 
>> No, I do not think the completion handler needs to use a RetainPtr to keep this object alive. The object is autoreleased and the function hasn’t returned yet. These are synchronous callbacks. The code relies on that, doesn’t it?
> 
> While most of the default items yield results synchronously, there appear to be other items (e.g. the share menu item) that may call the completion handler asynchronously, since it uses `-[NSSharingServicePicker getMenuWithCompletion:]`.

I see now, this was just too subtle for me. The magic where insertMenuItem decrements itemsRemaining until it hits zero and then finally we call completionHandler. Oblique! And every one of the calls to getContextMenuItem copies the insertMenuItem block, and thus makes N copies of everything it captures. Not very efficient!
Comment 13 Wenson Hsieh 2021-04-06 16:10:48 PDT
(In reply to Darin Adler from comment #12)
> Comment on attachment 425236 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425236&action=review
> 
> >>>>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:536
> >>>>> +    auto sparseMenuItems = retainPtr([NSPointerArray strongObjectsPointerArray]);
> >>>> 
> >>>> Not sure why we do retainPtr here.
> >>> 
> >>> This code was only moved, but I assume it's because the completion handler below needs to keep this alive while context menu items are being added. Are you suggesting that we should just do `RetainPtr<NSPointerArray>` instead of `auto` + `retainPtr`?
> >> 
> >> No, I do not think the completion handler needs to use a RetainPtr to keep this object alive. The object is autoreleased and the function hasn’t returned yet. These are synchronous callbacks. The code relies on that, doesn’t it?
> > 
> > While most of the default items yield results synchronously, there appear to be other items (e.g. the share menu item) that may call the completion handler asynchronously, since it uses `-[NSSharingServicePicker getMenuWithCompletion:]`.
> 
> I see now, this was just too subtle for me. The magic where insertMenuItem
> decrements itemsRemaining until it hits zero and then finally we call
> completionHandler. Oblique! And every one of the calls to getContextMenuItem
> copies the insertMenuItem block, and thus makes N copies of everything it
> captures. Not very efficient!

That's a good point..I think it should be possible to avoid all this copying by introducing something like a ref-counted helper class that encapsulates all of the captured state (e.g., `ContextMenuItemAccumulator`) and just copy around a `Ref` of that instead for each of the `getContextMenuItem` calls.
Comment 14 Wenson Hsieh 2021-04-06 16:14:35 PDT
Reopening to attach new patch.
Comment 15 Wenson Hsieh 2021-04-06 16:14:36 PDT
Created attachment 425334 [details]
Address some comments
Comment 16 EWS 2021-04-06 16:54:12 PDT
Committed r275572: <https://commits.webkit.org/r275572>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425334 [details].
Comment 17 Darin Adler 2021-04-06 16:59:30 PDT
Comment on attachment 425236 [details]
Patch for landing

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

>>>>>>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:536
>>>>>>> +    auto sparseMenuItems = retainPtr([NSPointerArray strongObjectsPointerArray]);
>>>>>> 
>>>>>> Not sure why we do retainPtr here.
>>>>> 
>>>>> This code was only moved, but I assume it's because the completion handler below needs to keep this alive while context menu items are being added. Are you suggesting that we should just do `RetainPtr<NSPointerArray>` instead of `auto` + `retainPtr`?
>>>> 
>>>> No, I do not think the completion handler needs to use a RetainPtr to keep this object alive. The object is autoreleased and the function hasn’t returned yet. These are synchronous callbacks. The code relies on that, doesn’t it?
>>> 
>>> While most of the default items yield results synchronously, there appear to be other items (e.g. the share menu item) that may call the completion handler asynchronously, since it uses `-[NSSharingServicePicker getMenuWithCompletion:]`.
>> 
>> I see now, this was just too subtle for me. The magic where insertMenuItem decrements itemsRemaining until it hits zero and then finally we call completionHandler. Oblique! And every one of the calls to getContextMenuItem copies the insertMenuItem block, and thus makes N copies of everything it captures. Not very efficient!
> 
> That's a good point..I think it should be possible to avoid all this copying by introducing something like a ref-counted helper class that encapsulates all of the captured state (e.g., `ContextMenuItemAccumulator`) and just copy around a `Ref` of that instead for each of the `getContextMenuItem` calls.

I was wrong about this, too. There is only one insertMenuItem block and it is reference counted. That’s what makeBlockPtr does. Otherwise this wouldn’t work because each copy of the insertMenuItem block would have its own copy of the itemsRemaining integer.

We could put the retainPtr in the capture expression instead of above:

    imageBitMap = retainPtr(imageBitmap),

instead of

    imageBitMap = WTFMove(imageBitmap),

But really, no change needed. This is just very tricky code! Would be nice to find a more straightforward way to write it.