lgtm++
some of the comments are about code that was just moved, not changed. I
started looking at all that in detail since rietveld doesn't know how to
align the diffs, but halfway through my attention started wandering, so
I threw the patch into diffmerge and focused on just the parts that were
changed.
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4799
src/com/google/caja/plugin/domado.js:4799: //
http://dev.w3.org/html5/2dcontext/
I usually look at whatwg instead of w3c, how about list both?
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#2dcontext
also, mdn is often a better guide to what's actually implemented by
browsers (not just by firefox)
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4815
src/com/google/caja/plugin/domado.js:4815: return '[Domado
CanvasRenderingContext2D]';
'[domado object ...'
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4818
src/com/google/caja/plugin/domado.js:4818: save: tameSimpleOp,
'simple' is vague, I'd prefer something like 'tameNoArgOp'
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4825
src/com/google/caja/plugin/domado.js:4825: setTransform:
tameFloatsOp(6),
note unimplemented: whatwg has resetTransform, though nobody seems to
implement it yet
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4829
src/com/google/caja/plugin/domado.js:4829: enforceType(x0, 'number',
'x0');
I think I'd prefer removing most of the enforceType 'number' in the
canvas code and just coercing to number with unary +. mainly, the cost
and codesize of all that redundant checking seems not worth it.
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4867
src/com/google/caja/plugin/domado.js:4867: arcTo: tameFloatsOp(5),
note unimplemented: whatwg adds two optional args to arcTo
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4876
src/com/google/caja/plugin/domado.js:4876: enforceType(anticlockwise,
'boolean', 'anticlockwise');
anticlockwise is an optional arg
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4877
src/com/google/caja/plugin/domado.js:4877: if (radius < 0) {
I don't think we gain anything by doing this check ourselves.
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4885
src/com/google/caja/plugin/domado.js:4885: fill: tameSimpleOp,
note unimplemented: w3c and whatwg say that fill, stroke, and clip can
have a path as arg 1, and w3c says fill and clip can have a string as
arg 1.
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4891
src/com/google/caja/plugin/domado.js:4891: isPointInPath:
tameMethodCustom(function(privates, x, y) {
note unimplemented: w3c and whatwg have 3 arg variant, and w3c has a 4
arg variant
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4906
src/com/google/caja/plugin/domado.js:4906: // Consider what policy to
have wrt reading the pixels from image
mark this with TODO. many canvas things are pretty much not doable
without drawImage
https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/domado.js#newcode4908
src/com/google/caja/plugin/domado.js:4908: throw new Error('Domita:
canvas drawImage not yet implemented');
domado
https://codereview.appspot.com/9698043/
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
