rdar://77522786
Created attachment 431043 [details] For EWS
Created attachment 431048 [details] For EWS
Comment on attachment 431048 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=431048&action=review r=me, nice! > Source/WebCore/html/HTMLElement.cpp:1513 > + if (auto* page = document().page(); page && cacheTextRecognitionResults == CacheTextRecognitionResults::Yes) NIT: Seems a bit odd to me that we always fetch `page` but then only use it based on something completely unrelated to `page`. I'd maybe restructure this into two nested `if` so that if someone adds to this in the future they're less likely to miss it. > Source/WebCore/page/Page.cpp:3622 > + m_textRecognitionResultsByElement.removeAllMatching([&](auto& elementAndResult) { NIT: Could this just be `[]` instead? > Source/WebCore/page/Page.cpp:3626 > + Vector<std::pair<Ref<HTMLElement>, TextRecognitionResult>> elementsToUpdate; unused > Source/WebCore/page/Page.cpp:3627 > + for (auto& [element, resultAndSize] : m_textRecognitionResultsByElement) { NIT: this would probably be a little uglier, but I wonder if you could move this logic into `m_textRecognitionResultsByElement` so we don't have to iterate twice > Source/WebCore/page/Page.cpp:3660 > + m_textRecognitionResultsByElement.removeAllMatching([&](auto& elementAndResult) { ditto (:3627) > Source/WebCore/page/Page.h:1193 > + Vector<std::pair<WeakPtr<HTMLElement>, std::pair<TextRecognitionResult, IntSize>>> m_textRecognitionResultsByElement; NIT: Maybe add a FIXME comment to change this to a `WeakHashMap` when implemented? > LayoutTests/fast/images/text-recognition/image-overlay-size-change.html:48 > + await new Promise(resolve => shouldBecomeEqual("width", "50", resolve)); FYI `shouldBecomeEqual` now supports returning a `Promise` after r278678 :)
Comment on attachment 431048 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=431048&action=review Thanks for the review! >> Source/WebCore/html/HTMLElement.cpp:1513 >> + if (auto* page = document().page(); page && cacheTextRecognitionResults == CacheTextRecognitionResults::Yes) > > NIT: Seems a bit odd to me that we always fetch `page` but then only use it based on something completely unrelated to `page`. I'd maybe restructure this into two nested `if` so that if someone adds to this in the future they're less likely to miss it. That's a good point — changed this to wrap inside an `if (cacheTextRecognitionResults == CacheTextRecognitionResults::Yes)`. >> Source/WebCore/page/Page.cpp:3622 >> + m_textRecognitionResultsByElement.removeAllMatching([&](auto& elementAndResult) { > > NIT: Could this just be `[]` instead? 👍🏻 >> Source/WebCore/page/Page.cpp:3626 >> + Vector<std::pair<Ref<HTMLElement>, TextRecognitionResult>> elementsToUpdate; > > unused Whoops, good catch! >> Source/WebCore/page/Page.cpp:3627 >> + for (auto& [element, resultAndSize] : m_textRecognitionResultsByElement) { > > NIT: this would probably be a little uglier, but I wonder if you could move this logic into `m_textRecognitionResultsByElement` so we don't have to iterate twice I'll leave it as-is for now; this is going to be removed once we use a weak hashmap anyways. >> Source/WebCore/page/Page.cpp:3660 >> + m_textRecognitionResultsByElement.removeAllMatching([&](auto& elementAndResult) { > > ditto (:3627) 👍🏻 >> Source/WebCore/page/Page.h:1193 >> + Vector<std::pair<WeakPtr<HTMLElement>, std::pair<TextRecognitionResult, IntSize>>> m_textRecognitionResultsByElement; > > NIT: Maybe add a FIXME comment to change this to a `WeakHashMap` when implemented? 👍🏻 >> LayoutTests/fast/images/text-recognition/image-overlay-size-change.html:48 >> + await new Promise(resolve => shouldBecomeEqual("width", "50", resolve)); > > FYI `shouldBecomeEqual` now supports returning a `Promise` after r278678 :) V. nice
Created attachment 431116 [details] Patch
Committed r278747 (238708@main): <https://commits.webkit.org/238708@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431116 [details].