Bug 103208 – Don't use ComposedShadowTreeWalker in NodeRenderingContext when SHADOW_DOM is not enabled

• Status: RESOLVED
• Resolution: INVALID
• Component: DOM

[Antti Koivisto]

It does tons of work that is not needed when SHADOW_DOM is not enabled.

[Antti Koivisto]

Created attachment 175895 [details]
patch

[Sam Weinig]

Comment on attachment 175895 [details]
patch

Egads.  I'm not a fan of more #ifdefs, but a 3-4% regression from shadow DOM stuff is clearly not acceptable.  You should file a follow up about the regression, since one day we may need to turn it on.

[Build Bot]

Comment on attachment 175895 [details]
patch

Attachment 175895 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14987445

New failing tests:
fast/html/details-add-summary-child-2.html
fast/html/details-add-summary-1.html
fast/html/details-add-details-child-1.html
fast/html/details-add-summary-7.html
fast/html/details-add-summary-5.html
fast/html/details-add-summary-5-and-click.html
fast/html/details-add-summary-6-and-click.html
fast/html/details-add-child-1.html
fast/html/details-add-summary-4.html
fast/html/details-add-summary-6.html
fast/html/details-click-controls.html
fast/html/details-keyboard-show-hide.html
fast/html/details-add-summary-2.html
fast/html/details-add-summary-10.html
fast/html/details-add-summary-10-and-click.html
fast/html/details-add-summary-9.html
fast/html/details-add-child-2.html
fast/block/block-remove-child-delete-line-box-crash.html
fast/html/details-add-details-child-2.html
fast/html/details-add-summary-8-and-click.html
fast/html/details-add-summary-1-and-click.html
fast/html/details-add-summary-2-and-click.html
fast/html/details-add-summary-7-and-click.html
fast/html/details-add-summary-9-and-click.html
fast/events/domactivate-sets-underlying-click-event-as-handled.html
fast/html/details-add-summary-3-and-click.html
fast/html/details-add-summary-4-and-click.html
fast/html/details-add-summary-3.html
fast/html/details-add-summary-child-1.html
fast/html/details-add-summary-8.html

[Antti Koivisto]

Looks like the internal implementation of the <details> element has been made to depend on a bunch of Web Components features. This is not great.

[Eric Seidel (no email)]

I suspect morrita is the <details> expert, judging from the revision log: http://trac.webkit.org/log/trunk/Source/WebCore/html/HTMLDetailsElement.cpp

[Hajime Morrita]

Yes, <details> and <summary> uses Shadow DOM internally,
which allowed us to kill a bunch of renderer-side code. 
I agree that the regression here isn't acceptable though.

Can I take some time to address this? I'll look into it.
The regression should be killed even if SHADOW_DOM is enabled,
at least eventually.

[Antti Koivisto]

Web Components code should be fully behind a feature flag like all other experimental features. Platforms that are not interested in this feature shouldn't need to take the performance and binary size hit from it. Also it is not ok to make other parts of WebKit internals depend on experimental features. The original implementation of <details> did not have such dependency.

The feature define should be renamed to WEB_COMPONENTS or similar. SHADOW_DOM is a bad name as we have tons of internal shadow DOMs that don't depend on most of this code.

[Ojan Vafai]

