Bug 156846

Created attachment 276909 [details] wip

Comment on attachment 276909 [details] wip Attachment 276909 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1195955 New failing tests: animations/keyframes-comma-separated.html

Created attachment 276911 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 276909 [details] wip Attachment 276909 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1195975 New failing tests: animations/keyframes-comma-separated.html

Created attachment 276912 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 276909 [details] wip Attachment 276909 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1196008 New failing tests: animations/keyframes-comma-separated.html

Created attachment 276914 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 276915 [details] wip

Created attachment 276935 [details] patch

Created attachment 276944 [details] patch

Comment on attachment 276944 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=276944&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2268 > +static inline RenderStyle* computeRenderStyleForProperty(Node* styledNode, PseudoId pseudoElementSpecifier, CSSPropertyID propertyID, std::unique_ptr<RenderStyle>& ownedStyle) Maybe we should make a class template that supports this use in the future. A union of unique_ptr with a raw pointer that can hold either an owned or unowned pointer. Or we can make a smaller implementation that uses a boolean. Would be nice to not have to add an extra local variable and argument for cases like this. Might even be a way to do this with a unique_ptr and a custom deleter; not sure if the deleter can have 1 bit of state.

Created attachment 277143 [details] patch

Comment on attachment 277143 [details] patch Clearing flags on attachment: 277143 Committed r199964: <http://trac.webkit.org/changeset/199964>

All reviewed patches have been landed. Closing bug.

Comment on attachment 277143 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=277143&action=review This makes me wonder why these are heap allocated. Maybe at some point should passing RenderStyle&& and moving them instead of using objects on the heap and moving unique_ptrs around. I guess they are kind of big, though. > Source/WebCore/css/MediaQueryEvaluator.cpp:88 > MediaQueryEvaluator::MediaQueryEvaluator(const String& acceptedMediaType, Frame* frame, RenderStyle* style) Seems like the argument should be RenderStyle&. > Source/WebCore/css/MediaQueryEvaluator.cpp:91 > + , m_style(WTFMove(style)) Doesn’t make sense since both style and m_style are a raw pointer. I imagine that was different in an earlier point of the development of this patch. > Source/WebCore/css/StyleResolver.h:135 > struct ElementStyle { > - ElementStyle(Ref<RenderStyle>&& renderStyle, std::unique_ptr<Style::Relations> relations = { }) > + ElementStyle(std::unique_ptr<RenderStyle> renderStyle, std::unique_ptr<Style::Relations> relations = { }) > : renderStyle(WTFMove(renderStyle)) > , relations(WTFMove(relations)) > { } > > - Ref<RenderStyle> renderStyle; > + std::unique_ptr<RenderStyle> renderStyle; > std::unique_ptr<Style::Relations> relations; > }; I suspect this might work better without the constructor, using aggregate syntax to construct this when needed. > Source/WebCore/dom/Document.cpp:2131 > + std::unique_ptr<RenderStyle> pageStyle(ensureStyleResolver().styleForPage(pageIndex)); Maybe auto instead? > Source/WebCore/dom/Document.cpp:2137 > + std::unique_ptr<RenderStyle> style = ensureStyleResolver().styleForPage(pageIndex); Maybe auto instead? > Source/WebCore/dom/PseudoElement.cpp:125 > + std::unique_ptr<RenderStyle> createdStyle = RenderStyle::createStyleInheritingFromPseudoStyle(renderer.style()); Maybe auto instead? > Source/WebCore/page/animation/AnimationBase.h:142 > + virtual void getAnimatedStyle(std::unique_ptr<RenderStyle>& /*animatedStyle*/) = 0; Why doesn’t this function use a return value? > Source/WebCore/rendering/RenderButton.cpp:115 > void RenderButton::setupInnerStyle(RenderStyle* innerStyle) Clearly this function should take a reference rather than a pointer. > Source/WebCore/rendering/RenderElement.cpp:1550 > + std::unique_ptr<RenderStyle> result = getUncachedPseudoStyle(PseudoStyleRequest(pseudo), parentStyle); Maybe auto instead? > Source/WebCore/rendering/RenderElement.cpp:1588 > + if (std::unique_ptr<RenderStyle> pseudoStyle = selectionPseudoStyle()) { Maybe auto instead? > Source/WebCore/rendering/RenderElement.cpp:1633 > + std::unique_ptr<RenderStyle> pseudoStyle = selectionPseudoStyle(); Maybe auto instead? > Source/WebCore/rendering/RenderLayerCompositor.cpp:519 > + std::unique_ptr<RenderStyle> scrollbarStyle = static_cast<RenderScrollbar*>(scrollbar)->getScrollbarPseudoStyle(ScrollbarBGPart, SCROLLBAR); Maybe auto instead? > Source/WebCore/rendering/RenderNamedFlowFragment.cpp:467 > + std::unique_ptr<RenderStyle> objectRegionStyle = RenderStyle::clone(&object->style()); > + std::unique_ptr<RenderStyle> objectOriginalStyle = RenderStyle::clone(objectPair.value.style.get()); Maybe auto instead? > Source/WebCore/rendering/RenderScrollbar.cpp:146 > + std::unique_ptr<RenderStyle> result = owningRenderer()->getUncachedPseudoStyle(PseudoStyleRequest(pseudoId, this, partType), &owningRenderer()->style()); Maybe auto instead? > Source/WebCore/rendering/RenderScrollbar.cpp:216 > + std::unique_ptr<RenderStyle> partStyle = getScrollbarPseudoStyle(partType, pseudoForScrollbarPart(partType)); Maybe auto instead? > Source/WebCore/rendering/style/RenderStyle.h:478 > +#if !ASSERT_DISABLED > + bool m_deletionHasBegun { false }; > +#endif Seems like this could be private. Also, neat that we have this in RefCounted, but do we really need it in RenderStyle?

(In reply to comment #15) > Comment on attachment 277143 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277143&action=review > > This makes me wonder why these are heap allocated. Maybe at some point > should passing RenderStyle&& and moving them instead of using objects on the > heap and moving unique_ptrs around. I guess they are kind of big, though. The plan is to move to RenderStyle&& in at least some places. One obvious thing we can do now is to inline RenderStyle to RenderElement, eliminating an allocation and indirection per renderer.

> > Source/WebCore/rendering/style/RenderStyle.h:478 > > +#if !ASSERT_DISABLED > > + bool m_deletionHasBegun { false }; > > +#endif > > Seems like this could be private. > > Also, neat that we have this in RefCounted, but do we really need it in > RenderStyle? I just wanted to check I wasn't making mistakes. It might be unnecessary to keep it around.