[Webkit-unassigned] [Bug 46493] [chromium] Add mipmap support for ImageLayerChromium

[bugzilla-daemon at webkit.org]

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


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69182|review?                     |review-
               Flag|                            |




--- Comment #20 from James Robinson <jamesr at chromium.org>  2010-10-01 10:52:33 PST ---
(From update of attachment 69182)
View in context: https://bugs.webkit.org/attachment.cgi?id=69182&action=review

R- for nits, but overall looks fine.  I mostly want to make sure the testing situation is accurately reflected in the ChangeLog.

> WebCore/ChangeLog:8
> +        No change in behaviour, use existing tests.

Comment is not really accurate, this does change behavior.  Could you list some tests that would cover this?

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:270
> +    bool doMipmap = generateMipmap && (layerRenderer()->contentLayerSharedValues()->npotSupported()

Not sure what it means to "do" mipmap. It looks like this boolean is actually recording whether to generate mipmaps, so why isn't it called generateMipmaps?  The parameter to this function could be called something like "requestMipmaps" to indicate that the implementation will not necessarily respect the caller's wishes.

> WebCore/platform/graphics/chromium/ContentLayerChromium.h:83
> +                           const IntRect& updateRect, unsigned textureId, bool generateMipmap = false);

In general, it's better to have a two-state enum for APIs like this so it's clearer at a callsite what true or false mean.

> WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:154
> +    const bool generateMipmap = true;

Why does this local exist?

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