Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(156)

Issue 9698043: Refactor Canvas 2D context taming to prototype style. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 10 months ago by kpreid2
Modified:
12 years, 10 months ago
Reviewers:
felix8a
CC:
google-caja-discuss_googlegroups.com
Base URL:
http://google-caja.googlecode.com/svn/trunk/
Visibility:
Public.

Description

* CanvasRenderingContext2D, CanvasGradient, and ImageData have regular taming ctors and prototypes rather than closure-based wrappers. * Method taming tools used by canvas taming are now implemented as property specs as used in the rest of Domado. * Removed drawFocusRing taming as drawFocusRing no longer exists in spec or implementations. * 2D canvas is no longer fetched overeagerly and <canvas>.getContext is capable of supporting other context types (if we had tamings for them). Supporting changes in es53-test-scan-guest: * Refer to canvas methods via prototype rather than string matching. * Fix bugs in "Show only errors" checkbox. @r5422

Patch Set 1 #

Patch Set 2 : Refactor Canvas 2D context taming to prototype style. #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+481 lines, -558 lines) Patch
M src/com/google/caja/plugin/domado.js View 1 5 chunks +417 lines, -463 lines 12 comments Download
M tests/com/google/caja/plugin/es53-test-domado-canvas-guest.html View 1 chunk +0 lines, -50 lines 0 comments Download
M tests/com/google/caja/plugin/es53-test-scan-guest.js View 7 chunks +64 lines, -45 lines 0 comments Download

Messages

Total messages: 3
kpreid2
12 years, 10 months ago (2013-05-23 18:46:52 UTC) #1
kpreid2
* CanvasRenderingContext2D, CanvasGradient, and ImageData have regular taming ctors and prototypes rather than closure-based wrappers. ...
12 years, 10 months ago (2013-05-23 18:54:17 UTC) #2
felix8a
12 years, 10 months ago (2013-05-23 20:46:27 UTC) #3
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/d...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/9698043/diff/3001/src/com/google/caja/plugin/d...
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-elemen...

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/d...
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/d...
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/d...
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/d...
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/d...
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/d...
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/d...
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/d...
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/d...
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/d...
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/d...
src/com/google/caja/plugin/domado.js:4908: throw new Error('Domita: canvas
drawImage not yet implemented');
domado
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b