Bug 226858 – [Live Text] Add a mechanism to regenerate text in an image element when it changes dimensions

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

rdar://77522786

[Wenson Hsieh]

Created attachment 431043 [details]
For EWS

[Wenson Hsieh]

Created attachment 431048 [details]
For EWS

[Devin Rousso]

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 :)

[Wenson Hsieh]

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

[Wenson Hsieh]

Created attachment 431116 [details]
Patch

[EWS]

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].