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

Wenson Hsieh
Reported 2022-04-20 11:05:04 PDT
rdar://91622151
Attachments
Reupload (29.93 KB, patch)
2022-04-20 14:59 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
For EWS (30.20 KB, patch)
2022-04-20 16:06 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2022-04-20 14:20:25 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 2 2022-04-20 14:22:49 PDT Comment hidden (obsolete)
Brent Fulgham
Comment 3 2022-04-20 14:30:15 PDT
The content of attachment 458015 [details] has been deleted
Wenson Hsieh
Comment 4 2022-04-20 14:59:28 PDT
Created attachment 458017 [details] Reupload
Aditya Keerthi
Comment 5 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.
Wenson Hsieh
Comment 6 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.
Wenson Hsieh
Comment 7 2022-04-20 16:06:27 PDT
Created attachment 458019 [details] For EWS
EWS
Comment 8 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].
Note You need to log in before you can comment on or make changes to this bug.