Image overlay code is fairly self-contained, and it would give us better compartmentalization if it were in its own header/compilation unit (instead of drifting around in HTMLElement.cpp)
The existing helpers on HTMLElement should belong in a separate header/compilation unit, instead of drifting around in HTMLElement.(h|cpp)
Created attachment 443953 [details] For EWS
Created attachment 443958 [details] Fix Windows build
Comment on attachment 443958 [details] Fix Windows build View in context: https://bugs.webkit.org/attachment.cgi?id=443958&action=review > Source/WebCore/ChangeLog:10 > + Move code for updating, querying, and removing image overlays out of HTMLElement.cpp and HTMLElement.h and into > + separate helper files instead (ImageOverlay.h and ImageOverlay.cpp). Future patches in this area will extend the > + functionality of these overlays, so this will help avoid code bloat inside HTMLElement when doing so. Nice! > Source/WebCore/dom/ImageOverlay.cpp:168 > +void updateWithTextRecognitionResult(HTMLElement& element, const TextRecognitionResult& result, CacheTextRecognitionResults cacheTextRecognitionResults) Would be nice to chop up this mega-function into smaller pieces at some point.
Comment on attachment 443958 [details] Fix Windows build View in context: https://bugs.webkit.org/attachment.cgi?id=443958&action=review > Source/WebCore/dom/ImageOverlay.h:50 > +WEBCORE_EXPORT bool hasImageOverlay(const HTMLElement&); > +WEBCORE_EXPORT bool isImageOverlayDataDetectorResult(const HTMLElement&); > +WEBCORE_EXPORT bool isInsideImageOverlay(const SimpleRange&); > +WEBCORE_EXPORT bool isInsideImageOverlay(const Node&); > +WEBCORE_EXPORT bool isImageOverlayText(const Node&); > +WEBCORE_EXPORT bool isImageOverlayText(const Node*); > +void removeImageOverlaySoonIfNeeded(HTMLElement&); > +IntRect containerRectForImageOverlay(HTMLElement&); > + > +#if ENABLE(IMAGE_ANALYSIS) > +enum class CacheTextRecognitionResults : bool { No, Yes }; > +WEBCORE_EXPORT void updateWithTextRecognitionResult(HTMLElement&, const TextRecognitionResult&, CacheTextRecognitionResults = CacheTextRecognitionResults::Yes); > +#endif You could consider wrapping these functions into a namespace or a class (say "ImageOverlay").
Comment on attachment 443958 [details] Fix Windows build View in context: https://bugs.webkit.org/attachment.cgi?id=443958&action=review Thanks for the review! >> Source/WebCore/dom/ImageOverlay.cpp:168 >> +void updateWithTextRecognitionResult(HTMLElement& element, const TextRecognitionResult& result, CacheTextRecognitionResults cacheTextRecognitionResults) > > Would be nice to chop up this mega-function into smaller pieces at some point. Good point! I'll do this refactoring as part of a future patch that'll augment this code. >> Source/WebCore/dom/ImageOverlay.h:50 >> +#endif > > You could consider wrapping these functions into a namespace or a class (say "ImageOverlay"). Makes sense — I'll wrap these helpers in a `namespace ImageOverlay` in this patch.
Created attachment 443975 [details] Add namespace, fix CMake builds
Tools/Scripts/svn-apply failed to apply attachment 443975 [details] to trunk. Please resolve the conflicts and upload a new patch.
Committed r285655 (244153@main): <https://commits.webkit.org/244153@main>
<rdar://problem/85314337>