Bug 239712

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>