Bug 166628 – Remove PassRefPtr use from the "css" directory, related cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Darin Adler]

Remove PassRefPtr use from the "css" directory, related cleanup

[Darin Adler]

Created attachment 297867 [details]
Patch

[Alex Christensen]

Comment on attachment 297867 [details]
Patch

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

> Source/WebCore/css/CSSGradientValue.cpp:98
> +    ASSERT(is<CSSRadialGradientValue>(value));

If we add conical gradients but forget to update this, then this assertion will no longer necessarily be true.  I liked the old design.  It was safer.  If only we could switch(type) or something...

> Source/WebCore/css/CSSImageGeneratorValue.cpp:73
> -    m_clients.add(renderer);
> +    m_clients.add(&renderer);

Can we make m_clients a HashCountedSet<std::reference_wrapper<RenderElement>> ?

> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:103
> +    ASSERT(renderer);

It seems like we should make the parameter type a RenderElement& instead of this.

[Darin Adler]

Comment on attachment 297867 [details]
Patch

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

>> Source/WebCore/css/CSSGradientValue.cpp:98
>> +    ASSERT(is<CSSRadialGradientValue>(value));
> 
> If we add conical gradients but forget to update this, then this assertion will no longer necessarily be true.  I liked the old design.  It was safer.  If only we could switch(type) or something...

A tiny bit safer in theory, but also an untested unreachable code path. I definitely did not like the old design. Maybe we should talk in person?

>> Source/WebCore/css/CSSImageGeneratorValue.cpp:73
>> +    m_clients.add(&renderer);
> 
> Can we make m_clients a HashCountedSet<std::reference_wrapper<RenderElement>> ?

I have no idea; this is a recurring pattern elsewhere so we should try that at some point.

>> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:103
>> +    ASSERT(renderer);
> 
> It seems like we should make the parameter type a RenderElement& instead of this.

Absolutely; we can keep pulling on the thread of the functions that take pointers that should be references. I didn’t want to go even further in my refactoring in this patch, so this is one of the places where I drew the line.

[Darin Adler]

Committed r210215: <http://trac.webkit.org/changeset/210215>