[webkit-reviews] review granted: [Bug 123051] Start passing RenderStyle around with PassRef. : [Attachment 214623] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 123051: Start passing RenderStyle around with PassRef.
https://bugs.webkit.org/show_bug.cgi?id=123051

Attachment 214623: Patch
https://bugs.webkit.org/attachment.cgi?id=214623&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.