(In reply to comment #7)
> Web Components code should be fully behind a feature flag like all other experimental features. Platforms that are not interested in this feature shouldn't need to take the performance and binary size hit from it. Also it is not ok to make other parts of WebKit internals depend on experimental features. The original implementation of <details> did not have such dependency.
> 
> The feature define should be renamed to WEB_COMPONENTS or similar. SHADOW_DOM is a bad name as we have tons of internal shadow DOMs that don't depend on most of this code.

In general, I agree with this principle. In this specific case, getting rid of custom renderer code and replacing it with components seems like a big win to me. Every time we write custom renderer code for form controls, it's a source of a long tail of correctness and security bugs.

Once (if) there is more general buy-in to shipping web components, I think replacing custom renderers with web components is a good direction to go. Maybe making those changes was premature though as it seems Apple doesn't necessarily want to ship web components?

[Hajime Morrita]

For performance, we should definitely get rid of the regression. I'll take a look to make this patch work with <details>.

For binary size, it's infeasible for now to hide whole Shadow DOM things behind a flag. It will result tons of ifdefs and hurts the code readability. Sine it replaces certain renderer implementations, it can taken as a nice-to-have WebCore plumbing. That being said, I agree that some more Shadow DOM/Web Components implementation can be moved behind the flag. That will be tracked by a bug 103339. 

On the flag name, please let us use SHADOW_DOM. The API standard is named like that http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html, and WEB_COMPONENTS sounds too generic anyway. There are other Web Components related APIs like custom dom elements and template .

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> Web Components code should be fully behind a feature flag like all other experimental features. Platforms that are not interested in this feature shouldn't need to take the performance and binary size hit from it. Also it is not ok to make other parts of WebKit internals depend on experimental features. The original implementation of <details> did not have such dependency.
> 
> The feature define should be renamed to WEB_COMPONENTS or similar. SHADOW_DOM is a bad name as we have tons of internal shadow DOMs that don't depend on most of this code.

As I mentioned at this year's WebKit contributors meeting (http://webkitmemes.tumblr.com/image/21402870608), all of our internal shadow DOMs use the same plumbing. It's all been painfully specified here (http://www.w3.org/TR/shadow-dom/). SHADOW_DOM intentionally guards only the public-facing API, because most of it had been just a reshaping of the existing custom shadow DOM code we keep writing for every darn HTML element. Most of the Shadow DOM spec is just writing down lessons I learned from that refactoring.

Speaking of which, the initial, hard-coded implementation of <details> was so bad that you would've cried tears of blood just looking at it. There are lots of tiny details that we figured out and made sure they worked right with insertion points, from accessibility to hit-testing, to event retargeting and rendering. I somehow doubt that there's a noticeable binary size hit. I'd bet that a proper <details> implementation in itself would've been a hefty chunk of code.

I understand your frustration and I totally agree that we should fix the performance regression though. We suck and we should fix it -- and we will. And may Kling create a thousand ghastly memes featuring me and Morrita if we don't.

[Antti Koivisto]

(In reply to comment #9)
> For binary size, it's infeasible for now to hide whole Shadow DOM things behind a flag. It will result tons of ifdefs and hurts the code readability. 

I'm not buying this.

Quick testing showed that with <details> disabled it was fairly trivial and non-invasive to compile out ComposedShadowTreeWalker, HTMLContentElement, HTMLShadowElement, InsertionPoint, ContentDistributor and ContentSelectorQuery. That's a more than 1k lines of complex code. The rest of the engine seems happy with just ShadowRoot and parts of ElementShadow.

I'm not sure if the cleaner looking <details> implementation is worth the complexity. I suspect it is completely opaque to many WebKit engineers. I spent long time trying to figure out what was going on to make this patch work with <details> and failed.

[Maciej Stachowiak]

(In reply to comment #8)
> (In reply to comment #7)
> > Web Components code should be fully behind a feature flag like all other experimental features. Platforms that are not interested in this feature shouldn't need to take the performance and binary size hit from it. Also it is not ok to make other parts of WebKit internals depend on experimental features. The original implementation of <details> did not have such dependency.
> > 
> > The feature define should be renamed to WEB_COMPONENTS or similar. SHADOW_DOM is a bad name as we have tons of internal shadow DOMs that don't depend on most of this code.
> 
> In general, I agree with this principle. In this specific case, getting rid of custom renderer code and replacing it with components seems like a big win to me. Every time we write custom renderer code for form controls, it's a source of a long tail of correctness and security bugs.
> 
> Once (if) there is more general buy-in to shipping web components, I think replacing custom renderers with web components is a good direction to go. Maybe making those changes was premature though as it seems Apple doesn't necessarily want to ship web components?

I think it was premature. We're not necessarily opposed to ever shipping web components related code, but since it is still officially an experimental and optional feature, it seems wrong to me that we're effectively forced to ship large chunks of it in our binary, and to have unrelated features depend on it.

I definitely see the potential, but I'd each port to be able to choose when/if to get on the bandwagon, at least until the WebKit community as a whole decides this is no longer an experimental or optional feature.

[Antti Koivisto]

I uploaded a WIP patch to bug 103208. Could people actively working on this feature please pick it up?

[Antti Koivisto]

I mean bug 103654.

[Eric Seidel (no email)]

Attachment 175895 [details] was posted by a committer and has review+, assigning to Antti Koivisto for commit.

[Andreas Kling]

Hue.