[Webkit-unassigned] [Bug 156846] RenderStyle should not be reference counted

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=156846

--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 277143
  --> https://bugs.webkit.org/attachment.cgi?id=277143
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160424/b56a1929/attachment.html>