Bug 139587 – Replace PassRef with Ref/Ref&& across the board.

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Andreas Kling]

If we allow Ref to become "invalid" after moving out of it, we don't need the PassRef class at all.
Ref<T> for return values, Ref<T>&& for arguments.

[Andreas Kling]

Created attachment 243199 [details]
Snack for EWS

[Andreas Kling]

Created attachment 243200 [details]
Snack for EWS II

[WebKit Commit Bot]

Attachment 243200 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/PassRefPtr.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/Font.cpp:218:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/style/DataRef.h:33:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Ref.h:104:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Ref.h:109:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Ref.h:120:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:118:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 582 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Created attachment 243207 [details]
Potentially a patch

[WebKit Commit Bot]

Attachment 243207 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/PassRefPtr.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/style/DataRef.h:33:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Ref.h:104:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Ref.h:109:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:118:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 599 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Created attachment 243220 [details]
Patch

[WebKit Commit Bot]

Attachment 243220 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/PassRefPtr.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/style/DataRef.h:33:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Ref.h:105:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Ref.h:110:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:118:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 599 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 243220 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243220&action=review

Nice.

If we forget to use WTF::move when passing a Ref to a Ref&& argument, what happens? Do we get a compilation error?

I’m also a bit confused about why we have to use WTF::move to when assigning to a Ref from a Ref&& argument. What does the WTF::move accomplish again? Same question but about the case where we are passing a Ref&& argument into another function that takes a Ref&&. I think we have a *lot* of WTF::move that we could remove now that we are using Ref&& instead of PassRef, unless I’m mixed up about something.

> Source/WTF/wtf/text/WTFString.h:118
> -    String(PassRef<StringImpl>&& impl) : m_impl(std::forward<PassRef<StringImpl>>(impl)) { }
> +    String(Ref<StringImpl>&& impl) : m_impl(std::forward<Ref<StringImpl>>(impl)) { }

I would have used WTF::move here. Is std::forward better for some reason? I was under the impression that std::forward typically only was needed when there were templates involved and the precise type was not known.

> Source/WTF/wtf/text/WTFString.h:515
> -    : m_impl(vector.size() ? StringImpl::create(vector.data(), vector.size()) : *StringImpl::empty())
> +    : m_impl(vector.size() ? StringImpl::create(vector.data(), vector.size()) : Ref<StringImpl>(*StringImpl::empty()))

Explicit construction here is annoying; I see it’s needed in a few places. More on this in a comment below.

> Source/WebCore/css/CSSFunctionValue.h:43
> +    static Ref<CSSFunctionValue> create(String name, PassRefPtr<CSSValueList> args)

Peculiar that this uses String. I would have expected const String& and I was wondering if String is used whether we also should be doing WTF::move on the String when passing it along.

> Source/WebCore/platform/graphics/Font.cpp:221
> +    FontGlyphsCacheEntry(FontGlyphsCacheKey&& k, Ref<FontGlyphs>&& g)
> +        : key(WTF::move(k))
> +        , glyphs(WTF::move(g))
> +    { }

We should just dump constructors like this from structs and use FontGlyphsCacheEntry{ x, y } syntax instead of a constructor.

> Source/WebCore/rendering/RenderScrollbarPart.h:38
> +    RenderScrollbarPart(Document&, Ref<RenderStyle>&&, RenderScrollbar* = 0, ScrollbarPart = NoPart);

Missed a chance to nullptr-ify here.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:258
> -    Ref<MutableStyleProperties> mutableStyle(style->isMutable() ? static_cast<MutableStyleProperties&>(*style) : style->mutableCopy());
> +    Ref<MutableStyleProperties> mutableStyle(style->isMutable() ? Ref<MutableStyleProperties>(static_cast<MutableStyleProperties&>(*style)) : style->mutableCopy());

We were getting away without the cast before. Why do we need it now?

If this was going to happen a lot, I would suggest providing a template function to wrap in a Ref like this, so we don’t have to repeat the type names such as MutableStyleProperties.

[Andreas Kling]

Comment on attachment 243220 [details]
Patch

Clearing flags on attachment: 243220

Committed r177259: <http://trac.webkit.org/changeset/177259>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

