[Webkit-unassigned] [Bug 65063] REGRESSION(r91628): 3 canvas tests crash on Chromium Linux and one test fail on Chromium Mac

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=65063


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #102814|1                           |0
        is obsolete|                            |




--- Comment #16 from Darin Adler <darin at apple.com>  2011-08-03 12:55:53 PST ---
(From update of attachment 102814)
View in context: https://bugs.webkit.org/attachment.cgi?id=102814&action=review

> Source/WebCore/platform/graphics/BitmapImage.h:163
> +    virtual bool hasTransparency() { return currentFrameHasAlpha(); }

It’s rarely helpful to have an inline definition of a function that is also virtual. If this is going to be virtual, I suggest putting the implementation into BitmapImage.cpp rather than putting it here in the header.

> Source/WebCore/platform/graphics/BitmapImage.h:-169
> -#if !ASSERT_DISABLED
> -    bool notSolidColor()
> +    virtual bool notSolidColor()
>      {
>          return size().width() != 1 || size().height() != 1 || frameCount() > 1;
>      }
> -#endif

It’s rarely helpful to have an inline definition of a function that is also virtual. If this is going to be virtual, I suggest putting the implementation into BitmapImage.cpp rather than putting it here in the header.

Why is this now outside ASSERT_DISABLED #if? I ask because the imprecision of this before was partly justified by the fact that it’s for assertions only.

There is no rationale for this change in the change log. In fact, the change log says nothing.

> Source/WebCore/platform/graphics/Image.h:92
> +    // This is not RTTI and may not indicate that the object can
> +    // be safely cast to WebCore::BitmapImage on all ports.
>      virtual bool isBitmapImage() const { return false; }

This comment indicates that we need to rename the function; not add a comment. It’s highly likely someone will use this function wrong and the comment won’t necessarily help.

Many people are not familiar with the RTTI acronym and so it would be better not to use it.

> Source/WebCore/platform/graphics/Image.h:163
> +    virtual bool hasTransparency () { return true; }
> +    virtual bool notSolidColor () { return true; }

I don’t like either of these function names. The reason is that both don’t do what they say. There are many images that have no transparency at all that will still return true for hasTransparency. There are many images that are a solid color that will return true for notSolidColor. Ideally the functions would have precise names rather than imprecise ones. What does it mean if these functions return false? We should name them so their behavior actually matches what they are named, even if that means a longer name.

These are formatted wrong with spaces before braces.

Also, it’s rarely helpful to have an inline definition of a function that is also virtual. If this is going to be virtual, I suggest putting the implementation into Image.cpp rather than putting it here in the header.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.