[webkit-reviews] review denied: [Bug 46493] [chromium] Add mipmap support for ImageLayerChromium : [Attachment 69182] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 46493: [chromium] Add mipmap support for ImageLayerChromium
https://bugs.webkit.org/show_bug.cgi?id=46493

Attachment 69182: Patch
https://bugs.webkit.org/attachment.cgi?id=69182&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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?