Bug 202520

Created attachment 380102 [details] patch

Created attachment 380146 [details] patch

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.

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

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

Created attachment 380192 [details] patch

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 on attachment 380192 [details] patch Clearing flags on attachment: 380192 Committed r250712: <https://trac.webkit.org/changeset/250712>

All reviewed patches have been landed. Closing bug.

<rdar://problem/55976544>