Bug 232974 - Move image overlay code out of HTMLElement and into a separate helper file
Summary: Move image overlay code out of HTMLElement and into a separate helper file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-10 17:12 PST by Wenson Hsieh
Modified: 2021-11-11 13:53 PST (History)
23 users (show)

See Also:


Attachments
For EWS (86.39 KB, patch)
2021-11-11 09:03 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix Windows build (85.87 KB, patch)
2021-11-11 09:23 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Add namespace, fix CMake builds (87.58 KB, patch)
2021-11-11 11:04 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-11-10 17:12:18 PST Comment hidden (obsolete)
Comment 1 Wenson Hsieh 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)
Comment 2 Wenson Hsieh 2021-11-11 09:03:29 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-11-11 09:23:01 PST
Created attachment 443958 [details]
Fix Windows build
Comment 4 Antti Koivisto 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.
Comment 5 Antti Koivisto 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").
Comment 6 Wenson Hsieh 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.
Comment 7 Wenson Hsieh 2021-11-11 11:04:31 PST
Created attachment 443975 [details]
Add namespace, fix CMake builds
Comment 8 EWS 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.
Comment 9 Wenson Hsieh 2021-11-11 13:52:10 PST
Committed r285655 (244153@main): <https://commits.webkit.org/244153@main>
Comment 10 Radar WebKit Bug Importer 2021-11-11 13:53:31 PST
<rdar://problem/85314337>