BackingStoreSkia: Use skia for painting in RenderWidgetHostViewViews.

BackingStoreSkia: Use skia for painting in RenderWidgetHostViewViews.

Instead of directly painting to X, paint to the skia canvas from RWHVV. This
will make it easier to apply transformation to views in a uniform way, without
requiring any special-casing for RWHVV.

BUG=none
TEST=none

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

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

[sadrul, 2011-02-05 05:45:04]

Please add additional reviewers as you see fit.

[sky, 2011-02-07 17:12:58]

agl is likely a good fit to review this.

  -Scott

[agl, 2011-02-07 17:26:47]

The code seems reasonable, although I have no idea what its purpose is. (Maybe
the description needs a pointer?)

(I certainly hope that you aren't intending to replace the X backend with this.)

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:37: static const
char* kBackingStoreSkiaSwitch = "use-backing-store-skia";
use |char kBackingStoreSkiaSwitch[]| to save a relocation record (and change the
line above, too).

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:347: if
(UsingBackingStoreSkia())
the style guide lets you do this, although I would ask that you consider using
braces in such a large conditional.

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:445:
NOTIMPLEMENTED();
NOTREACHED()?

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:621: if
(!is_showing_context_menu_ && !is_hidden_ && GetRenderWidgetHost())
This seems like it doesn't belong. If it's fixing a bug, then it probably should
live in its own CL.

[sadrul, 2011-02-07 18:22:38]

Addressed the comments. Thanks for the review.

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:37: static const
char* kBackingStoreSkiaSwitch = "use-backing-store-skia";
On 2011/02/07 17:26:47, agl wrote:
> use |char kBackingStoreSkiaSwitch[]| to save a relocation record (and change
the
> line above, too).

Done.

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:347: if
(UsingBackingStoreSkia())
On 2011/02/07 17:26:47, agl wrote:
> the style guide lets you do this, although I would ask that you consider using
> braces in such a large conditional.

Done.

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:445:
NOTIMPLEMENTED();
On 2011/02/07 17:26:47, agl wrote:
> NOTREACHED()?

It's not clear to me when |visually_deemphasized_| is set. But when it is set,
there is currently no implementation for skia. I think NOTIMPLEMENTED is
appropriate?

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:621: if
(!is_showing_context_menu_ && !is_hidden_ && GetRenderWidgetHost())
On 2011/02/07 17:26:47, agl wrote:
> This seems like it doesn't belong. If it's fixing a bug, then it probably
should
> live in its own CL.

Indeed. This is a fix for a bug, and sneaked into this. I will file it
separately.

[agl, 2011-02-07 18:28:38]

LGTM

[sadrul, 2011-02-07 18:34:45]

On 2011/02/07 17:26:47, agl wrote:
> The code seems reasonable, although I have no idea what its purpose is. (Maybe
> the description needs a pointer?)
> 
> (I certainly hope that you aren't intending to replace the X backend with
this.)

The X backing store will still be used for GTK+ (RenderWidgetHostViewGtk), of
course. For views (RenderWidgetHostViewViews), the skia backing store will be
used, for the moment, to allow transformations on it (e.g. rotation, translation
etc.). Ultimately, the goal is that RWHVV will use GPU rendering instead of the
X or the skia backing store.

[rjkroege, 2011-02-07 19:36:40]

LGTM with nits.

Please emphasize the temporary nature of this code: the expectation is to use
the GL back end as soon as it's ready.

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/ba...
File chrome/browser/renderer_host/backing_store_skia.cc (right):

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/ba...
chrome/browser/renderer_host/backing_store_skia.cc:38: return size().GetArea() *
4;
you are making a presumption about the way that bitmaps are stored that is not
necessarily valid. e.g.: if the SkCanvas is a sub-rectangle of a larger bitmap,
this will not be true.

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/ba...
chrome/browser/renderer_host/backing_store_skia.cc:69: SkBitmap b;
This looks like a double copy of the bitmap. If so, it makes me sad. Is there a
way to eliminate the double copy? Yes, I know this is interim. But...

[sadrul, 2011-02-07 20:55:38]

I added a comment about temporary status in the header, and made a change to
avoid double copy. Please take another look.

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/ba...
File chrome/browser/renderer_host/backing_store_skia.cc (right):

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/ba...
chrome/browser/renderer_host/backing_store_skia.cc:38: return size().GetArea() *
4;
On 2011/02/07 19:36:40, rjkroege wrote:
> you are making a presumption about the way that bitmaps are stored that is not
> necessarily valid. e.g.: if the SkCanvas is a sub-rectangle of a larger
bitmap,
> this will not be true.

I wasn't exactly sure what to do where. The SkBitmap in the backing store is of
size size() and uses 32 bits per pixels. Both the X and the windows backing
store seem to do something similar. So I went with it as well.

http://codereview.chromium.org/6246129/diff/1/chrome/browser/renderer_host/ba...
chrome/browser/renderer_host/backing_store_skia.cc:69: SkBitmap b;
On 2011/02/07 19:36:40, rjkroege wrote:
> This looks like a double copy of the bitmap. If so, it makes me sad. Is there
a
> way to eliminate the double copy? Yes, I know this is interim. But...

I changed this to use drawBitmapRect instead of readPixels/writePixels. I think
this rids of double copy?

[rjkroege, 2011-02-07 21:03:24]

LGTM.

http://codereview.chromium.org/6246129/diff/2003/chrome/browser/renderer_host...
File chrome/browser/renderer_host/backing_store_skia.cc (right):

http://codereview.chromium.org/6246129/diff/2003/chrome/browser/renderer_host...
chrome/browser/renderer_host/backing_store_skia.cc:39: return size().GetArea() *
4;
My whining about this doesn't apply until you permit passing in the bitmap
object so it's fine for now. I'm just worrying about some future where we wanted
to make the bitmap providable. 

You'd satisfy me with a comment here.

[sadrul, 2011-02-07 21:31:51]

http://codereview.chromium.org/6246129/diff/2003/chrome/browser/renderer_host...
File chrome/browser/renderer_host/backing_store_skia.cc (right):

http://codereview.chromium.org/6246129/diff/2003/chrome/browser/renderer_host...
chrome/browser/renderer_host/backing_store_skia.cc:39: return size().GetArea() *
4;
On 2011/02/07 21:03:24, rjkroege wrote:
> My whining about this doesn't apply until you permit passing in the bitmap
> object so it's fine for now. I'm just worrying about some future where we
wanted
> to make the bitmap providable. 
> 
> You'd satisfy me with a comment here.

Done.