[webkit-reviews] review denied: [Bug 178612] Let is<T>() and downcast<T>() accept RefPtrs/Refs : [Attachment 325605] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 178612: Let is<T>() and downcast<T>() accept RefPtrs/Refs
https://bugs.webkit.org/show_bug.cgi?id=178612

Attachment 325605: Patch

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




--- Comment #66 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 325605
  --> https://bugs.webkit.org/attachment.cgi?id=325605
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))->splitIntoMultiple
RulesWithMaximumSelectorComponentCount(RuleData::maximumSelectorComponentCount)
);

I don't think you want WTFMove here.

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

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.