Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8475009:
Proposed canvas and WebGL cleanup (Closed)
|
Created:
9 years, 1 month ago by Jacob Modified:
7 years, 9 months ago CC:
reviews_dartlang.org, vsm, sra1 Visibility:
Public. |
DescriptionProposed canvas and WebGL cleanup
BUG=
TEST=
Patch Set 1 : ready to discuss #
Total comments: 24
Messages
Total messages: 7 (0 generated)
I'm going to send out a number of CLs today that i'd like to go over in our meetings on Monday. These CLs only change the interfaces and not the implementations and are just intended to show speculative changes to the API so we can talk about whether they are the right way to go. http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (left): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:85: void drawImage(var canvas_OR_image, num sx_OR_x, num sy_OR_y, [num sw_OR_width, num height_OR_sh, num dx, num dy, num dw, num dh]); split drawImage into two methods so that the arguments are comprehensible. TODO(jacobr): add a comment specifying what types are allowed for [image] http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:87: void drawImageFromRect(ImageElement image, [num sx, num sy, num sw, num sh, num dx, num dy, num dw, num dh, String compositeOperation]); removed drawImageFromRect as it appears to be non-standard with functionality easily enough reproduced by calling setCompositeOperation and drawImageClipped. http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/WebGLRenderingContext.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/WebGLRenderingContext.dart:819: void texImage2DHtml(int target, int level, int internalformat, int format, int type, var image); I'm not a huge fan of these two names. Keeping these two methods as one method was unnecessarily confusing as different args are required for each version. I believe the version I've left as texImage2D is the fast path while texImage2DHtml is the slow version. I'm open to other names. It looks like some python bindings split these into multiple function names along what looks like somewhat similar lines (one of their two versions takes a multi-dimension array so just like the case of Canvas, ImageElement, etc the width and height do not make sense to specify. http://pyopengl.sourceforge.net/documentation/manual/glTexImage2D.3G.html
lgtm lgtm http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (left): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:123: void setFillColor(var c_OR_color_OR_grayLevel_OR_r, [num alpha_OR_g_OR_m, num b_OR_y, num a_OR_k, num a]); What are this and the other two below being replaced by? http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/WebGLRenderingContext.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/WebGLRenderingContext.dart:819: void texImage2DHtml(int target, int level, int internalformat, int format, int type, var image); On 2011/11/04 19:07:05, Jacob wrote: > I'm not a huge fan of these two names. > Keeping these two methods as one method was unnecessarily confusing as different > args are required for each version. > > I believe the version I've left as texImage2D is the fast path while > texImage2DHtml is the slow version. > I'm open to other names. > It looks like some python bindings split these into multiple function names > along what looks like somewhat similar lines (one of their two versions takes a > multi-dimension array so just like the case of Canvas, ImageElement, etc the > width and height do not make sense to specify. > http://pyopengl.sourceforge.net/documentation/manual/glTexImage2D.3G.html Maybe call the second method texHtml2D?
DBC http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasPixelArray.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasPixelArray.dart:8: // add additional helper methods like If we remove this, do you have an alternative scheme in mind for direct access to the underlying buffer? http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:89: void drawImage(var image, num dx, num dy, [num dw, num dh]); A Drawable interface to type image?
http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasPixelArray.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasPixelArray.dart:8: // add additional helper methods like On 2011/11/04 21:26:35, vsm wrote: > If we remove this, do you have an alternative scheme in mind for direct access > to the underlying buffer? You should still be able to directly access the underlying buffer by calling yourCanvasPixels[index] I don't see anywhere in the DOM were you can pass a CanvasPixelArray back to the DOM so there isn't a need to have a user visible type for this other than List<int> as far as I can see. However, having a marker type for this could have a little bit of value to ease debugging I guess. http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (left): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:81: CanvasPattern createPattern(var canvas_OR_image, String repetitionType); TODO(jacobr): cleanup this name as well http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:123: void setFillColor(var c_OR_color_OR_grayLevel_OR_r, [num alpha_OR_g_OR_m, num b_OR_y, num a_OR_k, num a]); On 2011/11/04 21:09:06, nweiz wrote: > What are this and the other two below being replaced by? I believe these are WebKit specific methods that never made it into the w3c spec. As they are ugly to express in dart I see little value in keeping them. The 3 removed methods are replaced by They are replaced by "set shadow\w+" setters. setFileStyle, and setStrokeStyle http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:139: void setStrokeStyle(var color_OR_gradient_OR_pattern); i'd like to cleanup this one as well. clearly it doesn't make sense to define a or type for {String, Gradient, or Pattern} http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:89: void drawImage(var image, num dx, num dy, [num dw, num dh]); On 2011/11/04 21:26:35, vsm wrote: > A Drawable interface to type image? I agree. http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/WebGLRenderingContext.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/WebGLRenderingContext.dart:819: void texImage2DHtml(int target, int level, int internalformat, int format, int type, var image); On 2011/11/04 21:09:06, nweiz wrote: > On 2011/11/04 19:07:05, Jacob wrote: > > I'm not a huge fan of these two names. > > Keeping these two methods as one method was unnecessarily confusing as > different > > args are required for each version. > > > > I believe the version I've left as texImage2D is the fast path while > > texImage2DHtml is the slow version. > > I'm open to other names. > > It looks like some python bindings split these into multiple function names > > along what looks like somewhat similar lines (one of their two versions takes > a > > multi-dimension array so just like the case of Canvas, ImageElement, etc the > > width and height do not make sense to specify. > > http://pyopengl.sourceforge.net/documentation/manual/glTexImage2D.3G.html > > Maybe call the second method texHtml2D? for webgl the convention is to append cryptic strings at the end of method names to specify different types of input args. That convention also has the benefit that if using an IDE< all the similar methods will be grouped together making it clear which version you want. One more option: texImage2DDrawable given we are considering adding a Drawable interface that matches the for DOM classes. That name is still up for debate and I'd be thrilled if we could find a better marker interface name.
http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (left): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:123: void setFillColor(var c_OR_color_OR_grayLevel_OR_r, [num alpha_OR_g_OR_m, num b_OR_y, num a_OR_k, num a]); only rgba http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:135: void setShadow(num width, num height, num blur, [var c_OR_color_OR_grayLevel_OR_r, num alpha_OR_g_OR_m, num b_OR_y, num a_OR_k, num a]); seems useful. rgba only though defaults to rgba(0, 0, 0, 1) http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:137: void setStrokeColor(var c_OR_color_OR_grayLevel_OR_r, [num alpha_OR_g_OR_m, num b_OR_y, num a_OR_k, num a]); same http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:80: ImageData createImageData(num sw, num sh); new ImageData(num sw, num sh) new ImageData.from(ImageData other)
http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:80: ImageData createImageData(num sw, num sh); On 2011/11/07 23:27:56, arv wrote: > new ImageData(num sw, num sh) > new ImageData.from(ImageData other) can we give more informative parameter names, e.g. sourceWidth, sourceHeight? http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:89: void drawImage(var image, num dx, num dy, [num dw, num dh]); On 2011/11/04 22:26:13, Jacob wrote: > On 2011/11/04 21:26:35, vsm wrote: > > A Drawable interface to type image? > > I agree. Can we use a Rect type here? Can we make the first argument be Element rather than var? http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:91: void drawImageClipped(var image, num sx, num sy, num sw, num sh, I think this should take two Rect parameters (called sourceRect and destRect) rather than all these parameters. http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:96: void fillRect(num x, num y, num width, num height); use Rect here http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:100: ImageData getImageData(num sx, num sy, num sw, num sh); use Rect here
http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... File client/html/generated/src/interface/CanvasRenderingContext2D.dart (right): http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:89: void drawImage(var image, num dx, num dy, [num dw, num dh]); On 2011/11/17 01:23:40, mattsh wrote: > On 2011/11/04 22:26:13, Jacob wrote: > > On 2011/11/04 21:26:35, vsm wrote: > > > A Drawable interface to type image? > > > > I agree. > Can we use a Rect type here? > Can we make the first argument be Element rather than var? I think that makes it more confusing as only a couple specific Element subtypes are allowed http://codereview.chromium.org/8475009/diff/2001/client/html/generated/src/in... client/html/generated/src/interface/CanvasRenderingContext2D.dart:91: void drawImageClipped(var image, num sx, num sy, num sw, num sh, On 2011/11/17 01:23:40, mattsh wrote: > I think this should take two Rect parameters (called sourceRect and destRect) > rather than all these parameters. in general i'd agree. ideally we'd like to support both versions though as this one will have a little better performance due to fewer object creations got any idea for appropriate method names for the versions that require Rect as args. *sigh* i wish dart would just support multiple method definitions with different #s of args. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
