Bug 202520 - [CSS Shadow Parts] Support 'exportparts' attribute
Summary: [CSS Shadow Parts] Support 'exportparts' attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 149443
  Show dependency treegraph
 
Reported: 2019-10-03 05:28 PDT by Antti Koivisto
Modified: 2019-10-04 01:34 PDT (History)
14 users (show)

See Also:


Attachments
patch (21.37 KB, patch)
2019-10-03 05:58 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (21.52 KB, patch)
2019-10-03 11:41 PDT, Antti Koivisto
rniwa: review+
Details | Formatted Diff | Diff
patch (21.54 KB, patch)
2019-10-03 23:49 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2019-10-03 05:28:58 PDT
Support 'exportparts' attribute for exporting part mappings from subcomponents.
Comment 1 Antti Koivisto 2019-10-03 05:58:20 PDT
Created attachment 380102 [details]
patch
Comment 2 Antti Koivisto 2019-10-03 11:41:51 PDT
Created attachment 380146 [details]
patch
Comment 3 Ryosuke Niwa 2019-10-03 14:12:26 PDT
Comment on attachment 380146 [details]
patch

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

> Source/WebCore/css/ElementRuleCollector.cpp:273
> +        SetForScope<const Element*> partMatchingScope(m_shadowHostInPartRuleScope, &shadowHost);

Can we use RefPtr?

> Source/WebCore/css/ElementRuleCollector.cpp:282
> +    if (!containingShadowRoot.partMappings().isEmpty()) {

Why not exit early instead of nesting?

> Source/WebCore/css/ElementRuleCollector.h:103
> +    const Element* m_shadowHostInPartRuleScope { nullptr };

Can we use RefPtr instead?

> Source/WebCore/css/SelectorChecker.cpp:1154
> +            auto mapPartName = [&](AtomString partName) {

mapPartName isn't quite descriptive.
How about partNameInRuleScope or translatePartNameToRuleScope?

> Source/WebCore/css/SelectorChecker.cpp:1155
> +                for (auto* shadowRoot = element.containingShadowRoot(); shadowRoot; shadowRoot = shadowRoot->host()->containingShadowRoot()) {

Use RefPtr?

> Source/WebCore/css/SelectorChecker.cpp:1166
> +            Vector<AtomString, 3> mappedElementPartNames;

Why not 4??

> Source/WebCore/css/SelectorChecker.h:87
> +        const Element* shadowHostInPartRuleScope { nullptr };

Can we use RefPtr instead?

> Source/WebCore/dom/Element.cpp:1746
> +        else if (name  == HTMLNames::exportpartsAttr) {

Nit: Two spaces.

> Source/WebCore/dom/Element.cpp:1747
> +            if (auto* shadowRoot = this->shadowRoot())

Use makeRefPtr?

> Source/WebCore/dom/ShadowRoot.cpp:259
> +    auto parsePartMapping = [&](StringView mappingString) {

I would have preferred this as a regular static function which returns an optional pair of AtomicString or a optional struct.

> Source/WebCore/dom/ShadowRoot.cpp:262
> +        auto skipWhitespace = [&] (auto position) {

We should really add StringView::skipUntil & StringView::skipWhile... not that I think we should do it in this patch.

> Source/WebCore/dom/ShadowRoot.cpp:278
> +        

Nit: whitespace.

> Source/WebCore/dom/ShadowRoot.cpp:288
> +            mappings.add(firstPart, firstPart);
> +            return;

That way, this would read: return { firstPart, firstPart };

> Source/WebCore/dom/ShadowRoot.cpp:313
> +    while (begin < end) {
> +        size_t mappingEnd = begin;
> +        while (mappingEnd < end && mappingsListString[mappingEnd] != ',')
> +            ++mappingEnd;

Can't we just use StringView::split and iterate over SplitResult?

> Source/WebCore/dom/ShadowRoot.h:123
> +    mutable std::unique_ptr<HashMap<AtomString, AtomString>> m_partMappings;

Maybe we can use Optional instead?
It doesn't seem like an extra pointer size increase to ShadowRoot itself isn't a big deal.
Comment 4 Antti Koivisto 2019-10-03 22:43:24 PDT
> Can we use RefPtr?

No, until bug 202562 is fixed.
 
> Can't we just use StringView::split and iterate over SplitResult?

The specced parsing is just complex enough that trying to take shortcuts leads to code where it is hard to see if it matches the spec or not.
Comment 5 Antti Koivisto 2019-10-03 23:40:13 PDT
> Maybe we can use Optional instead?
> It doesn't seem like an extra pointer size increase to ShadowRoot itself
> isn't a big deal.

HashTable is like 3 pointers but it still a not a big deal.
Comment 6 Antti Koivisto 2019-10-03 23:49:35 PDT
Created attachment 380192 [details]
patch
Comment 7 WebKit Commit Bot 2019-10-04 00:34:49 PDT
Comment on attachment 380192 [details]
patch

Rejecting attachment 380192 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 380192, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=380192&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=202520&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 380192 from bug 202520.
Fetching: https://bugs.webkit.org/attachment.cgi?id=380192
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/imported/w3c/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/imported/w3c/ChangeLog' is out of date
W: ca61dfd2accd813464098eb6aca9ddb3730f86fb and refs/remotes/origin/master differ, using rebase:
:040000 040000 3fa4548e4f4d7d62a414386b23af9da57f9651be a7519737312f74324bace8e89d66a28b8d1b1115 M	LayoutTests
:040000 040000 5eb8944f8412d2e5780731c5e75d1c9605368b28 9ebc68c3dbba69908e3eab5fc4793638d1677715 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/imported/w3c/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/imported/w3c/ChangeLog' is out of date
W: ca61dfd2accd813464098eb6aca9ddb3730f86fb and refs/remotes/origin/master differ, using rebase:
:040000 040000 3fa4548e4f4d7d62a414386b23af9da57f9651be a7519737312f74324bace8e89d66a28b8d1b1115 M	LayoutTests
:040000 040000 5eb8944f8412d2e5780731c5e75d1c9605368b28 9ebc68c3dbba69908e3eab5fc4793638d1677715 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: https://webkit-queues.webkit.org/results/13094104
Comment 8 WebKit Commit Bot 2019-10-04 01:33:12 PDT
Comment on attachment 380192 [details]
patch

Clearing flags on attachment: 380192

Committed r250712: <https://trac.webkit.org/changeset/250712>
Comment 9 WebKit Commit Bot 2019-10-04 01:33:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-10-04 01:34:17 PDT
<rdar://problem/55976544>