Added the bulk of the algorithm for GPU accelerated 2D vector curve...

Added the bulk of the algorithm for GPU accelerated 2D vector curve
rendering from "Rendering Vector Art on the GPU" by Loop and Blinn,
GPU Gems 3, Chapter 25.

The main entry point to the algorithm is the PathProcessor, which
takes in a Skia path and converts it to two triangle meshes: one for
the exterior region of the shape containing the curve segments, and
one for the interior region of the shape which is filled with constant
(1.0) alpha.

The o3d.ProcessedPath class is the internal object which exposes the
needed entry points to JavaScript. However, o3djs.gpu2d is the
user-level entry point to the algorithm. This exposes a Path primitive
to which line, quadratic curve and cubic curve segments can be added,
and simple fills (currently only a solid color).

An SVG loader in samples/gpu2d/svgloader.js illustrates how content
might be imported at run time. Several samples and regression tests
demonstrate the current state of the implementation. More work is
planned.

Some small generalizations to the O3D code were necessary to support
two-dimensional vertices.

Note that I plan to submit gpu2d.js and/or svgloader.js for JavaScript
readability. I have run both through the JS compiler and have fixed as
many of the doc generation errors as possible in svgloader.js without
pulling this file into the o3djs namespace.

Tested in O3D on Windows and Mac OS X.

BUG=none
TEST=various SVG based tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40079

[Ken Russell (switch to Gerrit), 2010-02-20 04:23:50]

Please review. Thanks.

[apatrick_chromium, 2010-02-23 18:46:26]

I'm sure I missed a bunch of stuff. This CL is huge! It's cool though.

http://codereview.chromium.org/652016/diff/1/9
File core/cross/gpu2d/cubic_math_utils.cc (right):

