expose bitmap in js.

expose bitmap in js.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20700

[gman, 2009-07-07 00:47:55]

Please fix all the lint errors.

[gman, 2009-07-07 02:22:17]

Hi Xiao,

I hope I wasn't too harsh but it looks like there's still some mis-communication
on what we wanted. If you have any questions feel free to ask.

http://codereview.chromium.org/150058/diff/2009/2018
File core/cross/bitmap.cc (right):

http://codereview.chromium.org/150058/diff/2009/2018#newcode43
Line 43: #include "core/cross/pack.h"
This line is no longer needed? Please delete if so.

http://codereview.chromium.org/150058/diff/2009/2018#newcode282
Line 282: // first check adjust start points and measures (width and height)
Needs to be proper a proper sentence. (Yes, I know that's not your first
language).  How about

"Clip source and destination rectangles to source and destination bitmaps."

http://codereview.chromium.org/150058/diff/2009/2018#newcode284
Line 284: if (!AdjustDrawImageBoundary(src_x, src_y,
This function should never fail.

Making a rectangle completely outside either the source or destination just
means the result is a zero width rectangle which is perfectly legal.

http://codereview.chromium.org/150058/diff/2009/2018#newcode299
Line 299: if (src_img->format_ == Texture::XRGB8 ||
All formats should be supported in the case of everything being the same size
and you're copying the entire bitmap.

In other words: If 
  src.format == dst.format &&
  src.width == dst.width &&
  src.height == dst.height &&
  src.x == 0 && src.y == 0 &&
  dst.x == 0 && dst.y == 0 &&
  src_img.width == dst.img.width &&
  src_img.height == dst.img.height) {
 // just do a plain copy of src -> dst.
}

http://codereview.chromium.org/150058/diff/2009/2018#newcode303
Line 303: DLOG(ERROR) << "DrawImage is not supported from format: "
How about "is not" -> "does not"

http://codereview.chromium.org/150058/diff/2009/2018#newcode312
Line 312: src_img->width_, src_img->height_, dst_img_data,
Indentation is off.

http://codereview.chromium.org/150058/diff/2009/2018#newcode331
Line 331: if (src_x < 0 || src_x + src_width > src_img_width ||
Change these to DCHECK. There is no reason the check these and return true,
false. Valid values should always be passed to this function.

http://codereview.chromium.org/150058/diff/2009/2018#newcode340
Line 340: for (unsigned int y = dest_height - 1; y < dest_height; --y) {
This makes no sense

Assume dest height = 100

The code above says:

for (y = 99; y < 100; --y)

The only reason this works at all is because when Y is zero Y becomes a large
number. Coding for side effects is not a good way to code.

On top of that, there is no reason to backward. Just go y = 0; y < dest_height;
++y)

http://codereview.chromium.org/150058/diff/2009/2018#newcode346
Line 346: for (unsigned int c = 0; c < components; ++c) {
When src_width == dst_width and src_height == dst_height then doing a per pixel
copy is fine.

When they are not the same it should bilinear interpolate to do the scaling.

http://codereview.chromium.org/150058/diff/2009/2018#newcode683
Line 683: // if width and height of src and dest are zero or negative.
negative width and height are legal values. That's how you flip an image.

http://codereview.chromium.org/150058/diff/2009/2018#newcode738
Line 738: if (src_length <= 0 || dest_length <= 0)
A negative start point is a valid point.

If I ask for 

src_x = -10
src_y = -5
src_width = 15
src_height = 7

dst_x = 16
dst_y = 20
dst_width = src_width;
dst_height = src_height;

What I should effectively get is

src_x = 0;
src_y = 0;
src_width = 5
src_height = 2
dst_x = 26
dst_y = 25
dst_width = 5
dst_height = 2

http://codereview.chromium.org/150058/diff/2009/2014
File core/cross/bitmap.h (right):

http://codereview.chromium.org/150058/diff/2009/2014#newcode252
Line 252: //   true if drawing image succeeds.
No need to return true or false.  The only reasons this should fail is because
you didn't something completely wrong, like try to drawImage into a DXT texture,
in that case just called O3D_ERROR because it's basically an exception.

http://codereview.chromium.org/150058/diff/2009/2014#newcode258
Line 258: // Crop part of an image from src, scale it to an arbitrary size
If this function is static does it need to be part of the public interface of
Bitmap?

http://codereview.chromium.org/150058/diff/2009/2014#newcode281
Line 281: static bool CropAndScale(const unsigned char* src, int src_x,
use uint8 instead of unsigned char.

http://codereview.chromium.org/150058/diff/2009/2014#newcode399
Line 399: private:
blank line before private:
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Class_Format#Class_Fo...

http://codereview.chromium.org/150058/diff/2009/2014#newcode440
Line 440: int& src_length, int& dest_length,
indentation is off

http://codereview.chromium.org/150058/diff/2009/2014#newcode443
Line 443: O3D_DECL_CLASS(Bitmap, ParamObject)
please end this line with a semicolon. It shouldn't require you to read all the
headerfiles to know that it doesn't need one so just put one and the issue goes
away.

http://codereview.chromium.org/150058/diff/2009/2011
File core/cross/bitmap_test.cc (right):

http://codereview.chromium.org/150058/diff/2009/2011#newcode856
Line 856: 
You need to add tests for DrawImage.

They should include drawing drawing on all boundries (left, left-top, top,
top-right, right, bottom-right, bottom, bottom-left, middle.  

They should include drawing 100% off for all 8 cases (left, left-top, top,
top-right, right, bottom-right, bottom, bottom-left, middle. )

They should include scaling up, scaling down, scaling larger than the
destination and also the all formats.

http://codereview.chromium.org/150058/diff/2009/2015
File core/cross/pack.cc (right):

http://codereview.chromium.org/150058/diff/2009/2015#newcode233
Line 233: if (!bitmap.IsNull()) {
This check needs to come before Allocate, otherwise it can crash trying to call
Allocate.

Plus you need to check the results of Allocate.

http://codereview.chromium.org/150058/diff/2009/2015#newcode249
Line 249: if (!bitmap.IsNull()) {
Same here. This check has to come on like 242

http://codereview.chromium.org/150058/diff/2009/2012
File core/cross/pack.h (right):

http://codereview.chromium.org/150058/diff/2009/2012#newcode218
Line 218: // Create a new Bitmap object of the specified size and format and
Create -> Creates

http://codereview.chromium.org/150058/diff/2009/2012#newcode227
Line 227: Bitmap* CreateBitmap(int width, int height,
This looks like it will fit in one line.

http://codereview.chromium.org/150058/diff/2009/2013
File core/cross/texture.cc (right):

http://codereview.chromium.org/150058/diff/2009/2013#newcode77
Line 77: bool Texture2D::DrawImage(Bitmap* src_img,
This function has all the same issues as Bitmap::DrawImage

http://codereview.chromium.org/150058/diff/2009/2013#newcode187
Line 187: bool TextureCUBE::DrawImage(Bitmap* src_img,
This function has all the same issues as Bitmap::DrawImage

http://codereview.chromium.org/150058/diff/2009/2025
File plugin/idl/bitmap.idl (right):

http://codereview.chromium.org/150058/diff/2009/2025#newcode38
Line 38: a RawData object via LoadFromRawData(), and also can be transfered
should be "The contents of a Bitmap can be created from RawData via
pack.createBitmapFromRawData()"

http://codereview.chromium.org/150058/diff/2009/2026
File samples/bitmap-draw-image.html (right):

http://codereview.chromium.org/150058/diff/2009/2026#newcode207
Line 207: var texture = g_pack.createTexture2D(300, 300, g_o3d.Texture.XRGB8, 0,
false);
> 80 chars

http://codereview.chromium.org/150058/diff/2009/2026#newcode217
Line 217: if (texture) {
There's no reason to check "if (texture)" given that the previous 6 lines are
using the texture.

http://codereview.chromium.org/150058/diff/2009/2026#newcode218
Line 218: var shape = makeShape(texture, effect);
shape is not used. No reason to set it.

http://codereview.chromium.org/150058/diff/2009/2026#newcode233
Line 233: This tutorial shows how to create bitmap and how to draw image on both
bitmap and texture.
> 80 chars

[yux, 2009-07-09 23:31:36]

Fixed. Could you review them again, pls?
Thanks.
---
Xiao
On 2009/07/07 02:22:17, gman wrote:
> Hi Xiao,
> 
> I hope I wasn't too harsh but it looks like there's still some
mis-communication
> on what we wanted. If you have any questions feel free to ask.
> 
> http://codereview.chromium.org/150058/diff/2009/2018
> File core/cross/bitmap.cc (right):
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode43
> Line 43: #include "core/cross/pack.h"
> This line is no longer needed? Please delete if so.
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode282
> Line 282: // first check adjust start points and measures (width and height)
> Needs to be proper a proper sentence. (Yes, I know that's not your first
> language).  How about
> 
> "Clip source and destination rectangles to source and destination bitmaps."
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode284
> Line 284: if (!AdjustDrawImageBoundary(src_x, src_y,
> This function should never fail.
> 
> Making a rectangle completely outside either the source or destination just
> means the result is a zero width rectangle which is perfectly legal.
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode299
> Line 299: if (src_img->format_ == Texture::XRGB8 ||
> All formats should be supported in the case of everything being the same size
> and you're copying the entire bitmap.
> 
> In other words: If 
>   src.format == dst.format &&
>   src.width == dst.width &&
>   src.height == dst.height &&
>   src.x == 0 && src.y == 0 &&
>   dst.x == 0 && dst.y == 0 &&
>   src_img.width == dst.img.width &&
>   src_img.height == dst.img.height) {
>  // just do a plain copy of src -> dst.
> }
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode303
> Line 303: DLOG(ERROR) << "DrawImage is not supported from format: "
> How about "is not" -> "does not"
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode312
> Line 312: src_img->width_, src_img->height_, dst_img_data,
> Indentation is off.
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode331
> Line 331: if (src_x < 0 || src_x + src_width > src_img_width ||
> Change these to DCHECK. There is no reason the check these and return true,
> false. Valid values should always be passed to this function.
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode340
> Line 340: for (unsigned int y = dest_height - 1; y < dest_height; --y) {
> This makes no sense
> 
> Assume dest height = 100
> 
> The code above says:
> 
> for (y = 99; y < 100; --y)
> 
> The only reason this works at all is because when Y is zero Y becomes a large
> number. Coding for side effects is not a good way to code.
> 
> On top of that, there is no reason to backward. Just go y = 0; y <
dest_height;
> ++y)
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode346
> Line 346: for (unsigned int c = 0; c < components; ++c) {
> When src_width == dst_width and src_height == dst_height then doing a per
pixel
> copy is fine.
> 
> When they are not the same it should bilinear interpolate to do the scaling.
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode683
> Line 683: // if width and height of src and dest are zero or negative.
> negative width and height are legal values. That's how you flip an image.
> 
> http://codereview.chromium.org/150058/diff/2009/2018#newcode738
> Line 738: if (src_length <= 0 || dest_length <= 0)
> A negative start point is a valid point.
> 
> If I ask for 
> 
> src_x = -10
> src_y = -5
> src_width = 15
> src_height = 7
> 
> dst_x = 16
> dst_y = 20
> dst_width = src_width;
> dst_height = src_height;
> 
> What I should effectively get is
> 
> src_x = 0;
> src_y = 0;
> src_width = 5
> src_height = 2
> dst_x = 26
> dst_y = 25
> dst_width = 5
> dst_height = 2
> 
> http://codereview.chromium.org/150058/diff/2009/2014
> File core/cross/bitmap.h (right):
> 
> http://codereview.chromium.org/150058/diff/2009/2014#newcode252
> Line 252: //   true if drawing image succeeds.
> No need to return true or false.  The only reasons this should fail is because
> you didn't something completely wrong, like try to drawImage into a DXT
texture,
> in that case just called O3D_ERROR because it's basically an exception.
> 
> http://codereview.chromium.org/150058/diff/2009/2014#newcode258
> Line 258: // Crop part of an image from src, scale it to an arbitrary size
> If this function is static does it need to be part of the public interface of
> Bitmap?
> 
> http://codereview.chromium.org/150058/diff/2009/2014#newcode281
> Line 281: static bool CropAndScale(const unsigned char* src, int src_x,
> use uint8 instead of unsigned char.
> 
> http://codereview.chromium.org/150058/diff/2009/2014#newcode399
> Line 399: private:
> blank line before private:
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Class_Format#Class_Fo...
> 
> http://codereview.chromium.org/150058/diff/2009/2014#newcode440
> Line 440: int& src_length, int& dest_length,
> indentation is off
> 
> http://codereview.chromium.org/150058/diff/2009/2014#newcode443
> Line 443: O3D_DECL_CLASS(Bitmap, ParamObject)
> please end this line with a semicolon. It shouldn't require you to read all
the
> headerfiles to know that it doesn't need one so just put one and the issue
goes
> away.
> 
> http://codereview.chromium.org/150058/diff/2009/2011
> File core/cross/bitmap_test.cc (right):
> 
> http://codereview.chromium.org/150058/diff/2009/2011#newcode856
> Line 856: 
> You need to add tests for DrawImage.
> 
> They should include drawing drawing on all boundries (left, left-top, top,
> top-right, right, bottom-right, bottom, bottom-left, middle.  
> 
> They should include drawing 100% off for all 8 cases (left, left-top, top,
> top-right, right, bottom-right, bottom, bottom-left, middle. )
> 
> They should include scaling up, scaling down, scaling larger than the
> destination and also the all formats.
> 
> http://codereview.chromium.org/150058/diff/2009/2015
> File core/cross/pack.cc (right):
> 
> http://codereview.chromium.org/150058/diff/2009/2015#newcode233
> Line 233: if (!bitmap.IsNull()) {
> This check needs to come before Allocate, otherwise it can crash trying to
call
> Allocate.
> 
> Plus you need to check the results of Allocate.
> 
> http://codereview.chromium.org/150058/diff/2009/2015#newcode249
> Line 249: if (!bitmap.IsNull()) {
> Same here. This check has to come on like 242
> 
> http://codereview.chromium.org/150058/diff/2009/2012
> File core/cross/pack.h (right):
> 
> http://codereview.chromium.org/150058/diff/2009/2012#newcode218
> Line 218: // Create a new Bitmap object of the specified size and format and
> Create -> Creates
> 
> http://codereview.chromium.org/150058/diff/2009/2012#newcode227
> Line 227: Bitmap* CreateBitmap(int width, int height,
> This looks like it will fit in one line.
> 
> http://codereview.chromium.org/150058/diff/2009/2013
> File core/cross/texture.cc (right):
> 
> http://codereview.chromium.org/150058/diff/2009/2013#newcode77
> Line 77: bool Texture2D::DrawImage(Bitmap* src_img,
> This function has all the same issues as Bitmap::DrawImage
> 
> http://codereview.chromium.org/150058/diff/2009/2013#newcode187
> Line 187: bool TextureCUBE::DrawImage(Bitmap* src_img,
> This function has all the same issues as Bitmap::DrawImage
> 
> http://codereview.chromium.org/150058/diff/2009/2025
> File plugin/idl/bitmap.idl (right):
> 
> http://codereview.chromium.org/150058/diff/2009/2025#newcode38
> Line 38: a RawData object via LoadFromRawData(), and also can be transfered
> should be "The contents of a Bitmap can be created from RawData via
> pack.createBitmapFromRawData()"
> 
> http://codereview.chromium.org/150058/diff/2009/2026
> File samples/bitmap-draw-image.html (right):
> 
> http://codereview.chromium.org/150058/diff/2009/2026#newcode207
> Line 207: var texture = g_pack.createTexture2D(300, 300, g_o3d.Texture.XRGB8,
0,
> false);
> > 80 chars
> 
> http://codereview.chromium.org/150058/diff/2009/2026#newcode217
> Line 217: if (texture) {
> There's no reason to check "if (texture)" given that the previous 6 lines are
> using the texture.
> 
> http://codereview.chromium.org/150058/diff/2009/2026#newcode218
> Line 218: var shape = makeShape(texture, effect);
> shape is not used. No reason to set it.
> 
> http://codereview.chromium.org/150058/diff/2009/2026#newcode233
> Line 233: This tutorial shows how to create bitmap and how to draw image on
both
> bitmap and texture.
> > 80 chars

[gman, 2009-07-10 22:25:04]

It's looking good.

There's a few things. You need to change the non-const references to pointers.

Then there are 2 big issues but you can do those in another CL.

We also need canvas.drawImage(texture, ...) in another CL as well.

http://codereview.chromium.org/150058/diff/2072/3058
File core/cross/bitmap.cc (right):

http://codereview.chromium.org/150058/diff/2072/3058#newcode48
Line 48: #include <cstring>
system includes always go first. The lint errors on this page 

http://codereview.chromium.org/150058

point this out

http://codereview.chromium.org/150058/diff/2072/3058#newcode302
Line 302: O3D_ERROR(service_locator()) << "DrawImage does not support "
This can be in another CL but it should be possible to draw RGB -> RGBA and RGBA
-> RGB

http://codereview.chromium.org/150058/diff/2072/3058#newcode346
Line 346: void Bitmap::BilinearInterpolateScale(const uint8* src_img_data,
This is my fault again. BilinearInterpolation only works for scaling up.

Using this function, if you wanted to make mips you'd have to do this

var b1 = pack.createBitmap(
   originalBitmap.width, originalBitmap.height, ...);
b1.drawImage(originalBitmap, 0, 0, 
             originalBitmap.width, originalBitmap.height,
             0, 0, b1.width, b1.height);
var b2 = pack.createBitmap(b1.width / 2, b1.height / 2, ..);
b2.drawImage(b1, 0, 0, 
             b1.width, b1.height,
             0, 0, b2.width, b2.height);
var b3 = pack.createBitmap(b2.width / 2, b2.height / 2, ..);
b3.drawImage(b2, 0, 0, 
             b2.width, b2.height,
             0, 0, b3.width, b3.height);
etc..
var t = pack.createTexture2D(
   originalBitmap.width, originalBitmap.height, ...);
t.drawImage(originalBitmap, ...., 0);
t.drawImage(b1, ...., 1);
t.drawImage(b2, ...., 2);
t.drawImage(b3, ...., 3);
pack.removeObject(originalBitmap);
pack.removeObject(b1);
pack.removeObject(b2);
pack.removeObject(b3);

Where as what we want to be able to do this this.

var t = pack.createTexture2D(
   originalBitmap.width, originalBitmap.height, ...);
t.drawImage(originalBitmap, ...., 0);
t.drawImage(originalBitmap, ...., 1);
t.drawImage(originalBitmap, ...., 2);
t.drawImage(originalBitmap, ...., 3);
pack.removeObject(originalBitmap);

We can fix this in another CL.

http://codereview.chromium.org/150058/diff/2072/3054
File core/cross/bitmap.h (right):

http://codereview.chromium.org/150058/diff/2072/3054#newcode394
Line 394: static bool AdjustDrawImageBoundary(int& src_x, int& src_y,
non-const references are not allowed by the google style guide.

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Reference_Arguments#R...

http://codereview.chromium.org/150058/diff/2072/3054#newcode429
Line 429: static bool AdjustDrawImageBoundHelper(int& src_a, int& dest_a,
non-const references not allowed.

http://codereview.chromium.org/150058/diff/2072/3064
File plugin/idl/pack.idl (right):

http://codereview.chromium.org/150058/diff/2072/3064#newcode119
Line 119: \li o3d.Bitmap
indentation

[yux, 2009-07-13 18:04:05]

I'll refine algorithm for scale down and do drawimage on canvas in another cl
and I fixed everything else.

On 2009/07/10 22:25:04, gman wrote:
> It's looking good.
> 
> There's a few things. You need to change the non-const references to pointers.
> 
> Then there are 2 big issues but you can do those in another CL.
> 
> We also need canvas.drawImage(texture, ...) in another CL as well.
> 
> http://codereview.chromium.org/150058/diff/2072/3058
> File core/cross/bitmap.cc (right):
> 
> http://codereview.chromium.org/150058/diff/2072/3058#newcode48
> Line 48: #include <cstring>
> system includes always go first. The lint errors on this page 
> 
> http://codereview.chromium.org/150058
> 
> point this out
> 
> http://codereview.chromium.org/150058/diff/2072/3058#newcode302
> Line 302: O3D_ERROR(service_locator()) << "DrawImage does not support "
> This can be in another CL but it should be possible to draw RGB -> RGBA and
RGBA
> -> RGB
> 
> http://codereview.chromium.org/150058/diff/2072/3058#newcode346
> Line 346: void Bitmap::BilinearInterpolateScale(const uint8* src_img_data,
> This is my fault again. BilinearInterpolation only works for scaling up.
> 
> Using this function, if you wanted to make mips you'd have to do this
> 
> var b1 = pack.createBitmap(
>    originalBitmap.width, originalBitmap.height, ...);
> b1.drawImage(originalBitmap, 0, 0, 
>              originalBitmap.width, originalBitmap.height,
>              0, 0, b1.width, b1.height);
> var b2 = pack.createBitmap(b1.width / 2, b1.height / 2, ..);
> b2.drawImage(b1, 0, 0, 
>              b1.width, b1.height,
>              0, 0, b2.width, b2.height);
> var b3 = pack.createBitmap(b2.width / 2, b2.height / 2, ..);
> b3.drawImage(b2, 0, 0, 
>              b2.width, b2.height,
>              0, 0, b3.width, b3.height);
> etc..
> var t = pack.createTexture2D(
>    originalBitmap.width, originalBitmap.height, ...);
> t.drawImage(originalBitmap, ...., 0);
> t.drawImage(b1, ...., 1);
> t.drawImage(b2, ...., 2);
> t.drawImage(b3, ...., 3);
> pack.removeObject(originalBitmap);
> pack.removeObject(b1);
> pack.removeObject(b2);
> pack.removeObject(b3);
> 
> Where as what we want to be able to do this this.
> 
> var t = pack.createTexture2D(
>    originalBitmap.width, originalBitmap.height, ...);
> t.drawImage(originalBitmap, ...., 0);
> t.drawImage(originalBitmap, ...., 1);
> t.drawImage(originalBitmap, ...., 2);
> t.drawImage(originalBitmap, ...., 3);
> pack.removeObject(originalBitmap);
> 
> We can fix this in another CL.
> 
> http://codereview.chromium.org/150058/diff/2072/3054
> File core/cross/bitmap.h (right):
> 
> http://codereview.chromium.org/150058/diff/2072/3054#newcode394
> Line 394: static bool AdjustDrawImageBoundary(int& src_x, int& src_y,
> non-const references are not allowed by the google style guide.
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Reference_Arguments#R...
> 
> http://codereview.chromium.org/150058/diff/2072/3054#newcode429
> Line 429: static bool AdjustDrawImageBoundHelper(int& src_a, int& dest_a,
> non-const references not allowed.
> 
> http://codereview.chromium.org/150058/diff/2072/3064
> File plugin/idl/pack.idl (right):
> 
> http://codereview.chromium.org/150058/diff/2072/3064#newcode119
> Line 119: \li o3d.Bitmap
> indentation

[gman, 2009-07-13 22:21:34]

Almost there.

Just a couple of things.

http://codereview.chromium.org/150058/diff/2086/3090
File core/cross/bitmap.cc (right):

http://codereview.chromium.org/150058/diff/2086/3090#newcode55
Line 55: #define EPSILON 0.0001
#1

constants use kCamelCase format

#2

Macros are discouraged.

So, make it 

namespace {

static const double kEpsilon = 0.0001;

}  // anonymous namespace.

http://codereview.chromium.org/150058/diff/2086/3090#newcode824
Line 824: *dest_length = *dest_length - (*dest_length) * src_length_delta /
(*src_length);
> 80 chars

http://codereview.chromium.org/150058/diff/2086/3090#newcode832
Line 832: *dest_length = *dest_length - (*dest_length) * src_length_delta /
*src_length;
> 80 chars

http://codereview.chromium.org/150058/diff/2086/3090#newcode843
Line 843: *dest_length = *dest_length - (*dest_length) * src_length_delta /
(*src_length);
> 80 chars

http://codereview.chromium.org/150058/diff/2086/3090#newcode849
Line 849: *dest_length = *dest_length + (*dest_length) * src_length_delta /
(*src_length);
> 80 chars

http://codereview.chromium.org/150058/diff/2086/3098
File samples/bitmap-draw-image.html (right):

http://codereview.chromium.org/150058/diff/2086/3098#newcode1
Line 1: <!--
Several lines in this file are > 80 chars. Please fix.

[yux, 2009-07-13 23:05:04]

Fixed.
Thanks.
---
Xiao

On 2009/07/13 22:21:34, gman wrote:
> Almost there.
> 
> Just a couple of things.
> 
> http://codereview.chromium.org/150058/diff/2086/3090
> File core/cross/bitmap.cc (right):
> 
> http://codereview.chromium.org/150058/diff/2086/3090#newcode55
> Line 55: #define EPSILON 0.0001
> #1
> 
> constants use kCamelCase format
> 
> #2
> 
> Macros are discouraged.
> 
> So, make it 
> 
> namespace {
> 
> static const double kEpsilon = 0.0001;
> 
> }  // anonymous namespace.
> 
> http://codereview.chromium.org/150058/diff/2086/3090#newcode824
> Line 824: *dest_length = *dest_length - (*dest_length) * src_length_delta /
> (*src_length);
> > 80 chars
> 
> http://codereview.chromium.org/150058/diff/2086/3090#newcode832
> Line 832: *dest_length = *dest_length - (*dest_length) * src_length_delta /
> *src_length;
> > 80 chars
> 
> http://codereview.chromium.org/150058/diff/2086/3090#newcode843
> Line 843: *dest_length = *dest_length - (*dest_length) * src_length_delta /
> (*src_length);
> > 80 chars
> 
> http://codereview.chromium.org/150058/diff/2086/3090#newcode849
> Line 849: *dest_length = *dest_length + (*dest_length) * src_length_delta /
> (*src_length);
> > 80 chars
> 
> http://codereview.chromium.org/150058/diff/2086/3098
> File samples/bitmap-draw-image.html (right):
> 
> http://codereview.chromium.org/150058/diff/2086/3098#newcode1
> Line 1: <!--
> Several lines in this file are > 80 chars. Please fix.

[gman, 2009-07-14 20:25:31]

Okay, there's a few more things you need to do then LGTM

1) You need to add samples/bitmap-draw-image.html to
tests/selenium/sample_list.txt and to samples/build.scons

2) You need to add samples/assets/bmp_mip.o3dtgz to samples/MANIFEST

3) You need to then run selenium (scons MODE=test-opt-d3d selenium) to generate
a screenshot.

4) You need to copy that screenshot from
scons-out/test-opt-d3d/artifacts/selenium/screenshots_firefox/bitmap-draw-image1.png
to o3d_assets/tests/screeshots/reference/bitmap-draw-image1_reference.png (note
the name change)

5) You need to cd to o3d_assets and do a "gcl change screenshot" and add the
reference to that CL.  Then you need to submit that CL and make note of the
revision number.

6) You need to edit o3d/DEPS and o3d/DEPS_gyp and edit the o3d_asset revision
number at the top to match.

7) Finally you can check it in this CL.

[gman, 2009-07-14 20:26:37]

One more thing. Before you check in you need to do an

scons MODE=docs

and make sure you don't get any errors.