rdar://92239384
Created attachment 458271 [details] Patch
Comment on attachment 458271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458271&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:1194 > #if ENABLE(CONTEXT_MENUS) > m_activeContextMenu = nullptr; > -#endif > +#if ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) > + m_croppedImageForContextMenu = nullptr; > +#endif // ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) > +#endif // ENABLE(CONTEXT_MENUS) I find && works much better than nesting for #if since we don’t indent. Even when it’s right next to another block. Also, #endif comments are so heavy for short sequences. So I would have written: #if ENABLE(CONTEXT_MENUS) m_activeContextMenu = nullptr; #endif #if ENABLE(CONTEXT_MENUS) && ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) m_croppedImageForContextMenu = nullptr #endif Even if, below, we use nesting instead of &&. Not sure about the order, I noticed elsewhere you used the reverse order. > Source/WebKit/UIProcess/WebPageProxy.h:2800 > #if ENABLE(CONTEXT_MENUS) > RefPtr<WebContextMenuProxy> m_activeContextMenu; > ContextMenuContextData m_activeContextMenuContextData; > -#endif > +#if ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) > + RetainPtr<CGImageRef> m_croppedImageForContextMenu; > +#endif // ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) > +#endif // ENABLE(CONTEXT_MENUS) Same here as above: #if ENABLE(CONTEXT_MENUS) RefPtr<WebContextMenuProxy> m_activeContextMenu; ContextMenuContextData m_activeContextMenuContextData; #endif #if ENABLE(CONTEXT_MENUS) && ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) RetainPtr<CGImageRef> m_croppedImageForContextMenu; #endif > Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:609 > + auto action = item.action(); Maybe a switch here instead of two if statements? I like how that eliminates the local variable.
Comment on attachment 458271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458271&action=review Thanks for the review! >> Source/WebKit/UIProcess/WebPageProxy.cpp:1194 >> +#endif // ENABLE(CONTEXT_MENUS) > > I find && works much better than nesting for #if since we don’t indent. Even when it’s right next to another block. Also, #endif comments are so heavy for short sequences. So I would have written: > > #if ENABLE(CONTEXT_MENUS) > m_activeContextMenu = nullptr; > #endif > > #if ENABLE(CONTEXT_MENUS) && ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) > m_croppedImageForContextMenu = nullptr > #endif > > Even if, below, we use nesting instead of &&. Not sure about the order, I noticed elsewhere you used the reverse order. Sounds good! Changed to use `ENABLE(CONTEXT_MENUS) && ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS)` here. >> Source/WebKit/UIProcess/WebPageProxy.h:2800 >> +#endif // ENABLE(CONTEXT_MENUS) > > Same here as above: > > #if ENABLE(CONTEXT_MENUS) > RefPtr<WebContextMenuProxy> m_activeContextMenu; > ContextMenuContextData m_activeContextMenuContextData; > #endif > > #if ENABLE(CONTEXT_MENUS) && ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) > RetainPtr<CGImageRef> m_croppedImageForContextMenu; > #endif 👍🏻 >> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:609 >> + auto action = item.action(); > > Maybe a switch here instead of two if statements? I like how that eliminates the local variable. Makes sense — changed to use a switch case here.
Created attachment 458280 [details] Patch
Created attachment 458281 [details] Patch
Committed r293340 (249961@main): <https://commits.webkit.org/249961@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458281 [details].
Oops, this broke the macOS Montery builds for me: /Users/nzimmermann/Software/GitRepositories/WebKitVanilla/Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:661:27: error: no member named 'setCroppedImageForContextMenu' in 'WebKit::WebPageProxy' page->setCroppedImageForContextMenu(nullptr); ~~~~~~^ /Users/nzimmermann/Software/GitRepositories/WebKitVanilla/Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:670:31: error: no member named 'setCroppedImageForContextMenu' in 'WebKit::WebPageProxy' page->setCroppedImageForContextMenu(result); ~~~~~~^ /Users/nzimmermann/Software/GitRepositories/WebKitVanilla/Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:662:21: error: use of undeclared identifier 'requestImageAnalysisMarkup' requestImageAnalysisMarkup(image.get(), [weakPage, protectedThis, copyCroppedImageItem = WTFMove(*copyCroppedImageItem)](auto result, auto) { ^ 3 errors generated. ... Th relevant code section is only guarded by "#if ENABLE(IMAGE_ANALYSIS)" - I guess you meant to test 'IMAGE_ANALYSIS_ENHANCEMENTS' instead? #if ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) void WebPageProxy::setCroppedImageForContextMenu(CGImageRef image) { ...
(In reply to Nikolas Zimmermann from comment #7) > Oops, this broke the macOS Montery builds for me: > > /Users/nzimmermann/Software/GitRepositories/WebKitVanilla/Source/WebKit/ > UIProcess/mac/WebContextMenuProxyMac.mm:661:27: error: no member named > 'setCroppedImageForContextMenu' in > 'WebKit::WebPageProxy' > page->setCroppedImageForContextMenu(nullptr); > ~~~~~~^ > /Users/nzimmermann/Software/GitRepositories/WebKitVanilla/Source/WebKit/ > UIProcess/mac/WebContextMenuProxyMac.mm:670:31: error: no member named > 'setCroppedImageForContextMenu' in > 'WebKit::WebPageProxy' > page->setCroppedImageForContextMenu(result); > ~~~~~~^ > /Users/nzimmermann/Software/GitRepositories/WebKitVanilla/Source/WebKit/ > UIProcess/mac/WebContextMenuProxyMac.mm:662:21: error: use of undeclared > identifier > 'requestImageAnalysisMarkup' > requestImageAnalysisMarkup(image.get(), [weakPage, > protectedThis, copyCroppedImageItem = WTFMove(*copyCroppedImageItem)](auto > result, auto) { > ^ > 3 errors generated. > ... > > Th relevant code section is only guarded by "#if ENABLE(IMAGE_ANALYSIS)" - I > guess you meant to test 'IMAGE_ANALYSIS_ENHANCEMENTS' instead? > > #if ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) > void WebPageProxy::setCroppedImageForContextMenu(CGImageRef image) > { > ... Sorry about that — I'm landing a fix for that momentarily!
Comment on attachment 458281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458281&action=review > Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:659 > + if (copyCroppedImageItem) { The whole if body needs to be guarded by #if ENABLE(IMAGE_ANALYSIS_ENHANCMENTS) -- not sure why EWS is not affected - I'm using 'vanilla' build options macOS Monterey debug / release builds. Otherwise setCroppedImageForContextMenu() is undefined.
(In reply to Wenson Hsieh from comment #8) > Sorry about that — I'm landing a fix for that momentarily! No worries, thanks for the prompt fix.
(In reply to Nikolas Zimmermann from comment #9) > Comment on attachment 458281 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458281&action=review > > > Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:659 > > + if (copyCroppedImageItem) { > > The whole if body needs to be guarded by #if > ENABLE(IMAGE_ANALYSIS_ENHANCMENTS) -- not sure why EWS is not affected - I'm > using 'vanilla' build options macOS Monterey debug / release builds. > Otherwise setCroppedImageForContextMenu() is undefined. Indeed — I'm changing it to this: ``` #if ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS) if (copyCroppedImageItem) { … } #else UNUSED_PARAM(copyCroppedImageItem); #endif ``` EWS didn't catch this, because the EWS bots are only on Big Sur.
Committed r293363: <https://commits.webkit.org/r293363>