WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
123051
Start passing RenderStyle around with PassRef.
https://bugs.webkit.org/show_bug.cgi?id=123051
Summary
Start passing RenderStyle around with PassRef.
Andreas Kling
Reported
2013-10-18 17:19:14 PDT
Time to kick this up a notch.
Attachments
Patch
(58.45 KB, patch)
2013-10-18 17:37 PDT
,
Andreas Kling
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Andreas Kling
Comment 1
2013-10-18 17:37:15 PDT
Created
attachment 214623
[details]
Patch
WebKit Commit Bot
Comment 2
2013-10-18 17:38:25 PDT
Attachment 214623
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/PassRef.h', u'Source/WTF/wtf/RefPtr.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/html/HTMLLinkElement.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationController.h', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderElement.cpp', u'Source/WebCore/rendering/RenderElement.h', u'Source/WebCore/rendering/RenderFlowThread.cpp', u'Source/WebCore/rendering/RenderFlowThread.h', u'Source/WebCore/rendering/RenderFullScreen.cpp', u'Source/WebCore/rendering/RenderImage.cpp', u'Source/WebCore/rendering/RenderInline.cpp', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderListItem.cpp', u'Source/WebCore/rendering/RenderNamedFlowFragment.cpp', u'Source/WebCore/rendering/RenderNamedFlowThread.cpp', u'Source/WebCore/rendering/RenderRuby.cpp', u'Source/WebCore/rendering/RenderRubyRun.cpp', u'Source/WebCore/rendering/RenderScrollbar.cpp', u'Source/WebCore/rendering/RenderSearchField.cpp', u'Source/WebCore/rendering/RenderTable.cpp', u'Source/WebCore/rendering/RenderTableCell.cpp', u'Source/WebCore/rendering/RenderTableRow.cpp', u'Source/WebCore/rendering/RenderTableSection.cpp', u'Source/WebCore/rendering/RenderTextControl.h', u'Source/WebCore/rendering/RenderTextControlMultiLine.cpp', u'Source/WebCore/rendering/RenderTextControlMultiLine.h', u'Source/WebCore/rendering/RenderTextControlSingleLine.cpp', u'Source/WebCore/rendering/RenderTextControlSingleLine.h', u'Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLRow.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/svg/RenderSVGBlock.cpp', u'Source/WebCore/rendering/svg/RenderSVGBlock.h', u'Source/WebCore/style/StyleResolveForDocument.cpp', u'Source/WebCore/style/StyleResolveForDocument.h', u'Source/WebCore/style/StyleResolveTree.cpp']" exit_code: 1 Source/WebCore/rendering/style/RenderStyle.cpp:85: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 3
2013-10-18 19:07:05 PDT
Comment on
attachment 214623
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=214623&action=review
Exciting patch
> Source/WTF/wtf/PassRef.h:46 > + const T* operator->() const; > + T* operator->();
I’m not so glad you are adding these. I hate using -> when it’s a reference. Honestly, I’d rather see .get(). in all those places. Just so there is no -> that is not for a pointer. Same for Ref, really.
> Source/WTF/wtf/PassRef.h:48 > + const T& get() const; > + T& get();
I’m really glad you are adding these!
> Source/WTF/wtf/PassRef.h:50 > + void dropReference();
Seems sad that we really need this. I think we need T& leakRef(), as I learned while trying to do another patch.
> Source/WTF/wtf/PassRef.h:106 > -template<typename T> PassRef<T>::~PassRef() > +template<typename T> inline PassRef<T>::~PassRef() > { > ASSERT(m_gaveUpReference); > }
Seems pointless to mark a debug-only function inline.
> Source/WTF/wtf/RefPtr.h:61 > + PassRef<T> releaseNonNull() { ASSERT(m_ptr); PassRef<T> tmp(*m_ptr); m_ptr = nullptr; return tmp; }
Clumsy name. Not sure we need this. Could we add PassRef(RefPtr&&) instead and do this? RefPtr<Style> xxx; setStyle(std::move(xxx)); The PassRef constructor that takes a RefPtr&& could ASSERT. It would be nice if we could get a "*" in there somehow, but I have no idea how. Long term we will be getting rid of PassRefPtr and release, so it would be nicer if we could do this in terms of move rather than release.
> Source/WebCore/css/StyleResolver.cpp:793 > + s_styleNotYetAvailable = RefPtr<RenderStyle>(RenderStyle::create()).release().leakRef();
Why not just implement a leakRef for PassRef instead of doing this beautiful dance? I have to admit I have quite of few of these RefPtr.release.leakRef in a patch that I decided not to land because that is too ugly.
> Source/WebCore/page/animation/AnimationController.cpp:526 > + // FIXME: This could be a PassRef<RenderStyle>. > + RefPtr<RenderStyle> blendedStyle = rendererAnimations.animate(&renderer, oldStyle, &newStyleBeforeAnimation.get());
Why isn’t this a PassRef?
>> Source/WebCore/rendering/style/RenderStyle.cpp:85 >> + static NeverDestroyed<Ref<RenderStyle>> style(RenderStyle::createDefaultStyle());; > > More than one command on the same line [whitespace/newline] [4]
Extra semicolon, he’s right. NeverDestroyed<Ref> is way overkill here. Just a plain old reference would be better: static RenderStyle& defaultStyle = RenderStyle::createDefaultStyle().leakRef(); return defaultStyle;
> Source/WebCore/style/StyleResolveTree.cpp:806 > + documentStyle.dropReference();
Wow, so lame that we have to do this.
Darin Adler
Comment 4
2013-10-18 19:26:55 PDT
Comment on
attachment 214623
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=214623&action=review
>> Source/WebCore/css/StyleResolver.cpp:793 >> + s_styleNotYetAvailable = RefPtr<RenderStyle>(RenderStyle::create()).release().leakRef(); > > Why not just implement a leakRef for PassRef instead of doing this beautiful dance? I have to admit I have quite of few of these RefPtr.release.leakRef in a patch that I decided not to land because that is too ugly.
I think takeReference is already leakRef, just with the wrong name. We should just rename it to leakRef.
Andreas Kling
Comment 5
2013-10-19 05:28:17 PDT
(In reply to
comment #3
)
> (From update of
attachment 214623
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=214623&action=review
> > Source/WTF/wtf/PassRef.h:46 > > + const T* operator->() const; > > + T* operator->(); > > I’m not so glad you are adding these. I hate using -> when it’s a reference. Honestly, I’d rather see .get(). in all those places. Just so there is no -> that is not for a pointer. Same for Ref, really.
I hate it too. I will drop operator-> from this patch.
> > Source/WTF/wtf/PassRef.h:50 > > + void dropReference(); > > Seems sad that we really need this.
I'm kinda hoping that we can come up with a better idiom as we learn more about this technique.
> > Source/WTF/wtf/RefPtr.h:61 > > + PassRef<T> releaseNonNull() { ASSERT(m_ptr); PassRef<T> tmp(*m_ptr); m_ptr = nullptr; return tmp; } > > Clumsy name. Not sure we need this. Could we add PassRef(RefPtr&&) instead and do this? > > RefPtr<Style> xxx; > setStyle(std::move(xxx)); > > The PassRef constructor that takes a RefPtr&& could ASSERT. It would be nice if we could get a "*" in there somehow, but I have no idea how.
The lack of * is indeed why I didn't do this. I feel that the pointer dereferencing needs to be explicit at the call site somehow.
> Long term we will be getting rid of PassRefPtr and release, so it would be nicer if we could do this in terms of move rather than release.
I agree that it would be nicer, it's just not clear to me how we will accomplish that.
> > Source/WebCore/css/StyleResolver.cpp:793 > > + s_styleNotYetAvailable = RefPtr<RenderStyle>(RenderStyle::create()).release().leakRef(); > > Why not just implement a leakRef for PassRef instead of doing this beautiful dance? I have to admit I have quite of few of these RefPtr.release.leakRef in a patch that I decided not to land because that is too ugly.
Sure. My patch was originally renaming takeReference() to leakReference() and making it public. I ended up holding back on that since it was only affecting one call site, but you're right. Let's get it done.
> > Source/WebCore/page/animation/AnimationController.cpp:526 > > + // FIXME: This could be a PassRef<RenderStyle>. > > + RefPtr<RenderStyle> blendedStyle = rendererAnimations.animate(&renderer, oldStyle, &newStyleBeforeAnimation.get()); > > Why isn’t this a PassRef?
Because of patch snowballing. Can and will be done later, hence the FIXME.
> NeverDestroyed<Ref> is way overkill here. Just a plain old reference would be better: > > static RenderStyle& defaultStyle = RenderStyle::createDefaultStyle().leakRef(); > return defaultStyle;
Will do.
Andreas Kling
Comment 6
2013-10-19 06:57:34 PDT
Committed
r157665
: <
http://trac.webkit.org/changeset/157665
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug