Bug 239555

Summary: Avoid falling back to snapshots for transparent images when triggering batch text recognition
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, akeerthi, andresg_22, annulen, apinheiro, bfulgham, cfleizach, dmazzoni, ews-watchlist, gyuyoung.kim, hi, jcraig, jdiggs, katherine_cheney, megan_gardner, ryuan.choi, samuel_white, sergio, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Reupload
none
For EWS none

Description Wenson Hsieh 2022-04-20 11:05:04 PDT 
rdar://91622151
Comment 1 Wenson Hsieh 2022-04-20 14:20:25 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2022-04-20 14:22:49 PDT Comment hidden (obsolete)
Comment 3 Brent Fulgham 2022-04-20 14:30:15 PDT 
The content of attachment 458015 [details] has been deleted
Comment 4 Wenson Hsieh 2022-04-20 14:59:28 PDT 
Created attachment 458017 [details]
Reupload
Comment 5 Aditya Keerthi 2022-04-20 15:18:24 PDT 
Comment on attachment 458017 [details]
Reupload

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

> Source/WebCore/platform/TextRecognitionOptions.h:32
> +enum class AllowSnapshots : bool { No, Yes };

Consider moving this inside `TextRecognitionOptions` or renaming to something more specific.

`WebCore::AllowSnapshots` seems too generic to be defined in this header.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:107
> +class CGImagePixelReader {

I wonder if we can share this logic with other tests – maybe leave a FIXME here? I think there are some PDF / GraphicsContext tests that also read pixels.
Comment 6 Wenson Hsieh 2022-04-20 15:48:42 PDT 
Comment on attachment 458017 [details]
Reupload

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

Thanks for the review!

>> Source/WebCore/platform/TextRecognitionOptions.h:32
>> +enum class AllowSnapshots : bool { No, Yes };
> 
> Consider moving this inside `TextRecognitionOptions` or renaming to something more specific.
> 
> `WebCore::AllowSnapshots` seems too generic to be defined in this header.

Makes sense — I'll move it into TextRecognitionOptions, as TextRecognitionOptions::AllowSnapshots

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:107
>> +class CGImagePixelReader {
> 
> I wonder if we can share this logic with other tests – maybe leave a FIXME here? I think there are some PDF / GraphicsContext tests that also read pixels.

Good point! It seems the only effective difference between this and the logic in `TestPDFPage::colorAtPoint` is that the latter uses `CGContextDrawPDFPageWithAnnotations`. We could probably abstract this away by adding a constructor that takes a `PDFPage`. I think we could also deploy this in `DragAndDropTestsIOS.mm`, as well.

I'll add a FIXME for this.
Comment 7 Wenson Hsieh 2022-04-20 16:06:27 PDT 
Created attachment 458019 [details]
For EWS
Comment 8 EWS 2022-04-20 18:52:17 PDT 
Committed r293137 (249836@main): <https://commits.webkit.org/249836@main>

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