.
<rdar://problem/76162272>
Created attachment 425051 [details] Patch
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 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 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`.
Created attachment 425236 [details] Patch for landing
Committed r275491: <https://commits.webkit.org/r275491> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425236 [details].
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 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 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 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 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!
(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.
Reopening to attach new patch.
Created attachment 425334 [details] Address some comments
Committed r275572: <https://commits.webkit.org/r275572> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425334 [details].
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.