Bug 226858

Summary: [Live Text] Add a mechanism to regenerate text in an image element when it changes dimensions
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, megan_gardner, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
For EWS
none
For EWS
none
Patch none

Description Wenson Hsieh 2021-06-09 16:31:30 PDT
rdar://77522786
Comment 1 Wenson Hsieh 2021-06-09 19:30:04 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-06-09 21:06:27 PDT
Created attachment 431048 [details]
For EWS
Comment 3 Devin Rousso 2021-06-10 09:49:32 PDT
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 4 Wenson Hsieh 2021-06-10 11:39:21 PDT
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
Comment 5 Wenson Hsieh 2021-06-10 13:12:56 PDT
Created attachment 431116 [details]
Patch
Comment 6 EWS 2021-06-10 18:22:59 PDT
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].