Bug 178612 – Let is<T>() accept RefPtrs

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Jiewen Tan]

Let is<T>() and downcast<T>() accept RefPtrs/Refs.

[Jiewen Tan]

<rdar://problem/35102004>

[Jiewen Tan]

Created attachment 324480 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1710:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 97 files


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

[Daniel Bates]

Comment on attachment 324480 [details]
Patch

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

> Source/WTF/ChangeLog:11
> +        safecasting methods to accept RefPtrs and Refs. Noted, downcast<T>() returns raw
> +        pointers for RefPtr and raw references for Ref to avoid ref churn.

Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?

[Ryosuke Niwa]

Comment on attachment 324480 [details]
Patch

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

>> Source/WTF/ChangeLog:11
>> +        pointers for RefPtr and raw references for Ref to avoid ref churn.
> 
> Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?

Yeah, that seems bad. We shouldn't be returning a raw pointer/reference.

[Jiewen Tan]

(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 324480 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324480&action=review
> 
> >> Source/WTF/ChangeLog:11
> >> +        pointers for RefPtr and raw references for Ref to avoid ref churn.
> > 
> > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?
> 
> Yeah, that seems bad. We shouldn't be returning a raw pointer/reference.

Then are we okay with the unnecessary ref counting? If so, I am willing to change them return RefPtr<T> and Ref<T>.

[Chris Dumez]

(In reply to Jiewen Tan from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > Comment on attachment 324480 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=324480&action=review
> > 
> > >> Source/WTF/ChangeLog:11
> > >> +        pointers for RefPtr and raw references for Ref to avoid ref churn.
> > > 
> > > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?
> > 
> > Yeah, that seems bad. We shouldn't be returning a raw pointer/reference.
> 
> Then are we okay with the unnecessary ref counting? If so, I am willing to
> change them return RefPtr<T> and Ref<T>.

You should be able to avoid ref counting churn. Did you look at static_pointer_cast ?

[Jiewen Tan]

(In reply to Chris Dumez from comment #7)
> (In reply to Jiewen Tan from comment #6)
> > (In reply to Ryosuke Niwa from comment #5)
> > > Comment on attachment 324480 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=324480&action=review
> > > 
> > > >> Source/WTF/ChangeLog:11
> > > >> +        pointers for RefPtr and raw references for Ref to avoid ref churn.
> > > > 
> > > > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?
> > > 
> > > Yeah, that seems bad. We shouldn't be returning a raw pointer/reference.
> > 
> > Then are we okay with the unnecessary ref counting? If so, I am willing to
> > change them return RefPtr<T> and Ref<T>.
> 
> You should be able to avoid ref counting churn. Did you look at
> static_pointer_cast ?

It is not obvious to me that how it could avoid ref counting churn. Anyway, proceeding with the suggestions.

[Chris Dumez]

(In reply to Jiewen Tan from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Jiewen Tan from comment #6)
> > > (In reply to Ryosuke Niwa from comment #5)
> > > > Comment on attachment 324480 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=324480&action=review
> > > > 
> > > > >> Source/WTF/ChangeLog:11
> > > > >> +        pointers for RefPtr and raw references for Ref to avoid ref churn.
> > > > > 
> > > > > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?
> > > > 
> > > > Yeah, that seems bad. We shouldn't be returning a raw pointer/reference.
> > > 
> > > Then are we okay with the unnecessary ref counting? If so, I am willing to
> > > change them return RefPtr<T> and Ref<T>.
> > 
> > You should be able to avoid ref counting churn. Did you look at
> > static_pointer_cast ?
> 
> It is not obvious to me that how it could avoid ref counting churn. Anyway,
> proceeding with the suggestions.

Yeah, it seems I remembered it wrong. I took a look at static_pointer_cast() and it does not avoid ref counting churn.

[Chris Dumez]

Comment on attachment 324480 [details]
Patch

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

>>>>>>> Source/WTF/ChangeLog:11
>>>>>>> +        pointers for RefPtr and raw references for Ref to avoid ref churn.
>>>>>> 
>>>>>> Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?
>>>>> 
>>>>> Yeah, that seems bad. We shouldn't be returning a raw pointer/reference.
>>>> 
>>>> Then are we okay with the unnecessary ref counting? If so, I am willing to change them return RefPtr<T> and Ref<T>.
>>> 
>>> You should be able to avoid ref counting churn. Did you look at static_pointer_cast ?
>> 
>> It is not obvious to me that how it could avoid ref counting churn. Anyway, proceeding with the suggestions.
> 
> Yeah, it seems I remembered it wrong. I took a look at static_pointer_cast() and it does not avoid ref counting churn.

One way to avoid the refcounting churn would be to have a downcast<> that takes in an Ref<>&& / RefPtr<>&& in parameter. static_pointer_cast<>() does this for RenderPtr.

[Jiewen Tan]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 324480 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324480&action=review
> 
> >>>>>>> Source/WTF/ChangeLog:11
> >>>>>>> +        pointers for RefPtr and raw references for Ref to avoid ref churn.
> >>>>>> 
> >>>>>> Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?
> >>>>> 
> >>>>> Yeah, that seems bad. We shouldn't be returning a raw pointer/reference.
> >>>> 
> >>>> Then are we okay with the unnecessary ref counting? If so, I am willing to change them return RefPtr<T> and Ref<T>.
> >>> 
> >>> You should be able to avoid ref counting churn. Did you look at static_pointer_cast ?
> >> 
> >> It is not obvious to me that how it could avoid ref counting churn. Anyway, proceeding with the suggestions.
> > 
> > Yeah, it seems I remembered it wrong. I took a look at static_pointer_cast() and it does not avoid ref counting churn.
> 
> One way to avoid the refcounting churn would be to have a downcast<> that
> takes in an Ref<>&& / RefPtr<>&& in parameter. static_pointer_cast<>() does
> this for RenderPtr.

This doesn't sound elegant as the behaviors are different from the normal downcast<>().

[Jiewen Tan]

By changing the return type, it reveals that the compiler will actually confuse between the newly added:
template<typename ExpectedType, typename ArgType>
inline Ref<ExpectedType> downcast(Ref<ArgType>& source)
{
    return downcast<ExpectedType>(source.get());
}
and the original one. The compiler will call the new one even if the caller passes an ArgType reference. Not sure how to deal with this issue.

[Jiewen Tan]

(In reply to Jiewen Tan from comment #12)
> By changing the return type, it reveals that the compiler will actually
> confuse between the newly added:
> template<typename ExpectedType, typename ArgType>
> inline Ref<ExpectedType> downcast(Ref<ArgType>& source)
> {
>     return downcast<ExpectedType>(source.get());
> }
> and the original one. The compiler will call the new one even if the caller
> passes an ArgType reference. Not sure how to deal with this issue.

Just spoke to Chris, he said this implicit conversion is done on purpose. He is suggesting to have syntax like downcast<Ref<T>>().

However, this doesn't solve my initial problems. I am trying to attack this problem as it is very annoying to add .get() after I have changed the type of a raw pointer to a RefPtr if the pointer is used in a is<T>() and downcast<T>() later on. With the new downcast<Ref<T>>() syntax, I still have to make changes after I have changed the type.

Without any other proper solutions, I might just go with the is<T>() alone.

[Jiewen Tan]

(In reply to Jiewen Tan from comment #13)
> (In reply to Jiewen Tan from comment #12)
> > By changing the return type, it reveals that the compiler will actually
> > confuse between the newly added:
> > template<typename ExpectedType, typename ArgType>
> > inline Ref<ExpectedType> downcast(Ref<ArgType>& source)
> > {
> >     return downcast<ExpectedType>(source.get());
> > }
> > and the original one. The compiler will call the new one even if the caller
> > passes an ArgType reference. Not sure how to deal with this issue.
> 
> Just spoke to Chris, he said this implicit conversion is done on purpose. He
> is suggesting to have syntax like downcast<Ref<T>>().
> 
> However, this doesn't solve my initial problems. I am trying to attack this
> problem as it is very annoying to add .get() after I have changed the type
> of a raw pointer to a RefPtr if the pointer is used in a is<T>() and
> downcast<T>() later on. With the new downcast<Ref<T>>() syntax, I still have
> to make changes after I have changed the type.
> 
> Without any other proper solutions, I might just go with the is<T>() alone.

Sorry, it turns out to be not the case. It is very specific to containers that define iterators as a Ref<T> object.

[Jiewen Tan]

Created attachment 324729 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1710:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 112 files


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

[Build Bot]

Comment on attachment 324729 [details]
Patch

Attachment 324729 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4975456

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 324743 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Jiewen Tan]

I don't think the mac-wk2 is related to my patch as I cannot reproduce any of those failures in my local machine.

[Jiewen Tan]

Created attachment 324778 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1710:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 112 files


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

[Ryosuke Niwa]

Comment on attachment 324778 [details]
Patch

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

> Source/WTF/wtf/Ref.h:251
> +inline const Ref<ExpectedType> downcast(const Ref<ArgType>& source)

We should add another variant which takes Ref<ArgType>&& when we can avoid a ref churn.

> Source/WTF/wtf/RefPtr.h:260
> +inline const RefPtr<ExpectedType> downcast(const RefPtr<ArgType>& source)

Ditto about adding the variant for RefPtr<ArgType>&&

> Source/WebCore/accessibility/AccessibilityListBoxOption.h:66
> -    HTMLSelectElement* listBoxOptionParentNode() const;
> +    RefPtr<HTMLSelectElement> listBoxOptionParentNode() const;

Looks like this is unrelated change?

[Jiewen Tan]

Comment on attachment 324778 [details]
Patch

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

Thanks Ryosuke for reviewing the patch. Besides adding the move version, I have also added WTFMove() to whenever appropriate.

>> Source/WTF/wtf/Ref.h:251
>> +inline const Ref<ExpectedType> downcast(const Ref<ArgType>& source)
> 
> We should add another variant which takes Ref<ArgType>&& when we can avoid a ref churn.

Sure.

>> Source/WTF/wtf/RefPtr.h:260
>> +inline const RefPtr<ExpectedType> downcast(const RefPtr<ArgType>& source)
> 
> Ditto about adding the variant for RefPtr<ArgType>&&

Fixed.

>> Source/WebCore/accessibility/AccessibilityListBoxOption.h:66
>> +    RefPtr<HTMLSelectElement> listBoxOptionParentNode() const;
> 
> Looks like this is unrelated change?

When adding .get() after some of the downcast operations, I picked some that can be benefit by changing the return type.

[Jiewen Tan]

Created attachment 324881 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1710:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 112 files


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

[Build Bot]

Comment on attachment 324881 [details]
Patch

Attachment 324881 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4988457

New failing tests:
media/track/track-webvtt-tc026-voice.html
media/track/track-cue-mutable-fragment.html
http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm
media/track/getCueAsHTMLCrash.html
media/track/track-webvtt-tc023-markup.html
media/track/track-webvtt-tc025-class-markup.html

[Build Bot]

Created attachment 324897 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 324881 [details]
Patch

Attachment 324881 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4988322

New failing tests:
media/track/track-webvtt-tc026-voice.html
media/track/getCueAsHTMLCrash.html
media/track/track-webvtt-tc023-markup.html
media/track/track-cue-mutable-fragment.html
media/track/track-webvtt-tc025-class-markup.html

[Build Bot]

Created attachment 324900 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jiewen Tan]

Created attachment 324908 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1710:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 112 files


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

[Ryosuke Niwa]

Comment on attachment 324908 [details]
Patch

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

> Source/WTF/wtf/Ref.h:245
> +inline Ref<ExpectedType> downcast(Ref<ArgType>& source)

I've started to think that we might want to call this variant downcastAndCopyRef or something.
The fact downcast<~> sometimes implicitly calls ref()'s is very subtle.

Another alternative Geoff and I discussed would be for this function to return ExpectedType& or Ref<ExpectedType>&.
For the case of Ref<ExpectedType>&, we would be casting Ref<~> itself, not the content of it.

> Source/WTF/wtf/Ref.h:251
> +inline const Ref<ExpectedType> downcast(const Ref<ArgType>& source)

Ditto.

> Source/WTF/wtf/RefPtr.h:254
> +inline RefPtr<ExpectedType> downcast(RefPtr<ArgType>& source)

Again, we might want to call this variant downcastAndCopyRef or copyRefToDowncast

> Source/WebCore/css/CSSFontFace.cpp:66
> -        CSSFontFaceSrcValue& item = downcast<CSSFontFaceSrcValue>(src.get());
> +        auto item = downcast<CSSFontFaceSrcValue>(src);

This code, for example, calls the variant which ref()'s and it's not obvious at all from the call site.

> Source/WebCore/css/CSSFontFaceSet.cpp:158
> -        String familyName = CSSFontFaceSet::familyNameFromPrimitive(downcast<CSSPrimitiveValue>(item.get()));
> +        String familyName = CSSFontFaceSet::familyNameFromPrimitive(downcast<CSSPrimitiveValue>(item));

Again, this copies ref().

> Source/WebCore/css/StyleBuilderConverter.h:392
> +        for (const auto& currentValue : downcast<CSSValueList>(value))

It's unfortunate that the type of currentValue, const Ref<CSSValue>&, is not obvious at all from the code inspection.

> Source/WebCore/css/StyleBuilderCustom.h:393
> -        CSSPrimitiveValue& primitiveValue = downcast<CSSPrimitiveValue>(item.get());
> +        CSSPrimitiveValue& primitiveValue = downcast<CSSPrimitiveValue>(item);

This seems wrong. Why is there an implicit coercion from Ref<CSSValue> to CSSPrimitiveValue& happening?

> Source/WebCore/css/StyleBuilderCustom.h:514
> +        auto primitiveValue = downcast<CSSPrimitiveValue>(item);

Ditto.

> Source/WebCore/css/StyleBuilderCustom.h:836
> -        auto& shadowValue = downcast<CSSShadowValue>(item.get());
> +        auto shadowValue = downcast<CSSShadowValue>(item);

Ditto.

> Source/WebCore/css/StyleBuilderCustom.h:940
> -        auto& contentValue = downcast<CSSPrimitiveValue>(item.get());
> +        auto contentValue = downcast<CSSPrimitiveValue>(item);

Ditto.

[Jiewen Tan]

Comment on attachment 324908 [details]
Patch

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

>> Source/WTF/wtf/Ref.h:245
>> +inline Ref<ExpectedType> downcast(Ref<ArgType>& source)
> 
> I've started to think that we might want to call this variant downcastAndCopyRef or something.
> The fact downcast<~> sometimes implicitly calls ref()'s is very subtle.
> 
> Another alternative Geoff and I discussed would be for this function to return ExpectedType& or Ref<ExpectedType>&.
> For the case of Ref<ExpectedType>&, we would be casting Ref<~> itself, not the content of it.

I like the idea of returning Ref<ExpectedType>&. Let's proceed with that.

[Jiewen Tan]

Created attachment 324976 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1710:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 112 files


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

[Build Bot]

Comment on attachment 324976 [details]
Patch

Attachment 324976 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4996238

New failing tests:
dom/html/level2/core/hc_nodedocumentfragmentnormalize2.html
fast/dom/Node/normalize.html
dom/xhtml/level2/core/hc_nodedocumentfragmentnormalize2.xhtml
imported/w3c/web-platform-tests/dom/nodes/Node-normalize.html

[Build Bot]

Created attachment 324981 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 324976 [details]
Patch

Attachment 324976 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4996313

New failing tests:
dom/xhtml/level2/core/hc_nodedocumentfragmentnormalize2.xhtml
fast/dom/Node/normalize.html
dom/html/level2/core/hc_nodedocumentfragmentnormalize2.html
imported/w3c/web-platform-tests/dom/nodes/Node-normalize.html

[Build Bot]

Created attachment 324983 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 324976 [details]
Patch

Attachment 324976 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4997283

New failing tests:
dom/html/level2/core/hc_nodedocumentfragmentnormalize2.html
fast/dom/Node/normalize.html
dom/xhtml/level2/core/hc_nodedocumentfragmentnormalize2.xhtml
imported/w3c/web-platform-tests/dom/nodes/Node-normalize.html

[Build Bot]

Created attachment 324996 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Build Bot]

Comment on attachment 324976 [details]
Patch

Attachment 324976 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4998630

New failing tests:
dom/xhtml/level2/core/hc_nodedocumentfragmentnormalize2.xhtml
fast/dom/Node/normalize.html
dom/html/level2/core/hc_nodedocumentfragmentnormalize2.html
imported/w3c/web-platform-tests/dom/nodes/Node-normalize.html

[Build Bot]

Created attachment 325013 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Jiewen Tan]

Created attachment 325042 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1710:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 112 files


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

[Chris Dumez]

Comment on attachment 325042 [details]
Patch

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

> Source/WTF/wtf/RefPtr.h:242
> +inline bool is(RefPtr<ArgType>& source)

Do we really need this one? isn't the const one below sufficient?

> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:86
> +    auto listBoxParentNode = listBoxOptionParentNode();

Not sure we should be making these unrelated changes in this patch.

> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:191
> +RefPtr<HTMLSelectElement> AccessibilityListBoxOption::listBoxOptionParentNode() const

Not sure we should be making these unrelated changes in this patch.

> Source/WebCore/accessibility/AccessibilityListBoxOption.h:66
> +    RefPtr<HTMLSelectElement> listBoxOptionParentNode() const;

Not sure we should be making these unrelated changes in this patch.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2360
> +    RefPtr<Node> node = hitTestResult.innerNode();

Not sure we should be making these unrelated changes in this patch.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:151
> +    const auto hmacKey = downcast<CryptoKeyHMAC>(WTFMove(key));

no need for const.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:199
> +    const auto rsaKey = downcast<CryptoKeyRSA>(WTFMove(key));

no need for the const.

> Source/WebCore/css/StyleBuilderConverter.h:392
> +        for (const auto& currentValue : downcast<CSSValueList>(value))

I don't think we should add those const here. We usually don't have const variables in WebKit. I remember Darin asking me to not use const in such cases too.

> Source/WebCore/css/StyleBuilderConverter.h:496
> +    for (const auto& currentValue : downcast<CSSValueList>(value))

ditto.

> Source/WebCore/css/StyleBuilderConverter.h:534
> +    for (const auto& currentValue : downcast<CSSValueList>(value)) {

ditto.

> Source/WebCore/css/StyleBuilderConverter.h:535
> +        const auto& primitiveValue = downcast<CSSPrimitiveValue>(currentValue);

ditto.

> Source/WebCore/dom/Node.cpp:588
> +        auto text = downcast<Text>(node).copyRef();

Is the copyRef() really needed? Doesn't this return a RefPtr?

[Chris Dumez]

Comment on attachment 325042 [details]
Patch

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

> Source/WTF/wtf/RefPtr.h:258
> +    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(is<ExpectedType>(source.get()));

So we pay a release-time cost just for casting?

> Source/WTF/wtf/RefPtr.h:267
> +    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(is<ExpectedType>(source.get()));

So we pay a release-time cost just for casting?

[Chris Dumez]

Comment on attachment 325042 [details]
Patch

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

> Source/WTF/wtf/RefPtr.h:259
> +    return reinterpret_cast<RefPtr<ExpectedType>&>(source);

Why is this reinterpret_cast safe? What about multiple inheritance?

class A { }
class B : public RefCounted<B> { }

class C : public A, public B { }

RefPtr<B> b = adoptRef(new C);
RefPtr<C> c = downcast<C>(b);

Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no?

[Jiewen Tan]

Comment on attachment 325042 [details]
Patch

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

Thanks Chris for reviewing my patch.

>> Source/WTF/wtf/RefPtr.h:242
>> +inline bool is(RefPtr<ArgType>& source)
> 
> Do we really need this one? isn't the const one below sufficient?

No, it is required. Otherwise, the compiler will then choose the original is<T>() instead of the latter one.

>> Source/WTF/wtf/RefPtr.h:258
>> +    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(is<ExpectedType>(source.get()));
> 
> So we pay a release-time cost just for casting?

I got warning from our style checker ask me to switch a debug assert to a release assert. Probably, we don't want to enable it for casting at this moment.

>> Source/WTF/wtf/RefPtr.h:259
>> +    return reinterpret_cast<RefPtr<ExpectedType>&>(source);
> 
> Why is this reinterpret_cast safe? What about multiple inheritance?
> 
> class A { }
> class B : public RefCounted<B> { }
> 
> class C : public A, public B { }
> 
> RefPtr<B> b = adoptRef(new C);
> RefPtr<C> c = downcast<C>(b);
> 
> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no?

RefPtr<C> c = downcast<C>(b);

I don't think the above semantic is allowed after this change. It will actually be:
RefPtr<C>& c = downcast<C>(b);

Since c is just an alias of b, yes, they share the same address.

>> Source/WTF/wtf/RefPtr.h:267
>> +    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(is<ExpectedType>(source.get()));
> 
> So we pay a release-time cost just for casting?

Ditto.

>> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:86
>> +    auto listBoxParentNode = listBoxOptionParentNode();
> 
> Not sure we should be making these unrelated changes in this patch.

It is a long story. Let's put it in short, some methods return downcast<T>() directly. After this change, downcast<T>() will return a Ref/RefPtr via the move downcast variant. Therefore, I felt it would be awkward to add .get() after downcast<T>(). Therefore, instead of doing that, I change the return type of those methods to Ref/RefPtr. And then, recursion happens. Consequently, I changed methods that return the mentioned before methods directly to Ref/RefPtr. That's how this seem unrelated ends up with.

In this cast, it is:
ownerSelectElement()
that introduces.

>> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:191
>> +RefPtr<HTMLSelectElement> AccessibilityListBoxOption::listBoxOptionParentNode() const
> 
> Not sure we should be making these unrelated changes in this patch.

Ditto.

>> Source/WebCore/accessibility/AccessibilityListBoxOption.h:66
>> +    RefPtr<HTMLSelectElement> listBoxOptionParentNode() const;
> 
> Not sure we should be making these unrelated changes in this patch.

Ditto.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2360
>> +    RefPtr<Node> node = hitTestResult.innerNode();
> 
> Not sure we should be making these unrelated changes in this patch.

I cannot recalled why I did these changes. Discarded...

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:151
>> +    const auto hmacKey = downcast<CryptoKeyHMAC>(WTFMove(key));
> 
> no need for const.

Removed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:199
>> +    const auto rsaKey = downcast<CryptoKeyRSA>(WTFMove(key));
> 
> no need for the const.

Ditto.

>> Source/WebCore/css/StyleBuilderConverter.h:392
>> +        for (const auto& currentValue : downcast<CSSValueList>(value))
> 
> I don't think we should add those const here. We usually don't have const variables in WebKit. I remember Darin asking me to not use const in such cases too.

Removed.

>> Source/WebCore/css/StyleBuilderConverter.h:496
>> +    for (const auto& currentValue : downcast<CSSValueList>(value))
> 
> ditto.

Ditto.

>> Source/WebCore/css/StyleBuilderConverter.h:534
>> +    for (const auto& currentValue : downcast<CSSValueList>(value)) {
> 
> ditto.

Ditto.

>> Source/WebCore/css/StyleBuilderConverter.h:535
>> +        const auto& primitiveValue = downcast<CSSPrimitiveValue>(currentValue);
> 
> ditto.

Ditto.

[Jiewen Tan]

Comment on attachment 325042 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2360
>>> +    RefPtr<Node> node = hitTestResult.innerNode();
>> 
>> Not sure we should be making these unrelated changes in this patch.
> 
> I cannot recalled why I did these changes. Discarded...

Okay, I recalled. We assigned:
node = downcast<HTMLOptionElement>(*node).ownerSelectElement();
Therefore, I change node from Node* to RefPtr<Node>.

[Chris Dumez]

Comment on attachment 325042 [details]
Patch

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

>>> Source/WTF/wtf/RefPtr.h:259
>>> +    return reinterpret_cast<RefPtr<ExpectedType>&>(source);
>> 
>> Why is this reinterpret_cast safe? What about multiple inheritance?
>> 
>> class A { }
>> class B : public RefCounted<B> { }
>> 
>> class C : public A, public B { }
>> 
>> RefPtr<B> b = adoptRef(new C);
>> RefPtr<C> c = downcast<C>(b);
>> 
>> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no?
> 
> RefPtr<C> c = downcast<C>(b);
> 
> I don't think the above semantic is allowed after this change. It will actually be:
> RefPtr<C>& c = downcast<C>(b);
> 
> Since c is just an alias of b, yes, they share the same address.

I think you understood me wrong:
b.get() and c.get() are supposed to return different addresses because of multiple inheritance. However, I believe you may end up with the same address because of your reinterpret_cast.
If so, it would likely lead to crashes when trying to call methods inherited from A on c.

[Jiewen Tan]

Comment on attachment 325042 [details]
Patch

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

>>>> Source/WTF/wtf/RefPtr.h:259
>>>> +    return reinterpret_cast<RefPtr<ExpectedType>&>(source);
>>> 
>>> Why is this reinterpret_cast safe? What about multiple inheritance?
>>> 
>>> class A { }
>>> class B : public RefCounted<B> { }
>>> 
>>> class C : public A, public B { }
>>> 
>>> RefPtr<B> b = adoptRef(new C);
>>> RefPtr<C> c = downcast<C>(b);
>>> 
>>> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no?
>> 
>> RefPtr<C> c = downcast<C>(b);
>> 
>> I don't think the above semantic is allowed after this change. It will actually be:
>> RefPtr<C>& c = downcast<C>(b);
>> 
>> Since c is just an alias of b, yes, they share the same address.
> 
> I think you understood me wrong:
> b.get() and c.get() are supposed to return different addresses because of multiple inheritance. However, I believe you may end up with the same address because of your reinterpret_cast.
> If so, it would likely lead to crashes when trying to call methods inherited from A on c.

Okay, I got you.

[Geoffrey Garen]

> >> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no?
> > 
> > RefPtr<C> c = downcast<C>(b);
> > 
> > I don't think the above semantic is allowed after this change. It will actually be:
> > RefPtr<C>& c = downcast<C>(b);
> > 
> > Since c is just an alias of b, yes, they share the same address.
> 
> I think you understood me wrong:
> b.get() and c.get() are supposed to return different addresses because of
> multiple inheritance. However, I believe you may end up with the same
> address because of your reinterpret_cast.
> If so, it would likely lead to crashes when trying to call methods inherited
> from A on c.

I wonder if this is also a problem with our implementation of weak_reference_downcast<T, U>.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #53)
> > >> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no?
> > > 
> > > RefPtr<C> c = downcast<C>(b);
> > > 
> > > I don't think the above semantic is allowed after this change. It will actually be:
> > > RefPtr<C>& c = downcast<C>(b);
> > > 
> > > Since c is just an alias of b, yes, they share the same address.
> > 
> > I think you understood me wrong:
> > b.get() and c.get() are supposed to return different addresses because of
> > multiple inheritance. However, I believe you may end up with the same
> > address because of your reinterpret_cast.
> > If so, it would likely lead to crashes when trying to call methods inherited
> > from A on c.
> 
> I wonder if this is also a problem with our implementation of
> weak_reference_downcast<T, U>.

I pointed out the same thing to Jiewen offline :)

[Darin Adler]

(In reply to Geoffrey Garen from comment #53)
> I wonder if this is also a problem with our implementation of
> weak_reference_downcast<T, U>.

This can easily be tested in TestWebKitAPI, so we should add that kind of test.

[Jiewen Tan]

(In reply to Darin Adler from comment #55)
> (In reply to Geoffrey Garen from comment #53)
> > I wonder if this is also a problem with our implementation of
> > weak_reference_downcast<T, U>.
> 
> This can easily be tested in TestWebKitAPI, so we should add that kind of
> test.

Test added. Surprisingly, it passes without any crashes. Not sure if I wrote something wrong. Here is the test.

enum class Class {
    Multi,
};

struct Base : RefCounted<Base> {
    virtual Class type() const = 0;
    virtual ~Base() { }
};

struct ExtraBase {
    int returnOne() { return 1; }
};

struct Multi : ExtraBase, Base {
    virtual Class type() const override { return Class::Multi; }
};

TEST(WTF_RefPtr, MultiInherence)
{
    RefPtr<Base> p1 = adoptRef(new Multi);
    auto& p2 = downcast<Multi>(p1);
    EXPECT_NE(p1.get(), p2.get());
    EXPECT_EQ(p2->returnOne(), 1);
}

#define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \
SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \
static bool isType(const TestWebKitAPI::Base& base) { return base.type() == TestWebKitAPI::Class; } \
SPECIALIZE_TYPE_TRAITS_END()

SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi)

[Jiewen Tan]

(In reply to Jiewen Tan from comment #56)
> (In reply to Darin Adler from comment #55)
> > (In reply to Geoffrey Garen from comment #53)
> > > I wonder if this is also a problem with our implementation of
> > > weak_reference_downcast<T, U>.
> > 
> > This can easily be tested in TestWebKitAPI, so we should add that kind of
> > test.
> 
> Test added. Surprisingly, it passes without any crashes. Not sure if I wrote
> something wrong. Here is the test.
> 
> enum class Class {
>     Multi,
> };
> 
> struct Base : RefCounted<Base> {
>     virtual Class type() const = 0;
>     virtual ~Base() { }
> };
> 
> struct ExtraBase {
>     int returnOne() { return 1; }
> };
> 
> struct Multi : ExtraBase, Base {
>     virtual Class type() const override { return Class::Multi; }
> };
> 
> TEST(WTF_RefPtr, MultiInherence)
> {
>     RefPtr<Base> p1 = adoptRef(new Multi);
>     auto& p2 = downcast<Multi>(p1);
>     EXPECT_NE(p1.get(), p2.get());
>     EXPECT_EQ(p2->returnOne(), 1);
> }
> 
> #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \
> SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \
> static bool isType(const TestWebKitAPI::Base& base) { return base.type() ==
> TestWebKitAPI::Class; } \
> SPECIALIZE_TYPE_TRAITS_END()
> 
> SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi)

Sorry, it should be:
EXPECT_EQ(p1.get(), p2.get());

[Chris Dumez]

(In reply to Jiewen Tan from comment #57)
> (In reply to Jiewen Tan from comment #56)
> > (In reply to Darin Adler from comment #55)
> > > (In reply to Geoffrey Garen from comment #53)
> > > > I wonder if this is also a problem with our implementation of
> > > > weak_reference_downcast<T, U>.
> > > 
> > > This can easily be tested in TestWebKitAPI, so we should add that kind of
> > > test.
> > 
> > Test added. Surprisingly, it passes without any crashes. Not sure if I wrote
> > something wrong. Here is the test.
> > 
> > enum class Class {
> >     Multi,
> > };
> > 
> > struct Base : RefCounted<Base> {
> >     virtual Class type() const = 0;
> >     virtual ~Base() { }
> > };
> > 
> > struct ExtraBase {
> >     int returnOne() { return 1; }
> > };
> > 
> > struct Multi : ExtraBase, Base {
> >     virtual Class type() const override { return Class::Multi; }
> > };
> > 
> > TEST(WTF_RefPtr, MultiInherence)
> > {
> >     RefPtr<Base> p1 = adoptRef(new Multi);
> >     auto& p2 = downcast<Multi>(p1);
> >     EXPECT_NE(p1.get(), p2.get());
> >     EXPECT_EQ(p2->returnOne(), 1);
> > }
> > 
> > #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \
> > SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \
> > static bool isType(const TestWebKitAPI::Base& base) { return base.type() ==
> > TestWebKitAPI::Class; } \
> > SPECIALIZE_TYPE_TRAITS_END()
> > 
> > SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi)
> 
> Sorry, it should be:
> EXPECT_EQ(p1.get(), p2.get());

I think that if you'd static_cast() then p1.get() and p2.get() would be different.

[Jiewen Tan]

(In reply to Chris Dumez from comment #58)
> (In reply to Jiewen Tan from comment #57)
> > (In reply to Jiewen Tan from comment #56)
> > > (In reply to Darin Adler from comment #55)
> > > > (In reply to Geoffrey Garen from comment #53)
> > > > > I wonder if this is also a problem with our implementation of
> > > > > weak_reference_downcast<T, U>.
> > > > 
> > > > This can easily be tested in TestWebKitAPI, so we should add that kind of
> > > > test.
> > > 
> > > Test added. Surprisingly, it passes without any crashes. Not sure if I wrote
> > > something wrong. Here is the test.
> > > 
> > > enum class Class {
> > >     Multi,
> > > };
> > > 
> > > struct Base : RefCounted<Base> {
> > >     virtual Class type() const = 0;
> > >     virtual ~Base() { }
> > > };
> > > 
> > > struct ExtraBase {
> > >     int returnOne() { return 1; }
> > > };
> > > 
> > > struct Multi : ExtraBase, Base {
> > >     virtual Class type() const override { return Class::Multi; }
> > > };
> > > 
> > > TEST(WTF_RefPtr, MultiInherence)
> > > {
> > >     RefPtr<Base> p1 = adoptRef(new Multi);
> > >     auto& p2 = downcast<Multi>(p1);
> > >     EXPECT_NE(p1.get(), p2.get());
> > >     EXPECT_EQ(p2->returnOne(), 1);
> > > }
> > > 
> > > #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \
> > > SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \
> > > static bool isType(const TestWebKitAPI::Base& base) { return base.type() ==
> > > TestWebKitAPI::Class; } \
> > > SPECIALIZE_TYPE_TRAITS_END()
> > > 
> > > SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi)
> > 
> > Sorry, it should be:
> > EXPECT_EQ(p1.get(), p2.get());
> 
> I think that if you'd static_cast() then p1.get() and p2.get() would be
> different.

No, static_cast() doesn't do the trick. Here is the revised test:
TEST(WTF_RefPtr, MultiInherence)
{
    RefPtr<Base> p1 = adoptRef(new Multi);
    auto& p2 = downcast<Multi>(p1);
    EXPECT_EQ(p1.get(), p2.get());
    EXPECT_EQ(p2->returnOne(), 1);

    auto* p3 = downcast<Multi>(p1.get());
    EXPECT_NE(p1.get(), p3);
    EXPECT_EQ(p3->returnOne(), 1);
}

Output:
jwtan$ run-api-tests WTF_RefPtr.MultiInherence
Running build-api-tests
aFAIL WTF_RefPtr.MultiInherence

/Users/jwtan/Documents/Source/OpenSource/Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:563
Expected: (p1.get()) != (p3), actual: 0x1056fb000 vs 0x1056fb000


Tests that failed:
  WTF_RefPtr.MultiInherence

[Jiewen Tan]

(In reply to Jiewen Tan from comment #59)
> (In reply to Chris Dumez from comment #58)
> > (In reply to Jiewen Tan from comment #57)
> > > (In reply to Jiewen Tan from comment #56)
> > > > (In reply to Darin Adler from comment #55)
> > > > > (In reply to Geoffrey Garen from comment #53)
> > > > > > I wonder if this is also a problem with our implementation of
> > > > > > weak_reference_downcast<T, U>.
> > > > > 
> > > > > This can easily be tested in TestWebKitAPI, so we should add that kind of
> > > > > test.
> > > > 
> > > > Test added. Surprisingly, it passes without any crashes. Not sure if I wrote
> > > > something wrong. Here is the test.
> > > > 
> > > > enum class Class {
> > > >     Multi,
> > > > };
> > > > 
> > > > struct Base : RefCounted<Base> {
> > > >     virtual Class type() const = 0;
> > > >     virtual ~Base() { }
> > > > };
> > > > 
> > > > struct ExtraBase {
> > > >     int returnOne() { return 1; }
> > > > };
> > > > 
> > > > struct Multi : ExtraBase, Base {
> > > >     virtual Class type() const override { return Class::Multi; }
> > > > };
> > > > 
> > > > TEST(WTF_RefPtr, MultiInherence)
> > > > {
> > > >     RefPtr<Base> p1 = adoptRef(new Multi);
> > > >     auto& p2 = downcast<Multi>(p1);
> > > >     EXPECT_NE(p1.get(), p2.get());
> > > >     EXPECT_EQ(p2->returnOne(), 1);
> > > > }
> > > > 
> > > > #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \
> > > > SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \
> > > > static bool isType(const TestWebKitAPI::Base& base) { return base.type() ==
> > > > TestWebKitAPI::Class; } \
> > > > SPECIALIZE_TYPE_TRAITS_END()
> > > > 
> > > > SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi)
> > > 
> > > Sorry, it should be:
> > > EXPECT_EQ(p1.get(), p2.get());
> > 
> > I think that if you'd static_cast() then p1.get() and p2.get() would be
> > different.
> 
> No, static_cast() doesn't do the trick. Here is the revised test:
> TEST(WTF_RefPtr, MultiInherence)
> {
>     RefPtr<Base> p1 = adoptRef(new Multi);
>     auto& p2 = downcast<Multi>(p1);
>     EXPECT_EQ(p1.get(), p2.get());
>     EXPECT_EQ(p2->returnOne(), 1);
> 
>     auto* p3 = downcast<Multi>(p1.get());
>     EXPECT_NE(p1.get(), p3);
>     EXPECT_EQ(p3->returnOne(), 1);
> }
> 
> Output:
> jwtan$ run-api-tests WTF_RefPtr.MultiInherence
> Running build-api-tests
> aFAIL WTF_RefPtr.MultiInherence
> 
> /Users/jwtan/Documents/Source/OpenSource/Tools/TestWebKitAPI/Tests/WTF/
> RefPtr.cpp:563
> Expected: (p1.get()) != (p3), actual: 0x1056fb000 vs 0x1056fb000
> 
> 
> Tests that failed:
>   WTF_RefPtr.MultiInherence

Okay. Here is a workable test case to demonstrate the problem:
enum class Class {
    Multi,
};

struct Base : RefCounted<Base> {
    virtual Class type() const = 0;
    virtual ~Base() { }
    int someData { 0 };
};

struct ExtraBase {
    virtual ~ExtraBase() { }
    int someExtraData { 1 };
};

struct Multi : ExtraBase, Base {
    virtual Class type() const override { return Class::Multi; }
};

struct MultiReverse : Base, ExtraBase {
    virtual Class type() const override { return Class::Multi; }
};

TEST(WTF_RefPtr, MultiInherence)
{
    RefPtr<Base> p1 = adoptRef(new Multi());
    auto& p2 = downcast<Multi>(p1);
    EXPECT_EQ((void*)p1.get(), (void*)p2.get());

    auto* p3 = downcast<Multi>(p1.get());
    EXPECT_NE((void*)p1.get(), (void*)p3);

    EXPECT_NE((void*)p2.get(), (void*)p3);
}

TEST(WTF_RefPtr, MultiInherenceReverse)
{
    RefPtr<Base> p1 = adoptRef(new Multi());
    auto& p2 = downcast<Multi>(p1);
    EXPECT_EQ((void*)p1.get(), (void*)p2.get());
}

#define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \
SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \
static bool isType(const TestWebKitAPI::Base& base) { return base.type() == TestWebKitAPI::Class; } \
SPECIALIZE_TYPE_TRAITS_END()

SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi)

The current implementation passes all the expectations which demonstrates Chris' comment. With that being said, my take will be making the first two variants return regular pointers and references.

[Darin Adler]

I don’t understand why the test case involves all those casts to (void*).

[Jiewen Tan]

(In reply to Darin Adler from comment #61)
> I don’t understand why the test case involves all those casts to (void*).

As far as I understand, when comparing pointers, implicit static casting will apply to one of the operands to make both operands the same type of pointers. To prevent this implicit static casting, my take is to cast them to void pointers.

[Jiewen Tan]

Created attachment 325544 [details]
Patch

[Jiewen Tan]

Created attachment 325605 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1710:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 109 files


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

[Geoffrey Garen]

Comment on attachment 325605 [details]
Patch

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

The ability to call is<T> without a .get() looks great.

This patch includes lots of uses of WTFMove that I don't think belong. I've noted some but not all of them. I think you should fix that. We should only WTFMove when we intend to transfer ownership, and not when we just want to dereference an object and access one of its data members.

I'm starting to think that it's bad for downcast<T> to implicitly remove the RefPtr lifetime guarantee. In combination with auto, this makes it hard to know at a glance if pointer lifetime is guaranteed. It seems like a foot-gun for 

    RefPtr<Base> base = ...;
    auto derived = downcast<Derived>(base);

to automatically remove the lifetime guarantee originally provided by base. You should have to do something special to remove lifetime guarantees.

I think this means that downcast<T> should return a RefPtr<T> or Ref<T>, just like static_pointer_cast does. It's OK to optimize for the case of move. Then you end up with these options:

(1) When you own the lifetime of base:

auto derived = downcast<Derived>(base.get()); // This is like other uses of .get(), and it's my job to make sure it's OK just like other uses of .get()

(2) When you transfer ownership of base:

return downcast<Derived>(WTFMove(base)); // This returns RefPtr<T> to preserve lifetime without refcount churn

(3) When you're not sure who owns the lifetime of base:

auto derived = downcast<Derived>(base); // This increments the refcount, which is a safe default

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2369
> +        return accessibilityImageMapHitTest(downcast<HTMLAreaElement>(WTFMove(node)).get(), point);

WTFMove() followed by .get() doesn't make sense to me here. I guess this should just be .get() inside the downcast<>()?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2372
> +        node = downcast<HTMLOptionElement>(WTFMove(node))->ownerSelectElement();

I don't think WTFMove() is a great match here. We aren't trying to transfer ownership to anyone. We're just trying to call the ownerSelectElement() getter. We can do that with .get() or downcast<>() without moving anything.

> Source/WebCore/css/StyleSheetContents.cpp:159
> +        m_childRules.appendVector(downcast<StyleRule>(WTFMove(rule))->splitIntoMultipleRulesWithMaximumSelectorComponentCount(RuleData::maximumSelectorComponentCount));

I don't think you want WTFMove here.

> Source/WebCore/css/parser/CSSParser.cpp:298
> +    return downcast<StyleRuleFontFace>(WTFMove(rule))->properties().getPropertyCSSValue(propertyID);

I don't think you want to move here.

> Source/WebCore/dom/ContainerNode.cpp:740
> +            downcast<ContainerNode>(*child).cloneChildNodes(downcast<ContainerNode>(WTFMove(clonedChild)));

I don't think you want move here.

> Source/WebCore/dom/Node.cpp:588
> +        auto text = downcast<Text>(node.copyRef());

It feels very subtle that this line of code preserves a reference to the node. The use of auto means that we don't know the type of 'text' is RefPtr<T>. The use of downcast<T> is ambiguous because some variants of downcast<> return temporaries, whereas the particular variant for rvalue reference returns RefPtr<>. So, to know that this line of code preserves lifetime, you have to read the return type of copyRef() and then select the appropriate downcast<> for that return type, and then look at its return type, and then you know you have a RefPtr<>. That's a lot of code chasing just to understand pointer lifetime. But we want pointer lifetime to be known correct at a glance.

> Source/WebCore/editing/ApplyStyleCommand.cpp:62
> +    return is<CSSPrimitiveValue>(value) ? downcast<CSSPrimitiveValue>(WTFMove(value))->valueID() : 0;

Move doesn't seem right here.

> Source/WebCore/html/FormAssociatedElement.cpp:112
> +            return downcast<HTMLFormElement>(WTFMove(newFormCandidate)).get();

I don't think you want move here.

> Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:566
> +    EXPECT_NE((void*)p1.get(), (void*)p2);

What I think you should do instead of void* casting is this:

EXPECT_EQ(static_cast<Multi*>(p1.get()), p2);

The nice thing about this is that it avoids relying on not-necessarily-obvious details of casting to void*. Also, it's usually better for a test to confirm that you got the one true correct answer, rather than confirming that you did not get one of many possible incorrect answers.

> Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:581
> +    RefPtr<Base> p1 = adoptRef(new MultiReverse());
> +    auto* p2 = downcast<MultiReverse>(p1);
> +    auto* p3 = downcast<MultiReverse>(p1.get());
> +    EXPECT_EQ((void*)p1.get(), (void*)p2);
> +    EXPECT_EQ((void*)p1.get(), (void*)p3);
> +}

Same comment here.

[Jiewen Tan]

Created attachment 325670 [details]
Patch

[Jiewen Tan]

Thanks Geoff for reviewing my patch. Here is the is<T>() only version patch per our discussion.

(In reply to Geoffrey Garen from comment #66)
> Comment on attachment 325605 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325605&action=review
> 
> The ability to call is<T> without a .get() looks great.
> 
> This patch includes lots of uses of WTFMove that I don't think belong. I've
> noted some but not all of them. I think you should fix that. We should only
> WTFMove when we intend to transfer ownership, and not when we just want to
> dereference an object and access one of its data members.
> 
> I'm starting to think that it's bad for downcast<T> to implicitly remove the
> RefPtr lifetime guarantee. In combination with auto, this makes it hard to
> know at a glance if pointer lifetime is guaranteed. It seems like a foot-gun
> for 
> 
>     RefPtr<Base> base = ...;
>     auto derived = downcast<Derived>(base);
> 
> to automatically remove the lifetime guarantee originally provided by base.
> You should have to do something special to remove lifetime guarantees.
> 
> I think this means that downcast<T> should return a RefPtr<T> or Ref<T>,
> just like static_pointer_cast does. It's OK to optimize for the case of
> move. Then you end up with these options:
> 
> (1) When you own the lifetime of base:
> 
> auto derived = downcast<Derived>(base.get()); // This is like other uses of
> .get(), and it's my job to make sure it's OK just like other uses of .get()
> 
> (2) When you transfer ownership of base:
> 
> return downcast<Derived>(WTFMove(base)); // This returns RefPtr<T> to
> preserve lifetime without refcount churn
> 
> (3) When you're not sure who owns the lifetime of base:
> 
> auto derived = downcast<Derived>(base); // This increments the refcount,
> which is a safe default
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2369
> > +        return accessibilityImageMapHitTest(downcast<HTMLAreaElement>(WTFMove(node)).get(), point);
> 
> WTFMove() followed by .get() doesn't make sense to me here. I guess this
> should just be .get() inside the downcast<>()?
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2372
> > +        node = downcast<HTMLOptionElement>(WTFMove(node))->ownerSelectElement();
> 
> I don't think WTFMove() is a great match here. We aren't trying to transfer
> ownership to anyone. We're just trying to call the ownerSelectElement()
> getter. We can do that with .get() or downcast<>() without moving anything.
> 
> > Source/WebCore/css/StyleSheetContents.cpp:159
> > +        m_childRules.appendVector(downcast<StyleRule>(WTFMove(rule))->splitIntoMultipleRulesWithMaximumSelectorComponentCount(RuleData::maximumSelectorComponentCount));
> 
> I don't think you want WTFMove here.
> 
> > Source/WebCore/css/parser/CSSParser.cpp:298
> > +    return downcast<StyleRuleFontFace>(WTFMove(rule))->properties().getPropertyCSSValue(propertyID);
> 
> I don't think you want to move here.
> 
> > Source/WebCore/dom/ContainerNode.cpp:740
> > +            downcast<ContainerNode>(*child).cloneChildNodes(downcast<ContainerNode>(WTFMove(clonedChild)));
> 
> I don't think you want move here.
> 
> > Source/WebCore/dom/Node.cpp:588
> > +        auto text = downcast<Text>(node.copyRef());
> 
> It feels very subtle that this line of code preserves a reference to the
> node. The use of auto means that we don't know the type of 'text' is
> RefPtr<T>. The use of downcast<T> is ambiguous because some variants of
> downcast<> return temporaries, whereas the particular variant for rvalue
> reference returns RefPtr<>. So, to know that this line of code preserves
> lifetime, you have to read the return type of copyRef() and then select the
> appropriate downcast<> for that return type, and then look at its return
> type, and then you know you have a RefPtr<>. That's a lot of code chasing
> just to understand pointer lifetime. But we want pointer lifetime to be
> known correct at a glance.
> 
> > Source/WebCore/editing/ApplyStyleCommand.cpp:62
> > +    return is<CSSPrimitiveValue>(value) ? downcast<CSSPrimitiveValue>(WTFMove(value))->valueID() : 0;
> 
> Move doesn't seem right here.
> 
> > Source/WebCore/html/FormAssociatedElement.cpp:112
> > +            return downcast<HTMLFormElement>(WTFMove(newFormCandidate)).get();
> 
> I don't think you want move here.
> 
> > Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:566
> > +    EXPECT_NE((void*)p1.get(), (void*)p2);
> 
> What I think you should do instead of void* casting is this:
> 
> EXPECT_EQ(static_cast<Multi*>(p1.get()), p2);
> 
> The nice thing about this is that it avoids relying on
> not-necessarily-obvious details of casting to void*. Also, it's usually
> better for a test to confirm that you got the one true correct answer,
> rather than confirming that you did not get one of many possible incorrect
> answers.
> 
> > Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:581
> > +    RefPtr<Base> p1 = adoptRef(new MultiReverse());
> > +    auto* p2 = downcast<MultiReverse>(p1);
> > +    auto* p3 = downcast<MultiReverse>(p1.get());
> > +    EXPECT_EQ((void*)p1.get(), (void*)p2);
> > +    EXPECT_EQ((void*)p1.get(), (void*)p3);
> > +}
> 
> Same comment here.

[Jiewen Tan]

Thanks Ryosuke for r+ my patch.

[WebKit Commit Bot]

Comment on attachment 325670 [details]
Patch

Clearing flags on attachment: 325670

Committed r224320: <https://trac.webkit.org/changeset/224320>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.