Bug 239933 – [macOS] The "Markup Image" services menu item should be gated on image analysis results

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

[Wenson Hsieh]

rdar://92348202

[Wenson Hsieh]

Created attachment 458640 [details]
For EWS

[Wenson Hsieh]

Created attachment 458642 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 458642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458642&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/image-controls.html:30
> +    await new Promise(async resolve => {
> +        let button = null;
> +        do {
> +            await new Promise(requestAnimationFrame);
> +            button = servicesControlButton();
> +        } while (!button);
> +        resolve();
> +        button.click();
> +    });

You don't need to wrap a Promise object since you're already in an async function, I think this should just work:

```

async function waitForAndClickImageControls() {
   if (!window.internals)
      return;

   let button = null;
   do {
      await new Promise(requestAnimationFrame);
      button = servicesControlButton();
   } while (!button);
   button.click();
}
```

[Tim Nguyen (:ntim)]

Comment on attachment 458642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458642&action=review

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/image-controls.html:30
>> +    });
> 
> You don't need to wrap a Promise object since you're already in an async function, I think this should just work:
> 
> ```
> 
> async function waitForAndClickImageControls() {
>    if (!window.internals)
>       return;
> 
>    let button = null;
>    do {
>       await new Promise(requestAnimationFrame);
>       button = servicesControlButton();
>    } while (!button);
>    button.click();
> }
> ```

or if you really meant to resolve() before the button click (which is pretty weird IMO):

```
function waitForAndClickImageControls() {
    return new Promise(async resolve => {
        if (!window.internals)
            return resolve();
        let button = null;
        do {
            await new Promise(requestAnimationFrame);
            button = servicesControlButton();
        } while (!button);
        resolve();
        button.click();
    });
}
```

[Wenson Hsieh]

Good point! With the current version of the test, we don't need to resolve the promise before clicking because we'll always dispatch the runloop timer before we start spinning the runloop for the context menu.

Changed to simplify waitForAndClickImageControls().

[Wenson Hsieh]

Created attachment 458651 [details]
Patch

[Kate Cheney]

Comment on attachment 458651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458651&action=review

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:299
> +    if (!page()->preferences().imageAnalysisMarkupEnabled())

Do we need to check for a null page here?

[Wenson Hsieh]

Comment on attachment 458651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458651&action=review

Thanks for the review!

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:299
>> +    if (!page()->preferences().imageAnalysisMarkupEnabled())
> 
> Do we need to check for a null page here?

Ah, so I think we're technically okay here since this method is only called from `setupServicesMenu()` which already dereferences `page()` (and therefore assumes that it's non-null).

That said, since this is a separate method now, adding an extra null check is probably a good idea — I'll add an early return here for `!page()`.

[Wenson Hsieh]

Created attachment 458697 [details]
Patch for landing

[EWS]

Committed r293681 (250181@main): <https://commits.webkit.org/250181@main>

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