Bug 239460 - Inline Element::shadowRoot()
Summary: Inline Element::shadowRoot()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-18 12:21 PDT by Chris Dumez
Modified: 2022-04-19 16:15 PDT (History)
23 users (show)

See Also:

Attachments
Patch (4.23 KB, patch)
2022-04-18 13:18 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (7.27 KB, patch)
2022-04-18 14:21 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (8.65 KB, patch)
2022-04-18 15:13 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (11.35 KB, patch)
2022-04-18 16:20 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (13.86 KB, patch)
2022-04-18 16:42 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (14.80 KB, patch)
2022-04-18 16:57 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.05 KB, patch)
2022-04-18 19:51 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.20 KB, patch)
2022-04-18 20:00 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.40 KB, patch)
2022-04-18 20:25 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.44 KB, patch)
2022-04-18 21:45 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (23.85 KB, patch)
2022-04-19 10:16 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (27.28 KB, patch)
2022-04-19 11:44 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (11) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2022-04-18 12:21:20 PDT 
Inline Element::shadowRoot() given that it is small and shows up in Speedometer profiles.
Comment 1 Chris Dumez 2022-04-18 13:18:43 PDT 
Created attachment 457818 [details]
Patch
Comment 2 Chris Dumez 2022-04-18 14:21:28 PDT 
Created attachment 457823 [details]
Patch
Comment 3 Chris Dumez 2022-04-18 15:13:16 PDT 
Created attachment 457828 [details]
Patch
Comment 4 Chris Dumez 2022-04-18 16:20:16 PDT 
Created attachment 457834 [details]
Patch
Comment 5 Chris Dumez 2022-04-18 16:42:12 PDT 
Created attachment 457839 [details]
Patch
Comment 6 Chris Dumez 2022-04-18 16:57:41 PDT 
Created attachment 457842 [details]
Patch
Comment 7 Chris Dumez 2022-04-18 19:51:49 PDT 
Created attachment 457848 [details]
Patch
Comment 8 Chris Dumez 2022-04-18 20:00:36 PDT 
Created attachment 457849 [details]
Patch
Comment 9 Chris Dumez 2022-04-18 20:25:50 PDT 
Created attachment 457850 [details]
Patch
Comment 10 Chris Dumez 2022-04-18 21:45:18 PDT 
Created attachment 457854 [details]
Patch
Comment 11 Darin Adler 2022-04-19 09:43:51 PDT 
Comment on attachment 457854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457854&action=review

> Source/WebCore/dom/Element.h:318
> +    ShadowRoot* shadowRoot() const;

If we add the inline keyword to the declaration of shadowRoot (like replace WEBCORE_EXPORT with inline) then, I seem to recall, we get compiler errors rather than linker errors when we call it without remembering to include ElementRareData.h. So I suggest including that keyword. We should use that technique more! I think Jer Noble is the one who pointed this out to me.

> Source/WebCore/editing/cocoa/DataDetection.mm:694
> +std::optional<std::pair<Ref<HTMLElement>, IntRect>> DataDetection::findDataDetectionResultElementInImageOverlay(const FloatPoint& location, const HTMLElement& imageOverlayHost)

This seems like a really good change, but not mentioned in change log.
Comment 12 Chris Dumez 2022-04-19 09:55:22 PDT 
(In reply to Darin Adler from comment #11)
> Comment on attachment 457854 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457854&action=review
> 
> > Source/WebCore/dom/Element.h:318
> > +    ShadowRoot* shadowRoot() const;
> 
> If we add the inline keyword to the declaration of shadowRoot (like replace
> WEBCORE_EXPORT with inline) then, I seem to recall, we get compiler errors
> rather than linker errors when we call it without remembering to include
> ElementRareData.h. So I suggest including that keyword. We should use that
> technique more! I think Jer Noble is the one who pointed this out to me.
> 
> > Source/WebCore/editing/cocoa/DataDetection.mm:694
> > +std::optional<std::pair<Ref<HTMLElement>, IntRect>> DataDetection::findDataDetectionResultElementInImageOverlay(const FloatPoint& location, const HTMLElement& imageOverlayHost)
> 
> This seems like a really good change, but not mentioned in change log.

Ok, I'll mention in the changelog. The issue is that we'd need to include ElementRareData.h from the WebKit layer otherwise. And this is currently not possible because this header is not exported.
Comment 13 Chris Dumez 2022-04-19 10:13:49 PDT 
(In reply to Darin Adler from comment #11)
> Comment on attachment 457854 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457854&action=review
> 
> > Source/WebCore/dom/Element.h:318
> > +    ShadowRoot* shadowRoot() const;
> 
> If we add the inline keyword to the declaration of shadowRoot (like replace
> WEBCORE_EXPORT with inline) then, I seem to recall, we get compiler errors
> rather than linker errors when we call it without remembering to include
> ElementRareData.h. So I suggest including that keyword. We should use that
> technique more! I think Jer Noble is the one who pointed this out to me.

Nice trick. This is good to know, thanks. I'll adopt in this patch.
Comment 14 Chris Dumez 2022-04-19 10:16:46 PDT 
Created attachment 457913 [details]
Patch
Comment 15 Chris Dumez 2022-04-19 11:44:53 PDT 
Created attachment 457921 [details]
Patch
Comment 16 EWS 2022-04-19 16:14:56 PDT 
Committed r293042 (249781@main): <https://commits.webkit.org/249781@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457921 [details].
Comment 17 Radar WebKit Bug Importer 2022-04-19 16:15:15 PDT 
<rdar://problem/91992406>