[webkit-reviews] review denied: [Bug 45519] Add cache for GPU-accelerated path processing results : [Attachment 67470] Revised patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Kenneth Russell
<kbr at google.com>'s request for review:
Bug 45519: Add cache for GPU-accelerated path processing results
https://bugs.webkit.org/show_bug.cgi?id=45519

Attachment 67470: Revised patch
https://bugs.webkit.org/attachment.cgi?id=67470&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
r- for the error handling.  Otherwise, this look all right

View in context:
https://bugs.webkit.org/attachment.cgi?id=67470&action=prettypatch

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:52
> +    // cleared or another vertex is added. Returns 0 if there are no
> +    // vertices in the mesh.
> +    const float* vertices() const
Wouldn't calling this function if there are no vertices be an error from the
caller? This should be an ASSERT() then, not an if check.

Also, why doesn't this return a const Vector<float>*?  Then the caller could
grab the .data() if they want and deal with the empty case in whatever way they
please.

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:63
> +    // cache is cleared or another vertex is added. Returns 0 if
> +    // there are no vertices in the mesh.
> +    const float* texcoords() const
Same here.

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:82
> +    // vertex, which can be drawn as GL_TRIANGLES. Returns 0 if there
> +    // are no interior vertices in the mesh.
> +    const float* interiorVertices() const
Same here

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:96
> +    unsigned numberOfInteriorEdgeVertices() const;
Why isn't this inline like the rest?

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:100
> +    // no interior edge vertices in the mesh.
> +    const float* interiorEdgeVertices() const;
Same comments as above (don't return NULL, consider exposing a const
Vector<float>&).