[webkit-reviews] review granted: [Bug 227807] SVGImageForContainer reports true for is<SVGImage>() but it doesn't inherit from SVGImage : [Attachment 433153] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 227807: SVGImageForContainer reports true for is<SVGImage>() but it doesn't
inherit from SVGImage
https://bugs.webkit.org/show_bug.cgi?id=227807

Attachment 433153: Patch

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




--- Comment #4 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 433153
  --> https://bugs.webkit.org/attachment.cgi?id=433153
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.