Bug 224126 – [macOS] Image preview context menu action should be shown conditionally

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

[Wenson Hsieh]

.

[Radar WebKit Bug Importer]

<rdar://problem/76162272>

[Wenson Hsieh]

Created attachment 425051 [details]
Patch

[Wenson Hsieh]

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.

[Devin Rousso]

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?

[Wenson Hsieh]

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

[Wenson Hsieh]

Created attachment 425236 [details]
Patch for landing

[EWS]

Committed r275491: <https://commits.webkit.org/r275491>

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

[Darin Adler]

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.

[Wenson Hsieh]

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.

[Darin Adler]

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?

[Wenson Hsieh]

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

[Darin Adler]

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!

[Wenson Hsieh]

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

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 425334 [details]
Address some comments

[EWS]

Committed r275572: <https://commits.webkit.org/r275572>

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

[Darin Adler]

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.