Hello cprince,
I'd like you to do a code review. Please execute
g4 diff -c 10010577
or point your web browser to
http://mondrian/10010577
to review the following code:
Change 10010577 by nigel...@nigeltao-srcwingears5 on 2009/02/05 14:57:56
*pending*
Clean up the canvas code, removing the (unimplemented) text methods,
as well as the non-HTML5 methods (such as blur).
R=cprince
[email protected]
DELTA=366 (52 added, 307 deleted, 7 changed)
OCL=10010577
Affected files ...
... //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#20 edit
... //depot/googleclient/gears/opensource/gears/canvas/canvas.h#15 edit
...
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc#16
edit
...
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h#11
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/canvas_tests.js#12
edit
366 delta lines: 52 added, 307 deleted, 7 changed
Also consider running:
g4 lint -c 10010577
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [email protected].
Change 10010577 by nigel...@nigeltao-srcwingears5 on 2009/02/05 14:57:56
*pending*
Clean up the canvas code, removing the (unimplemented) text methods,
as well as the non-HTML5 methods (such as blur).
Affected files ...
... //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#20 edit
... //depot/googleclient/gears/opensource/gears/canvas/canvas.h#15 edit
...
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc#16
edit
...
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h#11
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/canvas_tests.js#12
edit
==== //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#20 -
c:\devel\srcwingears5/googleclient/gears/opensource/gears/canvas/canvas.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas.cc 2009-02-05
14:07:34.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas.cc 2009-02-05
14:26:51.000000000 +1100
@@ -211,8 +211,6 @@
clone->set_alpha(alpha());
clone->set_composite_operation(composite_operation());
clone->set_fill_style(fill_style());
- clone->set_font(font());
- clone->set_text_align(text_align());
// TODO(nigeltao): Copy the transformation matrix and generally make sure
that
// all state is copied.
@@ -387,34 +385,6 @@
fill_style_ = new_fill_style;
}
-std::string16 GearsCanvas::font() const {
- return font_;
-}
-
-void GearsCanvas::set_font(std::string16 new_font) {
- // TODO(nigeltao):
- // if (new_font is not a valid CSS font specification) {
- // return;
- // }
-
- font_ = new_font;
-}
-
-std::string16 GearsCanvas::text_align() const {
- return text_align_;
-}
-
-void GearsCanvas::set_text_align(std::string16 new_text_align) {
- if (new_text_align != STRING16(L"left") &&
- new_text_align != STRING16(L"center") &&
- new_text_align != STRING16(L"right") &&
- new_text_align != STRING16(L"start") &&
- new_text_align != STRING16(L"end")) {
- return;
- }
- text_align_ = new_text_align;
-}
-
void GearsCanvas::ResetCanvas(int width, int height) {
// Since we're starting with a clean slate, let's reset the SkBitmap as well.
// For some reason things don't work otherwise.
@@ -431,8 +401,6 @@
alpha_ = 1.0;
composite_operation_ = STRING16(L"source-over");
fill_style_ = STRING16(L"#000000");
- font_ = STRING16(L"10px sans-serif");
- text_align_ = STRING16(L"start");
}
SkBitmap *GearsCanvas::skia_bitmap() const {
==== //depot/googleclient/gears/opensource/gears/canvas/canvas.h#15 -
c:\devel\srcwingears5/googleclient/gears/opensource/gears/canvas/canvas.h ====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas.h 2009-02-05
14:07:34.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas.h 2009-02-05
14:07:45.000000000 +1100
@@ -126,10 +126,6 @@
bool set_composite_operation(std::string16 new_composite_op);
std::string16 fill_style() const;
void set_fill_style(std::string16 new_fill_style);
- std::string16 font() const;
- void set_font(std::string16 new_font);
- std::string16 text_align() const;
- void set_text_align(std::string16 new_text_align);
private:
// Resets the Canvas to the specified dimensions and fills it with
transparent
@@ -152,8 +148,6 @@
double alpha_;
std::string16 composite_operation_;
std::string16 fill_style_;
- std::string16 font_;
- std::string16 text_align_;
DISALLOW_EVIL_CONSTRUCTORS(GearsCanvas);
};
====
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc#16
-
c:\devel\srcwingears5/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc
2009-02-05 14:07:34.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc
2009-02-05 14:07:49.000000000 +1100
@@ -72,54 +72,89 @@
template<>
void Dispatcher<GearsCanvasRenderingContext2D>::Init() {
+ // The section headings here (e.g., "state", "transformations") are copied
+ // from the HTML5 canvas spec: http://www.whatwg.org/specs/web-apps/
+ // current-work/multipage/the-canvas-element.html
+
+ // back-reference to the canvas
RegisterProperty("canvas", &GearsCanvasRenderingContext2D::GetCanvas, NULL);
+
+ // state
RegisterMethod("save", &GearsCanvasRenderingContext2D::Save);
RegisterMethod("restore", &GearsCanvasRenderingContext2D::Restore);
+
+ // transformations
RegisterMethod("scale", &GearsCanvasRenderingContext2D::Scale);
RegisterMethod("rotate", &GearsCanvasRenderingContext2D::Rotate);
RegisterMethod("translate", &GearsCanvasRenderingContext2D::Translate);
RegisterMethod("transform", &GearsCanvasRenderingContext2D::Transform);
RegisterMethod("setTransform", &GearsCanvasRenderingContext2D::SetTransform);
+
+ // compositing
RegisterProperty("globalAlpha",
&GearsCanvasRenderingContext2D::GetGlobalAlpha,
&GearsCanvasRenderingContext2D::SetGlobalAlpha);
RegisterProperty("globalCompositeOperation",
&GearsCanvasRenderingContext2D::GetGlobalCompositeOperation,
&GearsCanvasRenderingContext2D::SetGlobalCompositeOperation);
+
+ // colors and styles
+ // Missing wrt HTML5: strokeStyle.
RegisterProperty("fillStyle",
&GearsCanvasRenderingContext2D::GetFillStyle,
&GearsCanvasRenderingContext2D::SetFillStyle);
+ // Missing wrt HTML5: createLinearGradient.
+ // Missing wrt HTML5: createRadialGradient.
+ // Missing wrt HTML5: createPattern.
+
+ // line caps/joins
+ // Missing wrt HTML5: lineWidth.
+ // Missing wrt HTML5: lineCap.
+ // Missing wrt HTML5: lineJoin.
+ // Missing wrt HTML5: miterLimit.
+
+ // shadows
+ // Missing wrt HTML5: shadowOffsetX.
+ // Missing wrt HTML5: shadowOffsetY.
+ // Missing wrt HTML5: shadowBlur.
+ // Missing wrt HTML5: shadowColor.
+
+ // rects
RegisterMethod("clearRect", &GearsCanvasRenderingContext2D::ClearRect);
RegisterMethod("fillRect", &GearsCanvasRenderingContext2D::FillRect);
RegisterMethod("strokeRect", &GearsCanvasRenderingContext2D::StrokeRect);
- RegisterProperty("font", &GearsCanvasRenderingContext2D::GetFont,
- &GearsCanvasRenderingContext2D::SetFont);
- RegisterProperty("textAlign", &GearsCanvasRenderingContext2D::GetTextAlign,
- &GearsCanvasRenderingContext2D::SetTextAlign);
- RegisterMethod("fillText", &GearsCanvasRenderingContext2D::FillText);
- RegisterMethod("measureText", &GearsCanvasRenderingContext2D::MeasureText);
+
+ // path API
+ // Missing wrt HTML5: beginPath.
+ // Missing wrt HTML5: closePath.
+ // Missing wrt HTML5: moveTo.
+ // Missing wrt HTML5: lineTo.
+ // Missing wrt HTML5: quadraticCurveTo.
+ // Missing wrt HTML5: bezierCurveTo.
+ // Missing wrt HTML5: arcTo.
+ // Missing wrt HTML5: rect.
+ // Missing wrt HTML5: arc.
+ // Missing wrt HTML5: fill.
+ // Missing wrt HTML5: stroke.
+ // Missing wrt HTML5: clip.
+ // Missing wrt HTML5: isPointInPath.
+
+ // text
+ // Missing wrt HTML5: font.
+ // Missing wrt HTML5: textAlign.
+ // Missing wrt HTML5: textBaseline.
+ // Missing wrt HTML5: fillText.
+ // Missing wrt HTML5: strokeText.
+ // Missing wrt HTML5: measureText.
+
+ // drawing images
RegisterMethod("drawImage", &GearsCanvasRenderingContext2D::DrawImage);
+
+ // pixel manipulation
RegisterMethod("createImageData",
&GearsCanvasRenderingContext2D::CreateImageData);
RegisterMethod("getImageData", &GearsCanvasRenderingContext2D::GetImageData);
RegisterMethod("putImageData", &GearsCanvasRenderingContext2D::PutImageData);
- RegisterMethod("colorTransform",
- &GearsCanvasRenderingContext2D::ColorTransform);
- RegisterMethod("convolutionTransform",
- &GearsCanvasRenderingContext2D::ConvolutionTransform);
- RegisterMethod("medianFilter", &GearsCanvasRenderingContext2D::MedianFilter);
- RegisterMethod("adjustBrightness",
- &GearsCanvasRenderingContext2D::AdjustBrightness);
- RegisterMethod("adjustContrast",
- &GearsCanvasRenderingContext2D::AdjustContrast);
- RegisterMethod("adjustSaturation",
- &GearsCanvasRenderingContext2D::AdjustSaturation);
- RegisterMethod("adjustHue",
- &GearsCanvasRenderingContext2D::AdjustHue);
- RegisterMethod("blur", &GearsCanvasRenderingContext2D::Blur);
- RegisterMethod("sharpen", &GearsCanvasRenderingContext2D::Sharpen);
- RegisterMethod("resetTransform",
- &GearsCanvasRenderingContext2D::ResetTransform);
}
// TODO(nigeltao): Unless otherwise stated, for the 2D context interface, any
@@ -319,62 +354,6 @@
{ JSPARAM_REQUIRED, JSPARAM_INT, &y },
{ JSPARAM_REQUIRED, JSPARAM_INT, &width },
{ JSPARAM_REQUIRED, JSPARAM_INT, &height }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::GetFont(JsCallContext *context) {
- std::string16 font = canvas_->font();
- context->SetReturnValue(JSPARAM_STRING16, &font);
-}
-
-void GearsCanvasRenderingContext2D::SetFont(JsCallContext *context) {
- std::string16 new_font;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_STRING16, &new_font }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- canvas_->set_font(new_font);
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::GetTextAlign(JsCallContext *context) {
- std::string16 text_align = canvas_->text_align();
- context->SetReturnValue(JSPARAM_STRING16, &text_align);
-}
-
-void GearsCanvasRenderingContext2D::SetTextAlign(JsCallContext *context) {
- std::string16 new_align;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_STRING16, &new_align }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- canvas_->set_text_align(new_align);
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::FillText(JsCallContext *context) {
- std::string16 text;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_STRING16, &text }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::MeasureText(JsCallContext *context) {
- std::string16 text;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_STRING16, &text }
};
context->GetArguments(ARRAYSIZE(args), args);
if (context->is_exception_set())
@@ -568,111 +547,3 @@
return;
context->SetException(STRING16(L"Unimplemented"));
}
-
-void GearsCanvasRenderingContext2D::ColorTransform(JsCallContext *context) {
- scoped_ptr<JsObject> matrix;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_OBJECT, as_out_parameter(matrix) }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::ConvolutionTransform(
- JsCallContext *context) {
- scoped_ptr<JsObject> matrix;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_OBJECT, as_out_parameter(matrix) }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::MedianFilter(JsCallContext *context) {
- double radius;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &radius }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::AdjustBrightness(JsCallContext *context) {
- double delta;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &delta }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::AdjustContrast(JsCallContext *context) {
- double amount;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &amount }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::AdjustSaturation(JsCallContext *context) {
- double amount;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &amount }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::AdjustHue(JsCallContext *context) {
- double angle;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &angle }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::Blur(JsCallContext *context) {
- double factor;
- int radius;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &factor },
- { JSPARAM_REQUIRED, JSPARAM_INT, &radius }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::Sharpen(JsCallContext *context) {
- double factor;
- int radius;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &factor },
- { JSPARAM_REQUIRED, JSPARAM_INT, &radius }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
-
-void GearsCanvasRenderingContext2D::ResetTransform(JsCallContext *context) {
- context->SetException(STRING16(L"Unimplemented"));
-}
====
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h#11
-
c:\devel\srcwingears5/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h
2009-02-05 14:07:34.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h
2009-02-05 14:07:54.000000000 +1100
@@ -148,27 +148,6 @@
// TODO(nigeltao): Define stroke color.
void StrokeRect(JsCallContext *context);
- // These affect future text rendering-related
- // operations (FillText and MeasureText).
- void GetFont(JsCallContext *context);
- void SetFont(JsCallContext *context);
- void GetTextAlign(JsCallContext *context);
- void SetTextAlign(JsCallContext *context);
-
- // Renders filled text at the specified point (baseline left).
- // If a maxWidth is specified and the text exceeds the width, it is
- // truncated, unless wrap is specified, in which case it's wrapped.
- // The text may exceed the height of the canvas (especially with
- // wrapping) in which case it's truncated vertically.
- // IN: string text, float x, float y, optional float maxWidth
- // OUT: -
- void FillText(JsCallContext *context);
-
- // Returns a text metrics object with information about the given text.
- // IN: string text
- // OUT: TextMetrics
- void MeasureText(JsCallContext *context);
-
// Draws a canvas onto this canvas. sx, sy, sw, and sh identify a rectangular
// subset of the source canvas, and dx, dy, dw, dh identify the target area
// in the destination canvas (this canvas) into which pixels from the source
@@ -200,59 +179,6 @@
// OUT: -
void PutImageData(JsCallContext *context);
- // Performs a color transformation on the canvas with the given matrix.
- // IN: double[] colorMatrix
- // OUT: -
- void ColorTransform(JsCallContext *context);
-
- // Performs a convolution transformation on the canvas with the given matrix.
- // IN: double[] convolutionMatrix, optional float bias,
- // optional boolean applyToAlpha
- // OUT: -
- void ConvolutionTransform(JsCallContext *context);
-
- // Performs a median filter with the given radius, which can be fractional.
- // IN: double radius
- // OUT: -
- void MedianFilter(JsCallContext *context);
-
- // Adjusts brigthness by the given amount, which is between -1.0 and +1.0
- // IN: double delta.
- // OUT: -
- void AdjustBrightness(JsCallContext *context);
-
- // Adjusts contrast by the given amount.
- // IN: double contrast.
- // OUT: -
- void AdjustContrast(JsCallContext *context);
-
- // Adjusts saturation by the given amount, while preserving luminance.
- // IN: double saturation
- // OUT: -
- void AdjustSaturation(JsCallContext *context);
-
- // Rotates hue on the color wheel by the given amount, while preserving
- // luminance.
- // IN: double angle
- // OUT: -
- void AdjustHue(JsCallContext *context);
-
- // Performs a blur by taking a weighted mean of pixels within the
- // specified radius.
- // IN: double factor, int radius.
- // OUT: -
- void Blur(JsCallContext *context);
-
- // Inverse of Blur().
- // IN: double factor, int radius.
- // OUT: -
- void Sharpen(JsCallContext *context);
-
- // Resets the transformation matrix to the identity matrix.
- // IN: -
- // OUT: -
- void ResetTransform(JsCallContext *context);
-
private:
// Calls ClearRenderingContextReference() on canvas_, if canvas_ is not NULL.
void ClearReferenceFromGearsCanvas();
====
//depot/googleclient/gears/opensource/gears/test/testcases/canvas_tests.js#12 -
c:\devel\srcwingears5/googleclient/gears/opensource/gears/test/testcases/canvas_tests.js
====
# action=edit type=text
--- googleclient/gears/opensource/gears/test/testcases/canvas_tests.js
2009-02-05 15:32:19.000000000 +1100
+++ googleclient/gears/opensource/gears/test/testcases/canvas_tests.js
2009-02-05 14:19:27.000000000 +1100
@@ -100,9 +100,7 @@
assertEqual(1.0, ctx.globalAlpha);
assertEqual('source-over', ctx.globalCompositeOperation);
assertEqual('#000000', ctx.fillStyle);
- assertEqual('10px sans-serif', ctx.font);
- assertEqual('start', ctx.textAlign);
-}
+
function testCanvasStateInitially() {
startAsync();
@@ -120,9 +118,6 @@
ctx.globalAlpha = 0.45;
ctx.globalCompositeOperation = 'copy';
ctx.fillStyle = '#12345F';
- // TODO(nigeltao): Uncomment after implementing the properties.
- // ctx.font = '24px Tahoma';
- // ctx.textAlign = 'right';
}
// Tests that changing width or height resets all pixels to transparent black.
@@ -269,15 +264,10 @@
var alpha = 0.5;
var compositeOperation = 'copy';
var fillStyle = '#397F90';
- // var font = '24px tahoma';
- // var textAlign = 'center';
originalCtx.globalAlpha = alpha;
originalCtx.globalCompositeOperation = compositeOperation;
originalCtx.fillStyle = fillStyle;
- // TODO(nigeltao): After implementing font and textAlign, uncomment these.
- // originalCtx.font = font;
- // originalCtx.textAlign = textAlign;
// Now clone the canvas and check that the clone and its context are
// identical to the originals.
@@ -292,8 +282,6 @@
assertEqual(originalCtx.globalCompositeOperation,
cloneCtx.globalCompositeOperation);
assertEqual(originalCtx.fillStyle, cloneCtx.fillStyle);
- // assertEqual(originalCtx.font, cloneCtx.font);
- // assertEqual(originalCtx.textAlign, cloneCtx.textAlign);
// Now perform various operations on the clone and check that the original
@@ -306,8 +294,6 @@
assertEqual(alpha, originalCtx.globalAlpha);
assertEqual(compositeOperation, originalCtx.globalCompositeOperation);
assertEqual(fillStyle, originalCtx.fillStyle);
- // assertEqual(font, ctx.font);
- // assertEqual(textAlign, ctx.textAlign);
}
checkOriginal();