http://codereview.chromium.org/652016/diff/1/9#newcode74
core/cross/gpu2d/cubic_math_utils.cc:74: if (e >= 0 && e <= f) {
Would this work?
return e >= 0 && e <= f;

http://codereview.chromium.org/652016/diff/1/9#newcode78
core/cross/gpu2d/cubic_math_utils.cc:78: if (e<= 0 && e >= f) {
whitespace typo

http://codereview.chromium.org/652016/diff/1/10
File core/cross/gpu2d/path_cache.cc (right):

http://codereview.chromium.org/652016/diff/1/10#newcode38
core/cross/gpu2d/path_cache.cc:38: return vertices_.size() / 2;
Division goes away if vertices_ is an array of points.

http://codereview.chromium.org/652016/diff/1/10#newcode42
core/cross/gpu2d/path_cache.cc:42: return &vertices_.front();
Does this assert if there are no vertices? Is that what you want? I suspect it
should return null.

http://codereview.chromium.org/652016/diff/1/10#newcode46
core/cross/gpu2d/path_cache.cc:46: return &texcoords_.front();
Does this assert if there are no texture coordinates?

http://codereview.chromium.org/652016/diff/1/10#newcode51
core/cross/gpu2d/path_cache.cc:51: vertices_.push_back(x);
There are potentially more reallocations here than necessary.

http://codereview.chromium.org/652016/diff/1/10#newcode68
core/cross/gpu2d/path_cache.cc:68: return interior_vertices_.size() / 2;
assert?

http://codereview.chromium.org/652016/diff/1/10#newcode72
core/cross/gpu2d/path_cache.cc:72: return &interior_vertices_.front();
assert?

http://codereview.chromium.org/652016/diff/1/10#newcode76
core/cross/gpu2d/path_cache.cc:76: interior_vertices_.push_back(x);
See above.

http://codereview.chromium.org/652016/diff/1/10#newcode86
core/cross/gpu2d/path_cache.cc:86: return &interior_edge_vertices_.front();
might assert?

http://codereview.chromium.org/652016/diff/1/10#newcode90
core/cross/gpu2d/path_cache.cc:90: interior_edge_vertices_.push_back(x);
Reallocations

http://codereview.chromium.org/652016/diff/1/12
File core/cross/gpu2d/path_cache.h (right):

http://codereview.chromium.org/652016/diff/1/12#newcode56
core/cross/gpu2d/path_cache.h:56: unsigned int num_vertices();
const method?

http://codereview.chromium.org/652016/diff/1/12#newcode61
core/cross/gpu2d/path_cache.h:61: float* vertices();
const method?

http://codereview.chromium.org/652016/diff/1/12#newcode66
core/cross/gpu2d/path_cache.h:66: float* texcoords();
const method?

http://codereview.chromium.org/652016/diff/1/12#newcode76
core/cross/gpu2d/path_cache.h:76: unsigned int num_interior_vertices();
const method?

http://codereview.chromium.org/652016/diff/1/12#newcode79
core/cross/gpu2d/path_cache.h:79: float* interior_vertices();
const method?

http://codereview.chromium.org/652016/diff/1/12#newcode88
core/cross/gpu2d/path_cache.h:88: unsigned int num_interior_edge_vertices();
const method?

http://codereview.chromium.org/652016/diff/1/12#newcode91
core/cross/gpu2d/path_cache.h:91: float* interior_edge_vertices();
const method?

http://codereview.chromium.org/652016/diff/1/12#newcode97
core/cross/gpu2d/path_cache.h:97: std::vector<float> vertices_;
I know there is no Vector2 but this looks like it should be
std::vector<Vector2>.

http://codereview.chromium.org/652016/diff/1/12#newcode100
core/cross/gpu2d/path_cache.h:100: std::vector<float> texcoords_;
std::vector<Vector3>?

http://codereview.chromium.org/652016/diff/1/7
File core/cross/gpu2d/path_processor.cc (right):

http://codereview.chromium.org/652016/diff/1/7#newcode59
core/cross/gpu2d/path_processor.cc:59: const T& min2(const T& v1, const T& v2) {
Using const T& here will work but it is a little dangerous. How about just T.
The C++ optimizer is allowed to eliminate the redundant copy. And below.

http://codereview.chromium.org/652016/diff/1/7#newcode114
core/cross/gpu2d/path_processor.cc:114: void Setup(LocalTriangulator::Triangle*
triangle) {
const LocalTriangulator::Triangle& ?
You might need to make get_vertex a const method too.

http://codereview.chromium.org/652016/diff/1/7#newcode211
core/cross/gpu2d/path_processor.cc:211: Segment* next() { return next_; }
const method. more below.

http://codereview.chromium.org/652016/diff/1/7#newcode263
core/cross/gpu2d/path_processor.cc:263: const BBox& bbox() {
const method

http://codereview.chromium.org/652016/diff/1/7#newcode285
core/cross/gpu2d/path_processor.cc:285: int num_triangles() {
const method. more below.

http://codereview.chromium.org/652016/diff/1/7#newcode447
core/cross/gpu2d/path_processor.cc:447: Segment* begin() {
const method. more below.

http://codereview.chromium.org/652016/diff/1/7#newcode794
core/cross/gpu2d/path_processor.cc:794: if (query_seg->contour() == cur) {
if (query_seg->contour() != cur) {
  num_crossings += query_seg->NumCrossingsForXRay(seg->get_point(0));
}

and below.

http://codereview.chromium.org/652016/diff/1/7#newcode845
core/cross/gpu2d/path_processor.cc:845: while (seg != contour->end()) {
This looks like a for loop.

http://codereview.chromium.org/652016/diff/1/7#newcode910
core/cross/gpu2d/path_processor.cc:910: float x() { return x_; }
const method. and below.

http://codereview.chromium.org/652016/diff/1/7#newcode912
core/cross/gpu2d/path_processor.cc:912: SweepInterval& interval() { return
interval_; }
I don't think we're supposed to use non-const references.

http://codereview.chromium.org/652016/diff/1/7#newcode1110
core/cross/gpu2d/path_processor.cc:1110: while (++cur_iter <= kMaxIter) {
This looks like a for loop.

http://codereview.chromium.org/652016/diff/1/7#newcode1133
core/cross/gpu2d/path_processor.cc:1133: seg->set_marked_for_subdivision(true);
This is a lot of nesting. Could you extract at least one function?

http://codereview.chromium.org/652016/diff/1/6
File core/cross/primitive.cc (right):

http://codereview.chromium.org/652016/diff/1/6#newcode228
core/cross/primitive.cc:228: // Specialization which pads out the fetched
coordinates with zeros,
will leave this to gman.

http://codereview.chromium.org/652016/diff/1/4
File core/cross/processed_path.cc (right):

http://codereview.chromium.org/652016/diff/1/4#newcode94
core/cross/processed_path.cc:94: {
I suggest splitting these two blocks out in separate private functions.

http://codereview.chromium.org/652016/diff/1/5
File core/cross/processed_path.h (right):

http://codereview.chromium.org/652016/diff/1/5#newcode67
core/cross/processed_path.h:67: void QuadTo(float cx, float cy, float x, float
y);
My first thought was that this meant quadrilateral. Maybe expand to Quadratic?

http://codereview.chromium.org/652016/diff/1/5#newcode87
core/cross/processed_path.h:87: Primitive* exterior_triangles();
const method. and below.

http://codereview.chromium.org/652016/diff/1/36
File samples/gpu2d/Google_Logo.svg (right):

http://codereview.chromium.org/652016/diff/1/36#newcode1
samples/gpu2d/Google_Logo.svg:1: <?xml version="1.0" encoding="utf-8"?>
Can we use this file in a sample?

http://codereview.chromium.org/652016/diff/1/32
File samples/gpu2d/basic.html (right):

http://codereview.chromium.org/652016/diff/1/32#newcode42
samples/gpu2d/basic.html:42: O3D Canvas
Is this the right title?

http://codereview.chromium.org/652016/diff/1/32#newcode99
samples/gpu2d/basic.html:99: // o3djs.event.addEventListener(g_o3dElement,
'mousemove', drawCursor);
Delete or uncomment?

http://codereview.chromium.org/652016/diff/1/20
File samples/gpu2d/javalogo.svg (right):

http://codereview.chromium.org/652016/diff/1/20#newcode1
samples/gpu2d/javalogo.svg:1: <?xml version="1.0"?>
Where did this come from?

http://codereview.chromium.org/652016/diff/1/29
File samples/gpu2d/svgloader.js (right):

http://codereview.chromium.org/652016/diff/1/29#newcode168
samples/gpu2d/svgloader.js:168: var parser = new SAXDriver();
Is this the right way to do this in IE?

http://codereview.chromium.org/652016/diff/1/29#newcode278
samples/gpu2d/svgloader.js:278: if (cx || cy) {
If cx and cy are optional they should be called opt_cx and opt_cy.

http://codereview.chromium.org/652016/diff/1/29#newcode365
samples/gpu2d/svgloader.js:365: if (val.charAt(0) == '#') {
it might be saver to use a regular expression here. that would also validate the
syntax of the value.

http://codereview.chromium.org/652016/diff/1/29#newcode375
samples/gpu2d/svgloader.js:375: } else if (val.substr(0, 4) == 'rgb(' &&
regex

http://codereview.chromium.org/652016/diff/1/29#newcode454
samples/gpu2d/svgloader.js:454: if (tok.kind == BaseDataParser_.TokenTypes.WORD)
{
this looks like a switch

http://codereview.chromium.org/652016/diff/1/29#newcode484
samples/gpu2d/svgloader.js:484: return (c == ',' ||
comma is not usually whitespace. does this function have an appropriate name?

http://codereview.chromium.org/652016/diff/1/29#newcode492
samples/gpu2d/svgloader.js:492: * Pushes back a token.
i know you only need one token lookahead but "pushing" onto a stack that can
only hold one token seems a little unusual. how about setLookahead_?

http://codereview.chromium.org/652016/diff/1/29#newcode508
samples/gpu2d/svgloader.js:508: BaseDataParser_.prototype.nextToken_ =
function(parseState, data) {
could you use regular expressions to simplify the lexer?

http://codereview.chromium.org/652016/diff/1/29#newcode523
samples/gpu2d/svgloader.js:523: done = true;
i think this might be clearer using breaks.

http://codereview.chromium.org/652016/diff/1/29#newcode624
samples/gpu2d/svgloader.js:624: while (numFloats-- > 0) {
this looks like a for loop.

http://codereview.chromium.org/652016/diff/1/29#newcode697
samples/gpu2d/svgloader.js:697: this.makeAbsolute_(coords);
why call makeAbsolute_ when absolute is false? and below. Okay it makes sense to
me now. i was confused for several seconds though.

http://codereview.chromium.org/652016/diff/1/29#newcode838
samples/gpu2d/svgloader.js:838: if (word == 'matrix') {
this looks like a switch

http://codereview.chromium.org/652016/diff/1/29#newcode912
samples/gpu2d/svgloader.js:912: * @param {Object} atts the XMLJS attributes
object.
attribs or attributes. please :) also is Object the right type?

http://codereview.chromium.org/652016/diff/1/38
File samples/gpu2d/svgsample.js (right):

http://codereview.chromium.org/652016/diff/1/38#newcode207
samples/gpu2d/svgsample.js:207: g_math.degToRad(15),  // fieldOfViewAngle
that's a narrow field of view!

http://codereview.chromium.org/652016/diff/1/18
File samples/o3djs/gpu2d.js (right):

http://codereview.chromium.org/652016/diff/1/18#newcode444
samples/o3djs/gpu2d.js:444: '  float3 klm : TEXCOORD0;\n' +
what does klm mean?

http://codereview.chromium.org/652016/diff/1/16
File samples/third_party/xmljs/COPYING (right):

http://codereview.chromium.org/652016/diff/1/16#newcode1
samples/third_party/xmljs/COPYING:1: GNU LIBRARY GENERAL PUBLIC LICENSE
we can use this?

[greggman, 2010-02-24 02:50:03]

Very cool.

I stopped reviewing at the end of the C++ stuff because some of my suggestions
might change the HTML stuff.

http://codereview.chromium.org/652016/diff/1/12
File core/cross/gpu2d/path_cache.h (right):

http://codereview.chromium.org/652016/diff/1/12#newcode61
core/cross/gpu2d/path_cache.h:61: float* vertices();
like all said but should be 

const float* vertices() const;
?

http://codereview.chromium.org/652016/diff/1/12#newcode66
core/cross/gpu2d/path_cache.h:66: float* texcoords();
const float* texcoords() const; ?

http://codereview.chromium.org/652016/diff/1/12#newcode79
core/cross/gpu2d/path_cache.h:79: float* interior_vertices();
const float* interior_vertices() const;?

http://codereview.chromium.org/652016/diff/1/12#newcode88
core/cross/gpu2d/path_cache.h:88: unsigned int num_interior_edge_vertices();
why not just return

const std::vector<float>& interior_vertices() const {
  return interior_vertices_;
}

What would save the need for "num_interior_edge_vertices" and similar functions.
It would also mean other algorithms would work on this data.

You can wrap that with a typedef if you want it to be changable later

typedef std::vector<float> InteriorFloats;

...

const InteriorFloats& interior_vertices() const {
  return interior_vertices_;
}

...

InteriorFloats& interior_vertices_;

http://codereview.chromium.org/652016/diff/1/12#newcode91
core/cross/gpu2d/path_cache.h:91: float* interior_edge_vertices();
const float* interior_edge_vertices() const;?

http://codereview.chromium.org/652016/diff/1/7
File core/cross/gpu2d/path_processor.cc (right):

http://codereview.chromium.org/652016/diff/1/7#newcode59
core/cross/gpu2d/path_processor.cc:59: const T& min2(const T& v1, const T& v2) {
why not just use std::min, std::max?

http://codereview.chromium.org/652016/diff/1/7#newcode200
core/cross/gpu2d/path_processor.cc:200: Kind kind() {
const function?

http://codereview.chromium.org/652016/diff/1/7#newcode258
core/cross/gpu2d/path_processor.cc:258: Segment* Subdivide() {
Overloaded functions are discouraged. Is this really needed?

http://codereview.chromium.org/652016/diff/1/7#newcode269
core/cross/gpu2d/path_processor.cc:269: int NumCrossingsForXRay(SkPoint pt) {
can this be const?

http://codereview.chromium.org/652016/diff/1/7#newcode299
core/cross/gpu2d/path_processor.cc:299: int num_interior_vertices() {
const function?

http://codereview.chromium.org/652016/diff/1/7#newcode312
core/cross/gpu2d/path_processor.cc:312: SkPoint get_interior_vertex(int index) {
a const function?

http://codereview.chromium.org/652016/diff/1/7#newcode482
core/cross/gpu2d/path_processor.cc:482: Segment* first_;
I'm a little confused. Why all the effort to make your own instrusive linked
list instead of just using an std::list<Segment*>?

You can also use base/linked_list if you want an intrusive list

And you can use std compatible intrusives as well.

http://codereview.chromium.org/652016/diff/1/7#newcode628
core/cross/gpu2d/path_processor.cc:628: contours_.clear();
What happens to the contours that were in this vector?

http://codereview.chromium.org/652016/diff/1/8
File core/cross/gpu2d/path_processor.h (right):

http://codereview.chromium.org/652016/diff/1/8#newcode66
core/cross/gpu2d/path_processor.h:66: virtual ~PathProcessor();
Does this need to be virtual?

http://codereview.chromium.org/652016/diff/1/6
File core/cross/primitive.cc (right):

http://codereview.chromium.org/652016/diff/1/6#newcode230
core/cross/primitive.cc:230: class FieldReadAccessorPoint3 : public
FieldReadAccessor<Point3> {
This is fine but I'm uncomfortable with the idea that normal 3 component fields
have a performance penalty just so 2 component fields work.

Can you change the code so it uses the old class for 3 component fields and the
new class for other? Or does that make any sense?

http://codereview.chromium.org/652016/diff/1/5
File core/cross/processed_path.h (right):

http://codereview.chromium.org/652016/diff/1/5#newcode95
core/cross/processed_path.h:95: Primitive* interior_triangles();
It doesn't seem like this code should have any knowledge of Primitives. It
should take a Buffers or Fields or create a Buffer or Fields. Everything else
should be left to JavaScript.  All it's doing is filling out arrays of floats
and indices. Those operations are separate from whether or not they are used in
a Primitive or something else (like a Skin) and how they are used, (as
POSITIONS, as TEXCOORDs), it's up to the person writing the shader how the want
to use the data.

http://codereview.chromium.org/652016/diff/1/13
File plugin/idl/processed_path.idl (right):

http://codereview.chromium.org/652016/diff/1/13#newcode109
plugin/idl/processed_path.idl:109: void Update();
You could change this API so you put the curve values in then call

void CreateMesh(Field* position, Field* texcoords);
or
void CreateMesh(Buffer* buffer);
or
Buffer* CreateMesh(Pack* pack);

[greggman, 2010-02-24 22:04:07]

http://codereview.chromium.org/652016/diff/1/13
File plugin/idl/processed_path.idl (right):

http://codereview.chromium.org/652016/diff/1/13#newcode109
plugin/idl/processed_path.idl:109: void Update();
On 2010/02/24 02:50:03, greggman wrote:
> You could change this API so you put the curve values in then call
> 
> void CreateMesh(Field* position, Field* texcoords);
> or
> void CreateMesh(Buffer* buffer);
> or
> Buffer* CreateMesh(Pack* pack);
> 

I thought about this some more and I think this:

bool CreateMesh(Field* position, Field* texcoords);

might be the best API.

You pass in 2 fields. The code returns true/false if failure (if there is such a
thing)

Inside the code you can do

bool CreateMesh(Field* position, Field* texcoords);
  Field* fields[] = {position, texcoords}

  for (int ii = 0; ii < 2; ++ii) {
    Buffer* buffer = fields[ii]->buffer();
    if (!buffer) 
       // fail
    else
      buffer->AllocElements(num_elements_needed_for_mesh);
  }

  // fill out mesh.
}

What do you think?

[Ken Russell (switch to Gerrit), 2010-02-25 06:01:11]

http://codereview.chromium.org/652016/diff/1/9
File core/cross/gpu2d/cubic_math_utils.cc (right):

http://codereview.chromium.org/652016/diff/1/9#newcode74
core/cross/gpu2d/cubic_math_utils.cc:74: if (e >= 0 && e <= f) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Would this work?
> return e >= 0 && e <= f;

Yes. I was only trying to capture all of the failure cases in the fall-through
path. Done.

http://codereview.chromium.org/652016/diff/1/9#newcode78
core/cross/gpu2d/cubic_math_utils.cc:78: if (e<= 0 && e >= f) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> whitespace typo

Done.

http://codereview.chromium.org/652016/diff/1/10
File core/cross/gpu2d/path_cache.cc (right):

http://codereview.chromium.org/652016/diff/1/10#newcode38
core/cross/gpu2d/path_cache.cc:38: return vertices_.size() / 2;
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Division goes away if vertices_ is an array of points.

See above for why I want to leave this as floats.

http://codereview.chromium.org/652016/diff/1/10#newcode42
core/cross/gpu2d/path_cache.cc:42: return &vertices_.front();
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Does this assert if there are no vertices? Is that what you want? I suspect it
> should return null.

In the first version of this code it didn't matter, since this was only called
if num_vertices() > 0, but now it's called in all cases. I've updated the docs
and added a check to return NULL in this case.

http://codereview.chromium.org/652016/diff/1/10#newcode46
core/cross/gpu2d/path_cache.cc:46: return &texcoords_.front();
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Does this assert if there are no texture coordinates?

Fixed per above.

http://codereview.chromium.org/652016/diff/1/10#newcode51
core/cross/gpu2d/path_cache.cc:51: vertices_.push_back(x);
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> There are potentially more reallocations here than necessary.

I don't think it will be significant. During reallocation, vectors expand by
factors of two, not a constant numer of elements.

http://codereview.chromium.org/652016/diff/1/10#newcode68
core/cross/gpu2d/path_cache.cc:68: return interior_vertices_.size() / 2;
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> assert?

Fixed below.

http://codereview.chromium.org/652016/diff/1/10#newcode72
core/cross/gpu2d/path_cache.cc:72: return &interior_vertices_.front();
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> assert?

Fixed.

http://codereview.chromium.org/652016/diff/1/10#newcode86
core/cross/gpu2d/path_cache.cc:86: return &interior_edge_vertices_.front();
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> might assert?

Fixed.

http://codereview.chromium.org/652016/diff/1/12
File core/cross/gpu2d/path_cache.h (right):

http://codereview.chromium.org/652016/diff/1/12#newcode56
core/cross/gpu2d/path_cache.h:56: unsigned int num_vertices();
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const method?

Done.

http://codereview.chromium.org/652016/diff/1/12#newcode61
core/cross/gpu2d/path_cache.h:61: float* vertices();
On 2010/02/24 02:50:03, greggman wrote:
> like all said but should be 
> 
> const float* vertices() const;
> ?

Done.

http://codereview.chromium.org/652016/diff/1/12#newcode66
core/cross/gpu2d/path_cache.h:66: float* texcoords();
On 2010/02/24 02:50:03, greggman wrote:
> const float* texcoords() const; ?

Done.

http://codereview.chromium.org/652016/diff/1/12#newcode76
core/cross/gpu2d/path_cache.h:76: unsigned int num_interior_vertices();
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const method?

Done.

http://codereview.chromium.org/652016/diff/1/12#newcode79
core/cross/gpu2d/path_cache.h:79: float* interior_vertices();
On 2010/02/24 02:50:03, greggman wrote:
> const float* interior_vertices() const;?

Done.

http://codereview.chromium.org/652016/diff/1/12#newcode88
core/cross/gpu2d/path_cache.h:88: unsigned int num_interior_edge_vertices();
On 2010/02/24 02:50:03, greggman wrote:
> why not just return
> 
> const std::vector<float>& interior_vertices() const {
>   return interior_vertices_;
> }
> 
> What would save the need for "num_interior_edge_vertices" and similar
functions.
> It would also mean other algorithms would work on this data.
> 
> You can wrap that with a typedef if you want it to be changable later
> 
> typedef std::vector<float> InteriorFloats;
> 
> ...
> 
> const InteriorFloats& interior_vertices() const {
>   return interior_vertices_;
> }
> 
> ...
> 
> InteriorFloats& interior_vertices_;

I strongly want to avoid referencing higher-level data types in the accessors in
this class, to reduce the number of dependencies the outside world has on the
internal representations used by the algorithm.

http://codereview.chromium.org/652016/diff/1/12#newcode91
core/cross/gpu2d/path_cache.h:91: float* interior_edge_vertices();
On 2010/02/24 02:50:03, greggman wrote:
> const float* interior_edge_vertices() const;?

Done.

http://codereview.chromium.org/652016/diff/1/12#newcode97
core/cross/gpu2d/path_cache.h:97: std::vector<float> vertices_;
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> I know there is no Vector2 but this looks like it should be
> std::vector<Vector2>.

Yes, but see above regarding the desire to not reference any higher-level data
types in this class. This would also cause aliasing problems if we want to treat
the data as a float*.

http://codereview.chromium.org/652016/diff/1/12#newcode100
core/cross/gpu2d/path_cache.h:100: std::vector<float> texcoords_;
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> std::vector<Vector3>?

See above. Prefer to leave this as is.

http://codereview.chromium.org/652016/diff/1/7
File core/cross/gpu2d/path_processor.cc (right):

http://codereview.chromium.org/652016/diff/1/7#newcode59
core/cross/gpu2d/path_processor.cc:59: const T& min2(const T& v1, const T& v2) {
On 2010/02/24 02:50:03, greggman wrote:
> why not just use std::min, std::max?

No good reason. Changed.

http://codereview.chromium.org/652016/diff/1/7#newcode114
core/cross/gpu2d/path_processor.cc:114: void Setup(LocalTriangulator::Triangle*
triangle) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const LocalTriangulator::Triangle& ?
> You might need to make get_vertex a const method too.

This doesn't work with the LocalTriangulator API. Triangle::get_vertex()
deliberately returns a mutable Vertex.

http://codereview.chromium.org/652016/diff/1/7#newcode200
core/cross/gpu2d/path_processor.cc:200: Kind kind() {
On 2010/02/24 02:50:03, greggman wrote:
> const function?

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode211
core/cross/gpu2d/path_processor.cc:211: Segment* next() { return next_; }
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const method. more below.

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode258
core/cross/gpu2d/path_processor.cc:258: Segment* Subdivide() {
On 2010/02/24 02:50:03, greggman wrote:
> Overloaded functions are discouraged. Is this really needed?

Yes, both versions are needed (or will be -- just added a TODO). I prefer to use
the same name for this to make it easier to search for all places in the code
which perform subdivision.

http://codereview.chromium.org/652016/diff/1/7#newcode263
core/cross/gpu2d/path_processor.cc:263: const BBox& bbox() {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const method

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode269
core/cross/gpu2d/path_processor.cc:269: int NumCrossingsForXRay(SkPoint pt) {
On 2010/02/24 02:50:03, greggman wrote:
> can this be const?

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode285
core/cross/gpu2d/path_processor.cc:285: int num_triangles() {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const method. more below.

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode299
core/cross/gpu2d/path_processor.cc:299: int num_interior_vertices() {
On 2010/02/24 02:50:03, greggman wrote:
> const function?

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode312
core/cross/gpu2d/path_processor.cc:312: SkPoint get_interior_vertex(int index) {
On 2010/02/24 02:50:03, greggman wrote:
> a const function?

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode447
core/cross/gpu2d/path_processor.cc:447: Segment* begin() {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const method. more below.

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode482
core/cross/gpu2d/path_processor.cc:482: Segment* first_;
On 2010/02/24 02:50:03, greggman wrote:
> I'm a little confused. Why all the effort to make your own instrusive linked
> list instead of just using an std::list<Segment*>?
> 
> You can also use base/linked_list if you want an intrusive list
> 
> And you can use std compatible intrusives as well.

I want specific behavior of removal of a Segment from the Contour's list when it
is subdivided. std::list would be too inefficient. base/linked_list would work
but from looking at it it adds syntactic cruft during iteration. I would like to
leave this as is.

http://codereview.chromium.org/652016/diff/1/7#newcode628
core/cross/gpu2d/path_processor.cc:628: contours_.clear();
On 2010/02/24 02:50:03, greggman wrote:
> What happens to the contours that were in this vector?

They were allocated out of the Arena and will be freed in bulk when the Arena
is.

http://codereview.chromium.org/652016/diff/1/7#newcode794
core/cross/gpu2d/path_processor.cc:794: if (query_seg->contour() == cur) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> if (query_seg->contour() != cur) {
>   num_crossings += query_seg->NumCrossingsForXRay(seg->get_point(0));
> }
> 
> and below.

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode845
core/cross/gpu2d/path_processor.cc:845: while (seg != contour->end()) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> This looks like a for loop.

True. Changed.

http://codereview.chromium.org/652016/diff/1/7#newcode910
core/cross/gpu2d/path_processor.cc:910: float x() { return x_; }
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const method. and below.

Done.

http://codereview.chromium.org/652016/diff/1/7#newcode912
core/cross/gpu2d/path_processor.cc:912: SweepInterval& interval() { return
interval_; }
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> I don't think we're supposed to use non-const references.

You're right. It turns out a const reference works here. Changed.

http://codereview.chromium.org/652016/diff/1/7#newcode1110
core/cross/gpu2d/path_processor.cc:1110: while (++cur_iter <= kMaxIter) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> This looks like a for loop.

Changed.

http://codereview.chromium.org/652016/diff/1/7#newcode1133
core/cross/gpu2d/path_processor.cc:1133: seg->set_marked_for_subdivision(true);
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> This is a lot of nesting. Could you extract at least one function?

Factored out a "ConditionallySubdivide(Segment*, std::vector<Segment*>*)".

http://codereview.chromium.org/652016/diff/1/8
File core/cross/gpu2d/path_processor.h (right):

http://codereview.chromium.org/652016/diff/1/8#newcode66
core/cross/gpu2d/path_processor.h:66: virtual ~PathProcessor();
On 2010/02/24 02:50:03, greggman wrote:
> Does this need to be virtual?

Not currently. Changed.

http://codereview.chromium.org/652016/diff/1/6
File core/cross/primitive.cc (right):

http://codereview.chromium.org/652016/diff/1/6#newcode230
core/cross/primitive.cc:230: class FieldReadAccessorPoint3 : public
FieldReadAccessor<Point3> {
On 2010/02/24 02:50:03, greggman wrote:
> This is fine but I'm uncomfortable with the idea that normal 3 component
fields
> have a performance penalty just so 2 component fields work.
> 
> Can you change the code so it uses the old class for 3 component fields and
the
> new class for other? Or does that make any sense?

OK, per our discussion I've changed GetVerticesAccessor to allocate and return a
FieldReadAccessor<Point3>* and to choose the correct subclass. I had to make
FieldReadAccessor::Initialize() virtual for this to work.

http://codereview.chromium.org/652016/diff/1/4
File core/cross/processed_path.cc (right):

http://codereview.chromium.org/652016/diff/1/4#newcode94
core/cross/processed_path.cc:94: {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> I suggest splitting these two blocks out in separate private functions.

This has been rewritten based on Gregg's suggestion and there is now a function
sharing code between the two blocks.

http://codereview.chromium.org/652016/diff/1/5
File core/cross/processed_path.h (right):

http://codereview.chromium.org/652016/diff/1/5#newcode67
core/cross/processed_path.h:67: void QuadTo(float cx, float cy, float x, float
y);
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> My first thought was that this meant quadrilateral. Maybe expand to Quadratic?

OK, done here and throughout the code, all the way up to JavaScript.

http://codereview.chromium.org/652016/diff/1/5#newcode87
core/cross/processed_path.h:87: Primitive* exterior_triangles();
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> const method. and below.

Done.

http://codereview.chromium.org/652016/diff/1/13
File plugin/idl/processed_path.idl (right):

http://codereview.chromium.org/652016/diff/1/13#newcode109
plugin/idl/processed_path.idl:109: void Update();
On 2010/02/24 22:04:07, greggman wrote:
> On 2010/02/24 02:50:03, greggman wrote:
> > You could change this API so you put the curve values in then call
> > 
> > void CreateMesh(Field* position, Field* texcoords);
> > or
> > void CreateMesh(Buffer* buffer);
> > or
> > Buffer* CreateMesh(Pack* pack);
> > 
> 
> I thought about this some more and I think this:
> 
> bool CreateMesh(Field* position, Field* texcoords);
> 
> might be the best API.
> 
> You pass in 2 fields. The code returns true/false if failure (if there is such
a
> thing)
> 
> Inside the code you can do
> 
> bool CreateMesh(Field* position, Field* texcoords);
>   Field* fields[] = {position, texcoords}
> 
>   for (int ii = 0; ii < 2; ++ii) {
>     Buffer* buffer = fields[ii]->buffer();
>     if (!buffer) 
>        // fail
>     else
>       buffer->AllocElements(num_elements_needed_for_mesh);
>   }
> 
>   // fill out mesh.
> }
> 
> What do you think?

Good suggestion. I've removed Update() and the exterior and interior triangle
fields, added CreateMesh(Field*, Field*, Field*), and reorganized the calling
JavaScript code in gpu2d.js. The code is simpler and better separates the
concerns.

http://codereview.chromium.org/652016/diff/1/36
File samples/gpu2d/Google_Logo.svg (right):

http://codereview.chromium.org/652016/diff/1/36#newcode1
samples/gpu2d/Google_Logo.svg:1: <?xml version="1.0" encoding="utf-8"?>
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Can we use this file in a sample?

I picked it up off WikiMedia (
http://commons.wikimedia.org/wiki/File:Google_Logo.svg ); since it's us I didn't
think there would be an issue.

http://codereview.chromium.org/652016/diff/1/32
File samples/gpu2d/basic.html (right):

http://codereview.chromium.org/652016/diff/1/32#newcode42
samples/gpu2d/basic.html:42: O3D Canvas
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Is this the right title?

Fixed.

http://codereview.chromium.org/652016/diff/1/32#newcode99
samples/gpu2d/basic.html:99: // o3djs.event.addEventListener(g_o3dElement,
'mousemove', drawCursor);
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Delete or uncomment?

Actually this and the onMouseUp below were bogus. Deleted.

http://codereview.chromium.org/652016/diff/1/20
File samples/gpu2d/javalogo.svg (right):

http://codereview.chromium.org/652016/diff/1/20#newcode1
samples/gpu2d/javalogo.svg:1: <?xml version="1.0"?>
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Where did this come from?

WikiMedia. http://en.wikipedia.org/wiki/File:Java_logo.svg

I know it's copyrighted but it's a useful test case for the curve subdivision
and orientation determination algorithms and I don't think anyone will complain.
If they do we can replace it.

http://codereview.chromium.org/652016/diff/1/29
File samples/gpu2d/svgloader.js (right):

http://codereview.chromium.org/652016/diff/1/29#newcode168
samples/gpu2d/svgloader.js:168: var parser = new SAXDriver();
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> Is this the right way to do this in IE?

This comes from the XMLJS library; it isn't using built-in browser
functionality.

http://codereview.chromium.org/652016/diff/1/29#newcode278
samples/gpu2d/svgloader.js:278: if (cx || cy) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> If cx and cy are optional they should be called opt_cx and opt_cy.

They aren't optional, but this code should be skipped unless either is non-zero.

http://codereview.chromium.org/652016/diff/1/29#newcode365
samples/gpu2d/svgloader.js:365: if (val.charAt(0) == '#') {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> it might be saver to use a regular expression here. that would also validate
the
> syntax of the value.

I deliberately tried to avoid the use of regexps in this code because I want it
to exercise character fetching and substring operations -- I care about those
going fast and am thinking about turning this into a benchmark. When it becomes
more full featured I'll look into switching to regexps (but will probably still
use character fetches to determine which regexp to evaluate rather than doing
them in a chain).

http://codereview.chromium.org/652016/diff/1/29#newcode375
samples/gpu2d/svgloader.js:375: } else if (val.substr(0, 4) == 'rgb(' &&
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> regex

See above.

http://codereview.chromium.org/652016/diff/1/29#newcode454
samples/gpu2d/svgloader.js:454: if (tok.kind == BaseDataParser_.TokenTypes.WORD)
{
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> this looks like a switch

OK. Changed as well as the termination condition.

http://codereview.chromium.org/652016/diff/1/29#newcode484
samples/gpu2d/svgloader.js:484: return (c == ',' ||
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> comma is not usually whitespace. does this function have an appropriate name?

Renamed to isWhitespaceOrSeparator_.

http://codereview.chromium.org/652016/diff/1/29#newcode492
samples/gpu2d/svgloader.js:492: * Pushes back a token.
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> i know you only need one token lookahead but "pushing" onto a stack that can
> only hold one token seems a little unusual. how about setLookahead_?

It isn't really lookahead (not always used in that manner) so I don't like that
name. Changed to "putBack" and updated the docs.

http://codereview.chromium.org/652016/diff/1/29#newcode508
samples/gpu2d/svgloader.js:508: BaseDataParser_.prototype.nextToken_ =
function(parseState, data) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> could you use regular expressions to simplify the lexer?

Probably, but per above I was trying to write the whole thing with simple
character fetches and substring operations.

http://codereview.chromium.org/652016/diff/1/29#newcode523
samples/gpu2d/svgloader.js:523: done = true;
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> i think this might be clearer using breaks.

Changed.

http://codereview.chromium.org/652016/diff/1/29#newcode624
samples/gpu2d/svgloader.js:624: while (numFloats-- > 0) {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> this looks like a for loop.

Is there some sort of law against while loops?

Still, changed.

http://codereview.chromium.org/652016/diff/1/29#newcode838
samples/gpu2d/svgloader.js:838: if (word == 'matrix') {
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> this looks like a switch

Changed. I'm not used to being able to use strings as keys in switch statements.

http://codereview.chromium.org/652016/diff/1/29#newcode912
samples/gpu2d/svgloader.js:912: * @param {Object} atts the XMLJS attributes
object.
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> attribs or attributes. please :) also is Object the right type?

Changed to attributes. Object is the only type I can reference without learning
how to make externs for the xmljs library, which I would rather not do right now
(at least not before the initial checkin).

http://codereview.chromium.org/652016/diff/1/38
File samples/gpu2d/svgsample.js (right):

http://codereview.chromium.org/652016/diff/1/38#newcode207
samples/gpu2d/svgsample.js:207: g_math.degToRad(15),  // fieldOfViewAngle
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> that's a narrow field of view!

The CameraController multiplies it by 2. The samples look weird with it larger
(like 30 degrees).

http://codereview.chromium.org/652016/diff/1/18
File samples/o3djs/gpu2d.js (right):

http://codereview.chromium.org/652016/diff/1/18#newcode444
samples/o3djs/gpu2d.js:444: '  float3 klm : TEXCOORD0;\n' +
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> what does klm mean?

Those are the names of the cubic texture coordinates in Loop and Blinn's paper
(k, l, m).

http://codereview.chromium.org/652016/diff/1/16
File samples/third_party/xmljs/COPYING (right):

http://codereview.chromium.org/652016/diff/1/16#newcode1
samples/third_party/xmljs/COPYING:1: GNU LIBRARY GENERAL PUBLIC LICENSE
On 2010/02/23 18:46:28, apatrick_chromium wrote:
> we can use this?

I think it should be fine. The requirements of the LGPL don't taint using code
-- just that you need to be able to "re-link" the application, which is
implicitly the case with our code since it's open source.

[apatrick_chromium, 2010-02-25 20:01:12]

LGTM for my part.

[greggman, 2010-02-25 22:26:02]

Looking good.
Just a few more questions

http://codereview.chromium.org/652016/diff/5051/4017
File core/cross/processed_path.cc (right):

http://codereview.chromium.org/652016/diff/5051/4017#newcode123
core/cross/processed_path.cc:123: Buffer* buffer = position_field->buffer();
I believe it's possible for buffer to be NULL.  You can have a Field::Ref to a
field and delete the Buffer.

http://codereview.chromium.org/652016/diff/5051/4017#newcode124
core/cross/processed_path.cc:124: if (buffer->num_elements() < num_vertices) {
I guess it's okay of there is garbage at the end. It's just that it's unclear
how the code outside is supposed to know. CreateMesh does not return a
"num_valid_vertices"

Maybe this should be if  .. != num_vertices?

http://codereview.chromium.org/652016/diff/5051/4045
File samples/gpu2d/basic.html (right):

http://codereview.chromium.org/652016/diff/5051/4045#newcode160
samples/gpu2d/basic.html:160: * @param e The mouse down event.
@param {!Event} e The event. (here and below)

http://codereview.chromium.org/652016/diff/5051/4042
File samples/gpu2d/svgloader.js (right):

http://codereview.chromium.org/652016/diff/5051/4042#newcode89
samples/gpu2d/svgloader.js:89: var xhr = new XMLHttpRequest();
This is not cross platform. I suggest you use o3djs.io.loadTextFile.  It handles
IE and it handles this code running in V8. It also supplies a LoadInfo object
that can be used and chained for reporting progress.

http://codereview.chromium.org/652016/diff/5051/4051
File samples/gpu2d/svgsample.js (right):

http://codereview.chromium.org/652016/diff/5051/4051#newcode65
samples/gpu2d/svgsample.js:65: var l = document.getElementById('log');
It seems like you could assign a noop or a function with 'log' already looked
up?

http://codereview.chromium.org/652016/diff/5051/4051#newcode94
samples/gpu2d/svgsample.js:94: * @param clientElements the array of O3D client
elements that have
{!Array.<!Element>} clientElements?

http://codereview.chromium.org/652016/diff/5051/4051#newcode153
samples/gpu2d/svgsample.js:153: * @param e The mouse down event.
@param {!Event} e ...?  (and below)

http://codereview.chromium.org/652016/diff/5051/4031
File samples/o3djs/gpu2d.js (right):

http://codereview.chromium.org/652016/diff/5051/4031#newcode55
samples/o3djs/gpu2d.js:55: * @return {o3djs.gpu2d.Path} The created Path.
!o3djs.gpu2d.Path?

http://codereview.chromium.org/652016/diff/5051/4031#newcode479
samples/o3djs/gpu2d.js:479: * @private
@return {string}

http://codereview.chromium.org/652016/diff/5051/4031#newcode482
samples/o3djs/gpu2d.js:482: fillUniforms,
indentation

http://codereview.chromium.org/652016/diff/5051/4031#newcode549
samples/o3djs/gpu2d.js:549: * @private
@return {string}

http://codereview.chromium.org/652016/diff/5051/4031#newcode628
samples/o3djs/gpu2d.js:628: * @private
@return {!o3d.Effect} ...?

http://codereview.chromium.org/652016/diff/5051/4031#newcode666
samples/o3djs/gpu2d.js:666: * @private
@return {!Array.<!o3d.Effect>} (not sure about the type)

[Ken Russell (switch to Gerrit), 2010-02-25 23:48:23]

http://codereview.chromium.org/652016/diff/5051/4017
File core/cross/processed_path.cc (right):

http://codereview.chromium.org/652016/diff/5051/4017#newcode123
core/cross/processed_path.cc:123: Buffer* buffer = position_field->buffer();
On 2010/02/25 22:26:02, greggman wrote:
> I believe it's possible for buffer to be NULL.  You can have a Field::Ref to a
> field and delete the Buffer.  

Added checks and bailouts.

http://codereview.chromium.org/652016/diff/5051/4017#newcode124
core/cross/processed_path.cc:124: if (buffer->num_elements() < num_vertices) {
On 2010/02/25 22:26:02, greggman wrote:
> I guess it's okay of there is garbage at the end. It's just that it's unclear
> how the code outside is supposed to know. CreateMesh does not return a
> "num_valid_vertices"
> 
> Maybe this should be if  .. != num_vertices?

Yes, it should have been. Fixed here and below.

http://codereview.chromium.org/652016/diff/5051/4045
File samples/gpu2d/basic.html (right):

http://codereview.chromium.org/652016/diff/5051/4045#newcode160
samples/gpu2d/basic.html:160: * @param e The mouse down event.
On 2010/02/25 22:26:02, greggman wrote:
> @param {!Event} e The event. (here and below)

Done.

http://codereview.chromium.org/652016/diff/5051/4042
File samples/gpu2d/svgloader.js (right):

http://codereview.chromium.org/652016/diff/5051/4042#newcode89
samples/gpu2d/svgloader.js:89: var xhr = new XMLHttpRequest();
On 2010/02/25 22:26:02, greggman wrote:
> This is not cross platform. I suggest you use o3djs.io.loadTextFile.  It
handles
> IE and it handles this code running in V8. It also supplies a LoadInfo object
> that can be used and chained for reporting progress.

Thanks for the suggestion. Changed.

http://codereview.chromium.org/652016/diff/5051/4051
File samples/gpu2d/svgsample.js (right):

http://codereview.chromium.org/652016/diff/5051/4051#newcode65
samples/gpu2d/svgsample.js:65: var l = document.getElementById('log');
On 2010/02/25 22:26:02, greggman wrote:
> It seems like you could assign a noop or a function with 'log' already looked
> up?

I got this from one of Mozilla's WebGL samples. Changed to look up log before
returning the function.

http://codereview.chromium.org/652016/diff/5051/4051#newcode94
samples/gpu2d/svgsample.js:94: * @param clientElements the array of O3D client
elements that have
On 2010/02/25 22:26:02, greggman wrote:
> {!Array.<!Element>} clientElements?

No, per all of the other samples it's an array of some DOM objects, I think.
Copied the doc from one of the other samples.

http://codereview.chromium.org/652016/diff/5051/4051#newcode153
samples/gpu2d/svgsample.js:153: * @param e The mouse down event.
On 2010/02/25 22:26:02, greggman wrote:
> @param {!Event} e ...?  (and below)

Done.

http://codereview.chromium.org/652016/diff/5051/4031
File samples/o3djs/gpu2d.js (right):

http://codereview.chromium.org/652016/diff/5051/4031#newcode55
samples/o3djs/gpu2d.js:55: * @return {o3djs.gpu2d.Path} The created Path.
On 2010/02/25 22:26:02, greggman wrote:
> !o3djs.gpu2d.Path?

Done.

http://codereview.chromium.org/652016/diff/5051/4031#newcode479
samples/o3djs/gpu2d.js:479: * @private
On 2010/02/25 22:26:02, greggman wrote:
> @return {string}

Done.

http://codereview.chromium.org/652016/diff/5051/4031#newcode482
samples/o3djs/gpu2d.js:482: fillUniforms,
On 2010/02/25 22:26:02, greggman wrote:
> indentation

Done.

http://codereview.chromium.org/652016/diff/5051/4031#newcode549
samples/o3djs/gpu2d.js:549: * @private
On 2010/02/25 22:26:02, greggman wrote:
> @return {string}

Done.

http://codereview.chromium.org/652016/diff/5051/4031#newcode628
samples/o3djs/gpu2d.js:628: * @private
On 2010/02/25 22:26:02, greggman wrote:
> @return {!o3d.Effect} ...?

Done.

http://codereview.chromium.org/652016/diff/5051/4031#newcode666
samples/o3djs/gpu2d.js:666: * @private
On 2010/02/25 22:26:02, greggman wrote:
> @return {!Array.<!o3d.Effect>} (not sure about the type)

Changed (to !Array.<o3d.Effect>).

[greggman, 2010-02-25 23:54:09]

LGTM

http://codereview.chromium.org/652016/diff/5051/4051
File samples/gpu2d/svgsample.js (right):

http://codereview.chromium.org/652016/diff/5051/4051#newcode94
samples/gpu2d/svgsample.js:94: * @param clientElements the array of O3D client
elements that have
On 2010/02/25 23:48:23, kbr wrote:
> On 2010/02/25 22:26:02, greggman wrote:
> > {!Array.<!Element>} clientElements?
> 
> No, per all of the other samples it's an array of some DOM objects, I think.
> Copied the doc from one of the other samples.
> 

Unfortunately most of those samples were written before anyone knew about
Google's JavaScript Style Guide.

The style guide has it as "Element" for a generic element, or you could get more
specific as in "O3DElement"

[Ken Russell (switch to Gerrit), 2010-02-26 00:25:21]

http://codereview.chromium.org/652016/diff/5051/4051
File samples/gpu2d/svgsample.js (right):

http://codereview.chromium.org/652016/diff/5051/4051#newcode94
samples/gpu2d/svgsample.js:94: * @param clientElements the array of O3D client
elements that have
On 2010/02/25 23:54:09, greggman wrote:
> On 2010/02/25 23:48:23, kbr wrote:
> > On 2010/02/25 22:26:02, greggman wrote:
> > > {!Array.<!Element>} clientElements?
> > 
> > No, per all of the other samples it's an array of some DOM objects, I think.
> > Copied the doc from one of the other samples.
> > 
> 
> Unfortunately most of those samples were written before anyone knew about
> Google's JavaScript Style Guide.
> 
> The style guide has it as "Element" for a generic element, or you could get
more
> specific as in "O3DElement"

I see. Changed.