WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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)
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)
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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)
Created
attachment 443953
[details]
For EWS
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
Committed
r285655
(
244153@main
): <
https://commits.webkit.org/244153@main
>
Radar WebKit Bug Importer
Comment 10
2021-11-11 13:53:31 PST
<
rdar://problem/85314337
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug