[webkit-reviews] review granted: [Bug 224126] [macOS] Image preview context menu action should be shown conditionally : [Attachment 425051] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 224126: [macOS] Image preview context menu action should be shown
conditionally
https://bugs.webkit.org/show_bug.cgi?id=224126

Attachment 425051: Patch

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 425051
  --> https://bugs.webkit.org/attachment.cgi?id=425051
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?