Introduce PlatformBitmap, which is a minimal helper class that wraps an SkBitmap

Introduce PlatformBitmap, which is a minimal helper class that wraps an SkBitmap
and a PlatformSurface. This is used to replace the PlatformCanvas that was being
passed to BackingStore to return pixels.

The problem to solve is that PlatformCanvas is an extension of SkCanvas, and
SkCanvas is losing the ability to have its backend specified after its
constructor (for performance reasons).

The BackingStore interface only needs to return a copy of its pixels, and offer
a platform-specific way to draw into it (i.e. BitBlt). The PlatformSurface is
sufficient for this, so the larger infrastructure of PlatformCanvas/PlatformDevice
is not required.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=161163

[reed1, 2012-10-04 20:14:58]

update linux render_widget_host

[reed1, 2012-10-08 18:15:03]

Brett, ping me in chat if you need some guidance on reviewing this.

I think the core of the change is two-fold:

1. adding PlatformBitmap to platform_canvas.h
2. changing BackingStore::CopyFromBackingStore() to take PlatformBitmap instead
of PlatformCanvas

The rest mostly is just fallout.

[brettw, 2012-10-09 20:40:12]

I don't totally get the Lock/Unlock stuff. None of the unlock implementations do
anything, and many of the Lock calls don't do anything either. Does this give us
anything over just always locking in the Allocate function?

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_android.cc (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_android.cc:79: bool PlatformBitmap::Allocate(int
width, int height, bool isOpaque) {
is_opaque. Same for other ports.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_linux.cc (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_linux.cc:187: width, height);
Check indenting.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_linux.cc:208: // nothing to do for our surface.
It will be released in our destructor
Check indenting.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_mac.cc (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_mac.cc:19: #include "skia/ext/platform_canvas.h"
This should go in the above block in order.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:39: HBITMAP hbitmap =
CreateDIBSection(NULL, reinterpret_cast<BITMAPINFO*>(&hdr), 0,
Line too long.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:278: bool PlatformBitmap::Allocate(int
width, int height, bool isOpaque) {
is_opaque.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:298: // nothing to do for our surface_.
It will be released in our destructor
Style nit: should be a complete sentence with cap & period. Same for other
platforms.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:155: public:
nit: indenting for these two classes (2 space indents, public/private should be
indented one space).

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:156: PlatformBitmap() : surface_(0) {}
Can you put this in the .cc file? (Background: Chrome style requires this for
all nontrivial classes since the compiler ends up putting one in every .o file
that uses it, then the linker uniquifies it, and it slows things down. Since
this class has a SkBitmap and a PlatformSurface nested class, this would be
counted as a nontrivial constructor/destructor.)

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:159: bool Allocate(int width, int height, bool
isOpaque);
style: is_opaque.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:161: PlatformSurface LockSurface();
Can these have documentation? Be sure to mention what happens if you don't call
Allocate().

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:167: SkBitmap        bitmap_;
Normally we don't try to horizontally align these things, so just one space is
fine.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:169: };
Should these classes be DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:173: explicit ScopedPlatformBitmap(PlatformBitmap*
bitmap) : bitmap_(bitmap) {
Can you also put this constructor & destructor in the .cc?

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:183: };
Probably also DISALLOW_COPY_AND_ASSIGN here.

[reed1, 2012-10-09 21:06:42]

Good comments. I think I can replace Lock/Unlock with just Get, and I can remove
the need for the ScopedPlatformBitmap dude.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_linux.cc (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_linux.cc:187: width, height);
On 2012/10/09 20:40:12, brettw wrote:
> Check indenting.

Done.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_linux.cc:208: // nothing to do for our surface.
It will be released in our destructor
On 2012/10/09 20:40:12, brettw wrote:
> Check indenting.

Done.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_mac.cc (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_mac.cc:19: #include "skia/ext/platform_canvas.h"
On 2012/10/09 20:40:12, brettw wrote:
> This should go in the above block in order.

Done.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:39: HBITMAP hbitmap =
CreateDIBSection(NULL, reinterpret_cast<BITMAPINFO*>(&hdr), 0,
On 2012/10/09 20:40:12, brettw wrote:
> Line too long.

Done.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:155: public:
On 2012/10/09 20:40:12, brettw wrote:
> nit: indenting for these two classes (2 space indents, public/private should
be
> indented one space).

Done.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:156: PlatformBitmap() : surface_(0) {}
On 2012/10/09 20:40:12, brettw wrote:
> Can you put this in the .cc file? (Background: Chrome style requires this for
> all nontrivial classes since the compiler ends up putting one in every .o file
> that uses it, then the linker uniquifies it, and it slows things down. Since
> this class has a SkBitmap and a PlatformSurface nested class, this would be
> counted as a nontrivial constructor/destructor.)

Done.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:161: PlatformSurface LockSurface();
On 2012/10/09 20:40:12, brettw wrote:
> Can these have documentation? Be sure to mention what happens if you don't
call
> Allocate().

Done.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:167: SkBitmap        bitmap_;
On 2012/10/09 20:40:12, brettw wrote:
> Normally we don't try to horizontally align these things, so just one space is
> fine.

Done.

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:169: };
On 2012/10/09 20:40:12, brettw wrote:
> Should these classes be DISALLOW_COPY_AND_ASSIGN?

That is waht SkNoncopyable does. I have updated PlatformBitmap to also inherit
from it. (SkCanvas also inherits from SkNoncopyable).

[brettw, 2012-10-09 21:08:22]

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:169: };
If you're doing Chrome code I'd prefer the Chrome way of specifying this.

[reed1, 2012-10-09 21:30:19]

On 2012/10/09 21:08:22, brettw wrote:
> https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas.h
> File skia/ext/platform_canvas.h (right):
> 
>
https://codereview.chromium.org/11031055/diff/27009/skia/ext/platform_canvas....
> skia/ext/platform_canvas.h:169: };
> If you're doing Chrome code I'd prefer the Chrome way of specifying this.

Done

[reed1, 2012-10-09 21:39:37]

I had started with Lock/Unlock semantics, just trying to be defensive. As you
noticed, none of the ports ever needed that so that is now removed (replaced
with GetSurface()).

Once that pair was gone, there is no need for ScopedPlatformBitmap, so that is
gone too.

PTAL

(I've only compiled on mac w/ this version, so I will run trys in the morning)

[brettw, 2012-10-09 21:59:17]

https://codereview.chromium.org/11031055/diff/24019/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11031055/diff/24019/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:157: PlatformBitmap();
I think you lost the implementation of this function or you forgot to add
platform_canvas.cc to the CL.

[reed1, 2012-10-10 11:50:58]

On 2012/10/09 21:59:17, brettw wrote:
> https://codereview.chromium.org/11031055/diff/24019/skia/ext/platform_canvas.h
> File skia/ext/platform_canvas.h (right):
> 
>
https://codereview.chromium.org/11031055/diff/24019/skia/ext/platform_canvas....
> skia/ext/platform_canvas.h:157: PlatformBitmap();
> I think you lost the implementation of this function or you forgot to add
> platform_canvas.cc to the CL.

Yep, forgot to upload platform_canvas.cc

[reed1, 2012-10-10 17:47:27]

PTAL

I will try to run the ppapi tests locally (I never have before), so I don't know
if that time-out is related to this change or not.

[brettw, 2012-10-10 19:41:02]

lgtm