[webkit-reviews] review granted: [Bug 128671] CounterContentData::counter() and ImageContentData::image() should return references : [Attachment 223975] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 128671: CounterContentData::counter() and ImageContentData::image() should
return references
https://bugs.webkit.org/show_bug.cgi?id=128671

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223975&action=review


> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1408
> +	       const CounterContent& counter =
toCounterContentData(contentData)->counter();
> +	      
list.get().append(cssValuePool().createValue(counter.identifier(),
CSSPrimitiveValue::CSS_COUNTER_NAME));

Why not auto&?

Maybe drop the local variable entirely and do this in a single line?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1411
> +	       const StyleImage& image =
toImageContentData(contentData)->image();
> +	       list.get().append(image.cssValue());

Why not auto&?

Maybe drop the local variable entirely and do this in a single line?

> Source/WebCore/css/StyleResolver.cpp:3552
> +			   RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(const_cast<StyleImage*>(&image
)));

This const cast is ugly.

Separate point: Can we add checked casts for StyleImage?

> Source/WebCore/rendering/RenderElement.cpp:135
> +	   const StyleImage& styleImage =
toImageContentData(contentData)->image();

Why not auto&?

> Source/WebCore/rendering/style/ContentData.h:93
> +    const StyleImage& image() const { return *m_image; }

Why did you get rid of the const overload?

>> Source/WebCore/rendering/style/ContentData.h:94
>>	void setImage(PassRefPtr<StyleImage> image) { m_image = image; }
> 
> Is this function ever used in a way that makes m_image null?

Probably should add an assertion here too, as in the constructor and consider
changing to PassRef maybe?

> Source/WebCore/rendering/style/ContentData.h:156
>      }

We should assert m_counter.

> Source/WebCore/rendering/style/ContentData.h:159
>      void setCounter(std::unique_ptr<CounterContent> counter) { m_counter =
std::move(counter); }

We should assert counter (before) or m_counter (after).