Summary: | RenderStyle should inherit from RefCounted | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||||||
Component: | CSS | Assignee: | Dave Hyatt <hyatt> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cmarrin, simon.fraser | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Dave Hyatt
2008-10-16 11:14:12 PDT
Created attachment 24399 [details]
Patch
Created attachment 24403 [details]
Patch
Comment on attachment 24403 [details]
Patch
Your ChangeLog should at least include these two bits of information:
1) This patch makes RenderStyles not be allocated inside a RenderArena
2) (1) is OK to do because....
@@ RenderStyle* CSSStyleSelector::styleForE
11781165 m_style->setUnique();
11791166
11801167 // Now return the style.
1181 return m_style;
1168 return m_style.release();
11821169 }
This will set m_style to 0. The old code didn't do that. Why is that OK?
@@ RenderStyle* CSSStyleSelector::pseudoSty
13161300 if (m_fontDirty)
13171301 updateFont();
13181302 // Clean up our style object's display and text decorations (among other fixups).
1319 adjustRenderStyle(m_style, 0);
1303 adjustRenderStyle(style(), 0);
13201304
13211305 // Now return the style.
1322 return m_style;
1306 return m_style.release();
13231307 }
Same question here.
Why are these two functions modifying m_style at all? Their names make it sound like they should be const member functions.
@@ inline void ElementRareData::resetComput
5251 {
5352 if (!m_computedStyle)
5453 return;
55 m_computedStyle->deref(element->document()->renderArena());
5654 m_computedStyle = 0;
5755 }
I think we prefer .clear() instead of = 0 for clearing RefPtrs. You have this in a few other places in this patch.
1069 void Node::setRenderStyle(const PassRefPtr<RenderStyle>& s)
Why are you using a const PassRefPtr& instead of just a PassRefPtr? Ditto for all derived classes, of course.
@@ RenderStyle* AnimationController::update
234234 if ((!oldStyle || (!oldStyle->animations() && !oldStyle->transitions())) && (!newStyle->animations() && !newStyle->transitions()))
235235 return newStyle;
236236
237 RenderStyle* blendedStyle = newStyle;
237 RefPtr<RenderStyle> blendedStyle = newStyle;
This will ref/deref where we didn't used to before. Why is this needed?
183183 virtual void animate(CompositeAnimation*, RenderObject*, const RenderStyle* currentStyle,
184 const RenderStyle* targetStyle, RenderStyle*& animatedStyle) { }
184 const RenderStyle* targetStyle, RefPtr<RenderStyle>& animatedStyle) { }
Is animatedStyle an out parameter? Why not just return a PassRefPtr?
229 PassRefPtr<RenderStyle> CompositeAnimationPrivate::animate(RenderObject* renderer, RenderStyle* currentStyle, RenderStyle* targetStyle)
230230 {
231 RenderStyle* resultStyle = 0;
231 RefPtr<RenderStyle> resultStyle;
232232
233233 // Update animations first so we can see if any transitions are overridden
234234 updateKeyframeAnimations(renderer, currentStyle, targetStyle);
Somewhere you should have "return resultStyle.release();" But I don't see that in this patch.
37 ImplicitAnimation::ImplicitAnimation(const Animation* transition, int animatingProperty, RenderObject* renderer, CompositeAnimation* compAnim, RenderStyle* fromStyle)
3838 : AnimationBase(transition, renderer, compAnim)
3939 , m_transitionProperty(transition->property())
4040 , m_animatingProperty(animatingProperty)
4141 , m_overridden(false)
42 , m_fromStyle(0)
43 , m_toStyle(0)
4442 {
4543 ASSERT(animatingProperty != cAnimateAll);
46 if (fromStyle) {
44 if (fromStyle)
4745 m_fromStyle = fromStyle;
48 const_cast<RenderStyle*>(m_fromStyle)->ref();
49 }
You can just initialize m_fromStyle in the initializer list. The if isn't helping you at all.
78 animatedStyle = RenderStyle::clone(targetStyle);
Maybe we should have a non-static copy() member function on RenderStyle?
41 KeyframeAnimation::KeyframeAnimation(const Animation* animation, RenderObject* renderer, int index, CompositeAnimation* compAnim, RenderStyle* unanimatedStyle)
4242 : AnimationBase(animation, renderer, compAnim)
4343 , m_keyframes(renderer, animation->name())
4444 , m_index(index)
45 , m_unanimatedStyle(0)
4645 {
47 if (unanimatedStyle) {
48 const_cast<RenderStyle*>(unanimatedStyle)->ref();
46 if (unanimatedStyle)
4947 m_unanimatedStyle = unanimatedStyle;
50 }
Again, you can just initialize in the initializer list. The if isn't helping.
@@ void InlineTextBox::paint(RenderObject::
376376 selectionFillColor = foreground;
377377 }
378378
379 if (RenderStyle* pseudoStyle = object()->getPseudoStyle(RenderStyle::SELECTION)) {
379 RefPtr<RenderStyle> pseudoStyle = object()->getPseudoStyle(RenderStyle::SELECTION);
380 if (pseudoStyle) {
You can keep the declaration/assignment inside the if condition.
@@ void RenderContainer::addChild(RenderObj
113113 table = static_cast<RenderTable*>(afterChild);
114114 else {
115115 table = new (renderArena()) RenderTable(document() /* is anonymous */);
116 RenderStyle *newStyle = new (renderArena()) RenderStyle;
116 RefPtr<RenderStyle> newStyle = RenderStyle::create();
117117 newStyle->inheritFrom(style());
118118 newStyle->setDisplay(TABLE);
119119 table->setStyle(newStyle);
Should you be passing newStyle.release() to setStyle? There are a number of places in this patch where you don't do this -- maybe you should do it everywhere?
137 PassRefPtr<RenderStyle> RenderFileUploadControl::createButtonStyle(const RenderStyle* parentStyle) const
138138 {
139 RenderStyle* style = getPseudoStyle(RenderStyle::FILE_UPLOAD_BUTTON);
139 RefPtr<RenderStyle> style = getPseudoStyle(RenderStyle::FILE_UPLOAD_BUTTON);
At some point you should be returning style.release(), but I don't see that in this patch.
69 static RenderFlow* createAnonymousFlow(Document*, const PassRefPtr<RenderStyle>&);
Why a const PassRefPtr&?
@@ Color RenderObject::selectionForegroundC
20562056 if (style()->userSelect() == SELECT_NONE)
20572057 return color;
20582058
2059 if (RenderStyle* pseudoStyle = getPseudoStyle(RenderStyle::SELECTION)) {
2059 if (RefPtr<RenderStyle> pseudoStyle = getPseudoStyle(RenderStyle::SELECTION)) {
I don't understand why you had to add a RefPtr here. You didn't remove any calls to deref, so getPseudoStyle must not have been handing off ownership to the caller. So why use a RefPtr? There are other places in this patch where this happens, too.
Created attachment 24422 [details]
Patch #3
Address Adam's comments.
Comment on attachment 24422 [details]
Patch #3
@@ inline void ElementRareData::resetComput
5251 {
5352 if (!m_computedStyle)
5453 return;
55 m_computedStyle->deref(element->document()->renderArena());
56 m_computedStyle = 0;
54 m_computedStyle.clear();
5755 }
The if here isn't needed anymore.
152 void HTMLOptGroupElement::setRenderStyle(RenderStyle* newStyle)
146 void HTMLOptGroupElement::setRenderStyle(PassRefPtr<RenderStyle> newStyle)
153147 {
154 RenderStyle* oldStyle = m_style;
155148 m_style = newStyle;
156 if (newStyle)
157 newStyle->ref();
158 if (oldStyle)
159 oldStyle->deref(document()->renderArena());
160149 }
161150
162151 RenderStyle* HTMLOptGroupElement::nonRendererRenderStyle() const
163152 {
164 return m_style;
153 return m_style.get();
165154 }
You could even move these into the header now.
Ditto for HTMLOptionElement.
183183 virtual void animate(CompositeAnimation*, RenderObject*, const RenderStyle* currentStyle,
184 const RenderStyle* targetStyle, RenderStyle*& animatedStyle) { }
184 const RenderStyle* targetStyle, RefPtr<RenderStyle>& animatedStyle) { }
Why not just have this function return a PassRefPtr, rather than using an out parameter?
@@ RenderStyle* AnimationController::update
234234 if ((!oldStyle || (!oldStyle->animations() && !oldStyle->transitions())) && (!newStyle->animations() && !newStyle->transitions()))
235235 return newStyle;
236236
237 RenderStyle* blendedStyle = newStyle;
237 RefPtr<RenderStyle> blendedStyle = newStyle;
I think you can remove this line of code and just declare blendedStyle the first time it's used a few lines later.
page/animation/AnimationController.h
2929 #ifndef AnimationController_h
3030 #define AnimationController_h
3131
32 #include <wtf/PassRefPtr.h>
33
I think you can use wtf/Forward.h here instead.
229 PassRefPtr<RenderStyle> CompositeAnimationPrivate::animate(RenderObject* renderer, RenderStyle* currentStyle, RenderStyle* targetStyle)
230230 {
231 RenderStyle* resultStyle = 0;
231 RefPtr<RenderStyle> resultStyle;
Seems like you're missing a .release() in this function.
@@ void RenderContainer::updateBeforeAfterC
324324 genChild->setStyle(pseudoElementStyle);
325325 else if (genChild->isImage()) {
326326 // Images get an empty style that inherits from the pseudo.
327 RenderStyle* style = new (renderArena()) RenderStyle;
327 RefPtr<RenderStyle> style = RenderStyle::create();
328328 style->inheritFrom(pseudoElementStyle);
329329 genChild->setStyle(style);
330330 } else
@@ void RenderContainer::updateBeforeAfterC
353353 break;
354354 case CONTENT_OBJECT: {
355355 RenderImageGeneratedContent* image = new (renderArena()) RenderImageGeneratedContent(document()); // anonymous object
356 RenderStyle* style = new (renderArena()) RenderStyle;
356 RefPtr<RenderStyle> style = RenderStyle::create();
357357 style->inheritFrom(pseudoElementStyle);
358358 image->setStyle(style);
359359 if (StyleImage* styleImage = content->m_content.m_image)
Missing .release()s in both of these cases.
2165 void RenderObject::setStyle(PassRefPtr<RenderStyle> style)
21662166 {
21672167 if (m_style == style)
21682168 return;
21692169
21702170 RenderStyle::Diff diff = RenderStyle::Equal;
21712171 if (m_style)
2172 diff = m_style->diff(style);
2172 diff = m_style->diff(style.get());
21732173
21742174 // If we have no layer(), just treat a RepaintLayer hint as a normal Repaint.
21752175 if (diff == RenderStyle::RepaintLayer && !hasLayer())
21762176 diff = RenderStyle::Repaint;
21772177
2178 styleWillChange(diff, style);
2178 styleWillChange(diff, style.get());
21792179
2180 RenderStyle* oldStyle = m_style;
2181 m_style = const_cast<RenderStyle*>(style);
2182 if (m_style)
2183 m_style->ref();
2180 RefPtr<RenderStyle> oldStyle = m_style;
2181 m_style = style;
You can make this a little more efficient by doing:
RefPtr<RenderStyle> oldStyle = m_style.release();
2192 void RenderObject::setStyleInternal(PassRefPtr<RenderStyle> style)
21992193 {
22002194 if (m_style == style)
22012195 return;
2202 if (m_style)
2203 m_style->deref(renderArena());
22042196 m_style = style;
2205 if (m_style)
2206 m_style->ref();
22072197 }
The if here isn't needed anymore.
379 RenderStyle* getCachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* parentStyle = 0) const;
380 PassRefPtr<RenderStyle> getUncachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* parentStyle = 0) const;
I think it's worth adding comments about how one should choose which of these two functions to call.
38 void RenderSVGBlock::setStyle(PassRefPtr<RenderStyle> style)
3939 {
40 const RenderStyle* useStyle = style;
40 RefPtr<RenderStyle> useStyle = style;
4141
4242 // SVG text layout code expects us to be a block-level style element.
4343 if (useStyle->display() == NONE)
4444 setChildrenInline(false);
4545 else if (useStyle->isDisplayInlineType()) {
46 RenderStyle* newStyle = new (renderArena()) RenderStyle();
47 newStyle->inheritFrom(style);
46 RefPtr<RenderStyle> newStyle = RenderStyle::create();
47 newStyle->inheritFrom(useStyle.get());
4848 newStyle->setDisplay(BLOCK);
4949 useStyle = newStyle;
5050 }
You can do:
useStyle = newStyle.release();
177 PassRefPtr<RenderStyle> RenderSlider::createThumbStyle(const RenderStyle* parentStyle, const RenderStyle* oldStyle)
I think you're missing a .release() in this function.
36 inline RenderStyle* defaultStyle()
3937 {
40 return renderArena->allocate(sz);
38 static RefPtr<RenderStyle> s_defaultStyle;
39 if (!s_defaultStyle)
40 s_defaultStyle = RenderStyle::createDefaultStyle();
41 return s_defaultStyle.get();
4142 }
It would be better just to use a bare (leaked) pointer here so that we don't have to call deref() when the process is exiting. You also don't need the if -- you can just initialize on the same line you declare it on.
142121 RenderStyle::RenderStyle(const RenderStyle& o)
143 : inherited_flags(o.inherited_flags)
122 : RefCounted<RenderStyle>()
The RefCounted() shouldn't be necessary.
Have you tried run-webkit-tests --leaks?
Created attachment 24435 [details]
Patch #4
Comment on attachment 24435 [details]
Patch #4
379 // The pseudo element style can be cached or uncached. Use the cached method if the pseudo element doesn't respect
380 // any pseudo classes (and therefore has no concept of changing state).
381 RenderStyle* getCachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* parentStyle = 0) const;
382 PassRefPtr<RenderStyle> getUncachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* parentStyle = 0) const;
Is it possible to assert that people are calling the right function?
36 inline RenderStyle* defaultStyle()
3937 {
38 RenderStyle* s_defaultStyle = RenderStyle::createDefaultStyle().releaseRef();
39 return s_defaultStyle;
4140 }
I think you meant to declare s_defaultStyle static.
142119 RenderStyle::RenderStyle(const RenderStyle& o)
143 : inherited_flags(o.inherited_flags)
120 : RefCounted<RenderStyle>()
121 , inherited_flags(o.inherited_flags)
The RefCounted() isn't necessary here.
Were we leaking RenderStyle::pseudoStyle before? I don't see any existing calls to deref() it.
r=me if you fix the defaultStyle() bug.
(In reply to comment #7) > (From update of attachment 24435 [details] [edit]) > 379 // The pseudo element style can be cached or uncached. Use the cached > method if the pseudo element doesn't respect > 380 // any pseudo classes (and therefore has no concept of changing > state). > 381 RenderStyle* getCachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* > parentStyle = 0) const; > 382 PassRefPtr<RenderStyle> getUncachedPseudoStyle(RenderStyle::PseudoId, > RenderStyle* parentStyle = 0) const; > > Is it possible to assert that people are calling the right function? > I didn't want to make any assumptions about particular pseudo elements for now. > 36 inline RenderStyle* defaultStyle() > 3937 { > 38 RenderStyle* s_defaultStyle = > RenderStyle::createDefaultStyle().releaseRef(); > 39 return s_defaultStyle; > 4140 } > > I think you meant to declare s_defaultStyle static. > Indeed. > 142119 RenderStyle::RenderStyle(const RenderStyle& o) > 143 : inherited_flags(o.inherited_flags) > 120 : RefCounted<RenderStyle>() > 121 , inherited_flags(o.inherited_flags) > > The RefCounted() isn't necessary here. > Yes it is. > Were we leaking RenderStyle::pseudoStyle before? I don't see any existing calls > to deref() it. > > r=me if you fix the defaultStyle() bug. > Thanks for the comments! |