<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - RenderStyle should not be reference counted"
href="https://bugs.webkit.org/show_bug.cgi?id=156846#c15">Comment # 15</a>
on <a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - RenderStyle should not be reference counted"
href="https://bugs.webkit.org/show_bug.cgi?id=156846">bug 156846</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=277143&action=diff" name="attach_277143" title="patch">attachment 277143</a> <a href="attachment.cgi?id=277143&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=277143&action=review">https://bugs.webkit.org/attachment.cgi?id=277143&action=review</a>
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.
<span class="quote">> Source/WebCore/css/MediaQueryEvaluator.cpp:88
> MediaQueryEvaluator::MediaQueryEvaluator(const String& acceptedMediaType, Frame* frame, RenderStyle* style)</span >
Seems like the argument should be RenderStyle&.
<span class="quote">> Source/WebCore/css/MediaQueryEvaluator.cpp:91
> + , m_style(WTFMove(style))</span >
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.
<span class="quote">> 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;
> };</span >
I suspect this might work better without the constructor, using aggregate syntax to construct this when needed.
<span class="quote">> Source/WebCore/dom/Document.cpp:2131
> + std::unique_ptr<RenderStyle> pageStyle(ensureStyleResolver().styleForPage(pageIndex));</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/dom/Document.cpp:2137
> + std::unique_ptr<RenderStyle> style = ensureStyleResolver().styleForPage(pageIndex);</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/dom/PseudoElement.cpp:125
> + std::unique_ptr<RenderStyle> createdStyle = RenderStyle::createStyleInheritingFromPseudoStyle(renderer.style());</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/page/animation/AnimationBase.h:142
> + virtual void getAnimatedStyle(std::unique_ptr<RenderStyle>& /*animatedStyle*/) = 0;</span >
Why doesn’t this function use a return value?
<span class="quote">> Source/WebCore/rendering/RenderButton.cpp:115
> void RenderButton::setupInnerStyle(RenderStyle* innerStyle) </span >
Clearly this function should take a reference rather than a pointer.
<span class="quote">> Source/WebCore/rendering/RenderElement.cpp:1550
> + std::unique_ptr<RenderStyle> result = getUncachedPseudoStyle(PseudoStyleRequest(pseudo), parentStyle);</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/rendering/RenderElement.cpp:1588
> + if (std::unique_ptr<RenderStyle> pseudoStyle = selectionPseudoStyle()) {</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/rendering/RenderElement.cpp:1633
> + std::unique_ptr<RenderStyle> pseudoStyle = selectionPseudoStyle();</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/rendering/RenderLayerCompositor.cpp:519
> + std::unique_ptr<RenderStyle> scrollbarStyle = static_cast<RenderScrollbar*>(scrollbar)->getScrollbarPseudoStyle(ScrollbarBGPart, SCROLLBAR);</span >
Maybe auto instead?
<span class="quote">> 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());</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/rendering/RenderScrollbar.cpp:146
> + std::unique_ptr<RenderStyle> result = owningRenderer()->getUncachedPseudoStyle(PseudoStyleRequest(pseudoId, this, partType), &owningRenderer()->style());</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/rendering/RenderScrollbar.cpp:216
> + std::unique_ptr<RenderStyle> partStyle = getScrollbarPseudoStyle(partType, pseudoForScrollbarPart(partType));</span >
Maybe auto instead?
<span class="quote">> Source/WebCore/rendering/style/RenderStyle.h:478
> +#if !ASSERT_DISABLED
> + bool m_deletionHasBegun { false };
> +#endif</span >
Seems like this could be private.
Also, neat that we have this in RefCounted, but do we really need it in RenderStyle?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>