Bug 227807 – SVGImageForContainer reports true for is<SVGImage>() but it doesn't inherit from SVGImage

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

SVGImageForContainer reports true for is<SVGImage>() but it doesn't inherit from SVGImage

[Myles C. Maxfield]

Created attachment 433151 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 433151 [details]
Patch

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

> Source/WebCore/platform/graphics/Image.h:100
> +    virtual bool actsLikeSVGImage() const { return false; }

This can be just

bool actsLikeSVGImage() const { return isSVGImage() || isSVGImageForContainer(); }

[Myles C. Maxfield]

Created attachment 433153 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 433153 [details]
Patch

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

> Source/WebCore/platform/graphics/Image.h:100
> +    bool actsLikeSVGImage() const { return isSVGImage() || isSVGImageForContainer(); }

This name does not seem like what the return value represents. Can't we choose a name which says "is the image to be drawn at the end an SVGImage"? Maybe isUnderlyingSVGImage(), isDrawingSVGImage() or something similar?

> Source/WebCore/platform/graphics/ImageObserver.h:30
> +#include <wtf/URL.h>
> +#include <wtf/text/WTFString.h>

There is no obvious reason for including these headers here especially URL.h includes WTFString.h

> Tools/ChangeLog:10
> +        * TestWebKitAPI/Tests/WebCore/SVGImageCasts.cpp: Added.
> +        (TestWebKitAPI::TEST):

I do not see what mistakes this test will prevent in the future. I can add a new superclass of Image which returns true for isSVGImage() or change an existing Image type to true for isSVGImage() and nothing will be caught.

> Tools/TestWebKitAPI/Tests/WebCore/SVGImageCasts.cpp:35
> +using namespace WebCore;
> +
> +namespace TestWebKitAPI {

I think we usually put the using statement inside the namespace.

[Myles C. Maxfield]

Comment on attachment 433153 [details]
Patch

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

>> Source/WebCore/platform/graphics/Image.h:100
>> +    bool actsLikeSVGImage() const { return isSVGImage() || isSVGImageForContainer(); }
> 
> This name does not seem like what the return value represents. Can't we choose a name which says "is the image to be drawn at the end an SVGImage"? Maybe isUnderlyingSVGImage(), isDrawingSVGImage() or something similar?

Makes sense. I'll do "drawsSVGImage()."

>> Source/WebCore/platform/graphics/ImageObserver.h:30
>> +#include <wtf/text/WTFString.h>
> 
> There is no obvious reason for including these headers here especially URL.h includes WTFString.h

It's needed for the TestWebKitAPI test file which includes <WebCore/ImageObserver.h> as the first thing it includes. This file uses URL.h.

>> Tools/ChangeLog:10
>> +        (TestWebKitAPI::TEST):
> 
> I do not see what mistakes this test will prevent in the future. I can add a new superclass of Image which returns true for isSVGImage() or change an existing Image type to true for isSVGImage() and nothing will be caught.

The test catches what the ChangeLog says:

if (is<SVGImage>(image))
            downcast<SVGImage>(image).doSomething();

It tests this by making sure that is<SVGImage>() returns false for SVGImageForContainers.

[Myles C. Maxfield]

Committed r279793 (239560@main): <https://commits.webkit.org/239560@main>

[Radar WebKit Bug Importer]

<rdar://problem/80390790>