RESOLVED FIXED 232974
Move image overlay code out of HTMLElement and into a separate helper file
https://bugs.webkit.org/show_bug.cgi?id=232974
Summary Move image overlay code out of HTMLElement and into a separate helper file
Wenson Hsieh
Reported 2021-11-10 17:12:18 PST Comment hidden (obsolete)
Attachments
For EWS (86.39 KB, patch)
2021-11-11 09:03 PST, Wenson Hsieh
ews-feeder: commit-queue-
Fix Windows build (85.87 KB, patch)
2021-11-11 09:23 PST, Wenson Hsieh
ews-feeder: commit-queue-
Add namespace, fix CMake builds (87.58 KB, patch)
2021-11-11 11:04 PST, Wenson Hsieh
ews-feeder: commit-queue-
Wenson Hsieh
Comment 1 2021-11-10 18:29:33 PST
The existing helpers on HTMLElement should belong in a separate header/compilation unit, instead of drifting around in HTMLElement.(h|cpp)
Wenson Hsieh
Comment 2 2021-11-11 09:03:29 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 3 2021-11-11 09:23:01 PST
Created attachment 443958 [details] Fix Windows build
Antti Koivisto
Comment 4 2021-11-11 10:09:40 PST
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.
Antti Koivisto
Comment 5 2021-11-11 10:13:32 PST
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").
Wenson Hsieh
Comment 6 2021-11-11 10:19:42 PST
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.
Wenson Hsieh
Comment 7 2021-11-11 11:04:31 PST
Created attachment 443975 [details] Add namespace, fix CMake builds
EWS
Comment 8 2021-11-11 13:44:58 PST
Tools/Scripts/svn-apply failed to apply attachment 443975 [details] to trunk. Please resolve the conflicts and upload a new patch.
Wenson Hsieh
Comment 9 2021-11-11 13:52:10 PST
Radar WebKit Bug Importer
Comment 10 2021-11-11 13:53:31 PST
Note You need to log in before you can comment on or make changes to this bug.