Bug 232974 – Move image overlay code out of HTMLElement and into a separate helper file

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

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)

[Wenson Hsieh]

The existing helpers on HTMLElement should belong in a separate header/compilation unit, instead of drifting around in HTMLElement.(h|cpp)

[Wenson Hsieh]

Created attachment 443953 [details]
For EWS

[Wenson Hsieh]

Created attachment 443958 [details]
Fix Windows build

[Antti Koivisto]

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

Created attachment 443975 [details]
Add namespace, fix CMake builds

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 443975 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Wenson Hsieh]

Committed r285655 (244153@main): <https://commits.webkit.org/244153@main>

[Radar WebKit Bug Importer]

<rdar://problem/85314337>