Bug 239555 – Avoid falling back to snapshots for transparent images when triggering batch text recognition

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

rdar://91622151

[Wenson Hsieh]

Created attachment 458015 [details]
Patch

[Wenson Hsieh]

Created attachment 458016 [details]
Patch

[Brent Fulgham]

The content of attachment 458015 [details] has been deleted

[Wenson Hsieh]

Created attachment 458017 [details]
Reupload

[Aditya Keerthi]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 458019 [details]
For EWS

[EWS]

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