[webkit-reviews] review denied: [Bug 45519] Add cache for GPU-accelerated path processing results : [Attachment 67156] 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 67156: Patch
https://bugs.webkit.org/attachment.cgi?id=67156&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67156&action=prettypatch

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:43
> +class LoopBlinnPathCache : public Noncopyable {
> +public:
Please declare a destructor here and give it an (empty) definition in the .cpp.
 Otherwise the body of the destructor for vector will get inlined in to every
file that includes this header.

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:61
> +    // Get the base pointer to the vertex information. There are two
> +    // coordinates per vertex. This pointer is valid until the cache is
> +    // cleared or another vertex is added. Returns 0 if there are no
> +    // vertices in the mesh.
> +    const float* vertices() const
> +    {
> +	   if (!numberOfVertices())
> +	       return 0;
> +	   return m_vertices.data();
> +    }
> +
> +    // Get the base pointer to the texture coordinate information. There
> +    // are three coordinates per vertex. This pointer is valid until the
> +    // cache is cleared or another vertex is added. Returns 0 if
> +    // there are no vertices in the mesh.
> +    const float* texcoords() const
Instead of exposing raw pointers to this class' underlying vector types, this
class should expose getters for the length and data at a given offset.	That
way users of this class are insulated from the details of the class' guts. 
That would also avoid oddness around how to do the correct pointer math for
vertices vs texcoords and allow us to change out the underlying implementation
if we wanted to.

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:80
> +    // Base pointer to the interior vertices; two coordinates per
> +    // 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. A better interface would be something like "int
interiorVertexCount(); FloatPoint interiorVertex(int i);"

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:87
> +    void addInteriorVertex(float x, float y);
Why not use FloatPoint here (and all the other locations that use pairs or
triples of floats)?