rdar://92348202
Created attachment 458640 [details] For EWS
Created attachment 458642 [details] Patch
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(); } ```
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(); }); } ```
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().
Created attachment 458651 [details] Patch
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?
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()`.
Created attachment 458697 [details] Patch for landing
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].