(In reply to comment #8)
> Comment on attachment 243220 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243220&action=review
> 
> Nice.
> 
> If we forget to use WTF::move when passing a Ref to a Ref&& argument, what
> happens? Do we get a compilation error?

We do. You either have to WTF::move(), or .copyRef() the Ref, in order to pass it to a Ref&& argument.

> I’m also a bit confused about why we have to use WTF::move to when assigning
> to a Ref from a Ref&& argument. What does the WTF::move accomplish again?
> Same question but about the case where we are passing a Ref&& argument into
> another function that takes a Ref&&. I think we have a *lot* of WTF::move
> that we could remove now that we are using Ref&& instead of PassRef, unless
> I’m mixed up about something.

AFAICT that is still necessary.

> > Source/WTF/wtf/text/WTFString.h:118
> > -    String(PassRef<StringImpl>&& impl) : m_impl(std::forward<PassRef<StringImpl>>(impl)) { }
> > +    String(Ref<StringImpl>&& impl) : m_impl(std::forward<Ref<StringImpl>>(impl)) { }
> 
> I would have used WTF::move here. Is std::forward better for some reason? I
> was under the impression that std::forward typically only was needed when
> there were templates involved and the precise type was not known.

> 
> > Source/WebCore/css/CSSFunctionValue.h:43
> > +    static Ref<CSSFunctionValue> create(String name, PassRefPtr<CSSValueList> args)
> 
> Peculiar that this uses String. I would have expected const String& and I
> was wondering if String is used whether we also should be doing WTF::move on
> the String when passing it along.

Will tweak.

> > Source/WebCore/platform/graphics/Font.cpp:221
> > +    FontGlyphsCacheEntry(FontGlyphsCacheKey&& k, Ref<FontGlyphs>&& g)
> > +        : key(WTF::move(k))
> > +        , glyphs(WTF::move(g))
> > +    { }
> 
> We should just dump constructors like this from structs and use
> FontGlyphsCacheEntry{ x, y } syntax instead of a constructor.

Will tweak.

> > Source/WebCore/rendering/RenderScrollbarPart.h:38
> > +    RenderScrollbarPart(Document&, Ref<RenderStyle>&&, RenderScrollbar* = 0, ScrollbarPart = NoPart);
> 
> Missed a chance to nullptr-ify here.

Will tweak.

> 
> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:258
> > -    Ref<MutableStyleProperties> mutableStyle(style->isMutable() ? static_cast<MutableStyleProperties&>(*style) : style->mutableCopy());
> > +    Ref<MutableStyleProperties> mutableStyle(style->isMutable() ? Ref<MutableStyleProperties>(static_cast<MutableStyleProperties&>(*style)) : style->mutableCopy());
> 
> We were getting away without the cast before. Why do we need it now?

Because Chris added an operator T& to Ref, so now that ternary becomes ambiguous.

> 
> If this was going to happen a lot, I would suggest providing a template
> function to wrap in a Ref like this, so we don’t have to repeat the type
> names such as MutableStyleProperties.

I don't think it'll happen a lot, though that's not a bad idea either.

[Anders Carlsson]

(In reply to comment #11)
> (In reply to comment #8)
> > Comment on attachment 243220 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=243220&action=review
> > 
> > Nice.
> > 
> > If we forget to use WTF::move when passing a Ref to a Ref&& argument, what
> > happens? Do we get a compilation error?
> 
> We do. You either have to WTF::move(), or .copyRef() the Ref, in order to
> pass it to a Ref&& argument.
> 
> > I’m also a bit confused about why we have to use WTF::move to when assigning
> > to a Ref from a Ref&& argument. What does the WTF::move accomplish again?
> > Same question but about the case where we are passing a Ref&& argument into
> > another function that takes a Ref&&. I think we have a *lot* of WTF::move
> > that we could remove now that we are using Ref&& instead of PassRef, unless
> > I’m mixed up about something.
> 
> AFAICT that is still necessary.

Anything that has a name is considered an lvalue, even if it's an rvalue reference.

> 
> > > Source/WTF/wtf/text/WTFString.h:118
> > > -    String(PassRef<StringImpl>&& impl) : m_impl(std::forward<PassRef<StringImpl>>(impl)) { }
> > > +    String(Ref<StringImpl>&& impl) : m_impl(std::forward<Ref<StringImpl>>(impl)) { }
> > 
> > I would have used WTF::move here. Is std::forward better for some reason? I
> > was under the impression that std::forward typically only was needed when
> > there were templates involved and the precise type was not known.

Yes, WTF::forward should only be used when the parameter is a "forwarding reference". (T&& where T is a template parameter).