[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





--- Comment #19 from Tom Hudson <tomhudson at google.com>  2011-08-03 13:28:56 PST ---
Darin,

Thanks for the review. 

(In reply to comment #16)
> 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.

It looks like this is debugging code left over from testing m_isSolidColor. Can it be removed safely and the unit tests relied on? If not, and we decide to keep this function, I can push the #if !ASSERT_DISABLED to other classes that implement it.

> > 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.

The function appears to be being used in two different ways:
 1 - can I cast to a BitmapImage
 2 - does this thing have the semantics of a bitmap

This patch was attempting to remove any uses of case #1 (a previous patch for bug 55821 only addressed a single caller). I'd be happy to add a second function that directly addresses case #2, but it seems to me that #1 is common in WebKit and is difficult to stop people from doing, thus the other approaches we considered that I mentioned in comment #18 above.

> > 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.

notSolidColor() was preexisting on BitmapImage; I agree that the name is not descriptive. isSinglePixelConstantColor() (with the sense reversed)?

Do you have alternate name suggestions for hasTransparency()? mayBeTransparent() / isNotCertainlyOpaque(), perhaps?

> 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.

Inline virtuals are absolutely endemic to these class headers. Should I go through and move the rest into their cpp files? As a new WebKit contributor I was trying to follow established style regardless of whether it conflicted with best practices I'd learned elsewhere.

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