Bug 123051 – Start passing RenderStyle around with PassRef.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Andreas Kling]

Time to kick this up a notch.

[Andreas Kling]

Created attachment 214623 [details]
Patch

[WebKit Commit Bot]

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 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 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]

(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]

Committed r157665: <http://trac.webkit.org/changeset/157665>