Created attachment 191867 [details] Patch

I'm not sure this is the solution we want to go with long-term. :) But it would appear to solve the bug. Do you have any perf data to show this is a win?

There got to be a better way of doing this?

(In reply to comment #2) > I'm not sure this is the solution we want to go with long-term. :) But it would appear to solve the bug. > > Do you have any perf data to show this is a win? I'm having issues getting the perf tests to not hang after running them, but I did a few runs and it didn't seem like it was a win at all on html5-full-render.html

(In reply to comment #3) > There got to be a better way of doing this? It requires considerable amounts of plumbing since we'd need to pass the style from recalcStyle => reattach => attach => createRendererIfNeeded => NodeRenderingContext::createRendererForElementIfNeeded() which finally calls styleForRenderer(). I guess we could use the same class and use the style in createRendererForElementIfNeeded instead though which means StyleResolver doesn't need this knowledge, and it would work for PseudoElements too.

Comment on attachment 191867 [details] Patch That's just too ugly. I have a patch somewhere that fixes this properly, by passing the style to attach().

Comment on attachment 191867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191867&action=review > Source/WebCore/css/StyleResolver.cpp:1388 > + return style; If we did this approach, this code should be inside NodeRenderingContext::createRendererForElementIfNeeded

Ran perf tests for html5-full-render: With this caching: 3202.66 ± 0.23% 3205.94 ± 0.17% Without this caching: 3198.89 ± 0.24% 3202.49 ± 0.12% So, surprisingly, it doesn't seem to really matter. I guess there's caching stuff down inside StyleResolver already?

Comment on attachment 191867 [details] Patch Attachment 191867 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17062114 New failing tests: fast/css/pseudo-empty-dynamic-empty.html fast/events/drag-display-none-element.html fast/selectors/empty-element-made-non-empty.html

Comment on attachment 191867 [details] Patch Attachment 191867 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17062117 New failing tests: fast/css/pseudo-empty-dynamic-empty.html http/tests/misc/acid3.html fast/selectors/empty-element-made-non-empty.html

(In reply to comment #8) > Ran perf tests for html5-full-render: > > With this caching: > 3202.66 ± 0.23% > 3205.94 ± 0.17% > > Without this caching: > 3198.89 ± 0.24% > 3202.49 ± 0.12% > > So, surprisingly, it doesn't seem to really matter. I guess there's caching stuff down inside StyleResolver already? It only caches in the sense that it will reuse the calculated style of a sibling if the elements are similar enough. To get a performance improvement you probably need to trigger a style that is unique.

> So, surprisingly, it doesn't seem to really matter. I guess there's caching stuff down inside StyleResolver already? StyleResolver has multiple levels of caches. Also the case just doesn't generally get hit that much. When reattach() gets called the whole subtree under the element will get attached outside recalcStyle without hitting the double calculation case. I have yet to see a real case where this is practical problem. It would still be nice to fix it as it is silly.

(In reply to comment #12) > > So, surprisingly, it doesn't seem to really matter. I guess there's caching stuff down inside StyleResolver already? > > Also the case just doesn't generally get hit that much. When reattach() gets called the whole subtree under the element will get attached outside recalcStyle without hitting the double calculation case. Ah! this is the key bit I was missing. My understanding is lazyAttach() puts us into this state all the time. But it doesn't end up showing up because we only double-style-calc for the root most element of a subtree.

Comment on attachment 191867 [details] Patch Attachment 191867 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17081252 New failing tests: fast/css/pseudo-empty-dynamic-empty.html fast/events/drag-display-none-element.html http/tests/misc/acid3.html fast/selectors/empty-element-made-non-empty.html

(In reply to comment #14) > (From update of attachment 191867 [details]) > Attachment 191867 [details] did not pass mac-ews (mac): > Output: http://webkit-commit-queue.appspot.com/results/17081252 > > New failing tests: > fast/css/pseudo-empty-dynamic-empty.html > fast/events/drag-display-none-element.html > http/tests/misc/acid3.html > fast/selectors/empty-element-made-non-empty.html These failures have me pretty confused. Apparently when calling styleForRenderer() inside Element::recalcStyle we get a different style than we would when later calling it inside NodeRenderingContext. Specifically: style1 = styleForRenderer(); detach(); style2 = styleForRenderer(); These two styles don't have the same properties which breaks the idea of passing the style down through the reattach() call. @antti Do you know what's going on here?

(In reply to comment #15) > (In reply to comment #14) > ... > > These failures have me pretty confused. Apparently when calling styleForRenderer() inside Element::recalcStyle we get a different style than we would when later calling it inside NodeRenderingContext. > > Specifically: > > style1 = styleForRenderer(); > detach(); > style2 = styleForRenderer(); > > These two styles don't have the same properties which breaks the idea of passing the style down through the reattach() call. > > @antti Do you know what's going on here? Hmm, I'm wrong, there's something else weird going on here I'm missing. :/

(In reply to comment #16) > ... > > Hmm, I'm wrong, there's something else weird going on here I'm missing. :/ Figured this out. We used to store the affectedBy bits in the RenderStyle, but now we store them in the ElementRareData and reset them in detach() which means calling styleForRenderer in recalcStyle sets a bunch of bits, but then detach() inside reattach() clears the affectedBy bits so we get a RenderStyle but none of the correct affectedBy bits. So we need to tell detach() not to clear the bits so we get the right values since we're not going to call styleForRenderer() again to set them.

These are some of the side effects of style resolve which we'll need to make explicit. Dimitri and I were just musing about this yesterday.

(In reply to comment #18) > These are some of the side effects of style resolve which we'll need to make explicit. Dimitri and I were just musing about this yesterday. Yeah. So not calling resetComputedStyle() inside detach() (from reattach) fixes some of the tests, but I'm still stuck for LayoutTests/fast/events/drag-display-none-element.html This test fails because the paragraph remains hidden. I tried not calling resetDynamicRestyleObservations but that doesn't seem to help. I need to figure out what other shared state change happened inside style recalc.

This would be resolved by passing the context in Bug 113123

(In reply to comment #19) > (In reply to comment #18) > > These are some of the side effects of style resolve which we'll need to make explicit. Dimitri and I were just musing about this yesterday. > > Yeah. So not calling resetComputedStyle() inside detach() (from reattach) fixes some of the tests, but I'm still stuck for LayoutTests/fast/events/drag-display-none-element.html > > This test fails because the paragraph remains hidden. I tried not calling resetDynamicRestyleObservations but that doesn't seem to help. > > I need to figure out what other shared state change happened inside style recalc. I figured this out btw, it's because SelectorChecker looks at the renderer for isDragging to set some state which is different after detach() since we lose the renderer. We should probably break that dependency.

http://trac.webkit.org/changeset/148970 fixed the most common instance of this problem.