Hello cprince,
I'd like you to do a code review. Please execute
g4 diff -c 10391639
or point your web browser to
http://mondrian/10391639
to review the following code:
Change 10391639 by nigel...@nigeltao-srcgears5 on 2009/03/06 14:40:15 *pending*
In GearsCanvas, lazily call SkBitmap::allocPixels. This
cuts down the number of allocPixels calls from 3 to 1 for
something like:
var canvas = factory.create('beta.canvas');
canvas.width = 100;
canvas.height = 80;
PRESUBMIT=passed
R=cprince
[email protected]
DELTA=25 (14 added, 10 deleted, 1 changed)
OCL=10391639
Affected files ...
... //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#23 edit
... //depot/googleclient/gears/opensource/gears/canvas/canvas.h#18 edit
25 delta lines: 14 added, 10 deleted, 1 changed
Also consider running:
g4 lint -c 10391639
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 10391639 by nigel...@nigeltao-srcgears5 on 2009/03/06 14:40:15 *pending*
In GearsCanvas, lazily call SkBitmap::allocPixels. This
cuts down the number of allocPixels calls from 3 to 1 for
something like:
var canvas = factory.create('beta.canvas');
canvas.width = 100;
canvas.height = 80;
Affected files ...
... //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#23 edit
... //depot/googleclient/gears/opensource/gears/canvas/canvas.h#18 edit
==== //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#23 -
/home/nigeltao/srcgears5/googleclient/gears/opensource/gears/canvas/canvas.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas.cc 2009-03-06
14:34:56.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas.cc 2009-03-06
14:37:29.000000000 +1100
@@ -73,6 +73,13 @@
RegisterMethod("getContext", &GearsCanvas::GetContext);
}
+void GearsCanvas::EnsureBitmapPixelsAreAllocated() {
+ if (!skia_bitmap_->getPixels()) {
+ skia_bitmap_->allocPixels();
+ skia_bitmap_->eraseARGB(0, 0, 0, 0);
+ }
+}
+
void GearsCanvas::Load(JsCallContext *context) {
ModuleImplBaseClass *other_module;
JsArgument args[] = {
@@ -124,9 +131,7 @@
return;
}
- // Pixels should have been allocated, either in the contructor,
- // or in SetWidth() and SetHeight().
- assert(skia_bitmap_->getPixels());
+ EnsureBitmapPixelsAreAllocated();
// SkBitmap's isOpaque flag tells whether the bitmap has any transparent
// pixels. If this flag is set, the encoder creates a file without alpha
@@ -213,6 +218,7 @@
L"bounds of the bitmap or has negative dimensions"));
return;
}
+ EnsureBitmapPixelsAreAllocated();
SkBitmap new_bitmap;
new_bitmap.setConfig(skia_config, width, height);
new_bitmap.allocPixels();
@@ -238,6 +244,7 @@
context->SetException(STRING16(L"Cannot resize to negative dimensions."));
return;
}
+ EnsureBitmapPixelsAreAllocated();
SkBitmap new_bitmap;
new_bitmap.setConfig(skia_config, new_width, new_height);
new_bitmap.allocPixels();
@@ -312,6 +319,7 @@
context->SetException(STRING16(L"Unable to create context"));
return;
}
+ EnsureBitmapPixelsAreAllocated();
rendering_context_ = rendering_context_scoped_ptr.get();
rendering_context_->SetCanvas(this, skia_bitmap_.get());
}
@@ -353,16 +361,8 @@
}
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.
- // TODO(nigeltao): Figure out why.
skia_bitmap_.reset(new SkBitmap);
skia_bitmap_->setConfig(skia_config, width, height);
-
- // Must allocate pixels before performing any operations,
- // or assertions fire and some operations (like eraseARGB) fail silently.
- skia_bitmap_->allocPixels();
- skia_bitmap_->eraseARGB(0, 0, 0, 0);
alpha_ = 1.0;
composite_operation_ = STRING16(L"source-over");
==== //depot/googleclient/gears/opensource/gears/canvas/canvas.h#18 -
/home/nigeltao/srcgears5/googleclient/gears/opensource/gears/canvas/canvas.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas.h 2009-03-06
14:34:56.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas.h 2009-03-06
14:37:30.000000000 +1100
@@ -122,6 +122,10 @@
// requires this when the user sets the canvas's width or height.
void ResetCanvas(int width, int height);
+ // We lazily allocate the SkBitmap's pixels. This method ensures that those
+ // pixels have been allocated.
+ void EnsureBitmapPixelsAreAllocated();
+
// Can't use a scoped_refptr since that will create a reference cycle.
// Instead, use a plain pointer and clear it when the target is destroyed.
// Recreate this pointer when accessed again. For this to work, we make
