GTK: Move CairoCachedSurface from being owned by GtkThemeService to gfx::Image.

GTK: Move CairoCachedSurface from being owned by GtkThemeService to gfx::Image.

CairoCachedSurfaces are representations of a GdkPixbuf that live on the display
server instead of in process. GtkThemeService currently returns a
CairoCachedSurface for a (IDR#, Display). Instead, make gfx::Image keep
CairoCachedSurfaces as a representation and make CairoCachedSurface work on any
display, keeping the mapping to display server resources as an implementation
detail.

This has the benefit of:

- You don't need to go through GtkThemeService to access all resources,
  simplifying the code in some places and allowing sharing of static images
  across profiles.
- This will let us (in the future) remove several image conversions in the GTK
  port.

This is part 1. Part 2 will remove the custom CairoCachedSurface calls from
GtkThemeService.

BUG=106060
TEST=none
R=rsesek@chromium.org
TBR=ben@chromium.org

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

[Elliot Glaysher, 2011-12-01 19:03:38]

rsesek: gfx::Image additions
estade: touchups to ui/gtk/ callsites. (More coming to move lots of stuff off
GtkThemeService, move GdkGC users to CairoCachedSurface, etc.)

[Robert Sesek, 2011-12-01 19:30:35]

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc#newcode47
ui/gfx/image/image.cc:47: class ImageRepCairoCached;
nit: put below ImageRepGdk

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc#newcode165
ui/gfx/image/image.cc:165: 
nit: remove blank line

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc#newcode166
ui/gfx/image/image.cc:166: #endif
  // defined(TOOLKIT_USES_GTK)

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc#newcode374
ui/gfx/image/image.cc:374: }
What about going from Cairo to Skia?

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.h#newcode105
ui/gfx/image/image.h:105: GdkPixbuf* CopyGdkPixbuf() const;
Are there copy semantics for CairoCachedSurface?

[Elliot Glaysher, 2011-12-01 20:30:11]

nits addressed

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc#newcode374
ui/gfx/image/image.cc:374: }
On 2011/12/01 19:30:35, rsesek wrote:
> What about going from Cairo to Skia?

We shouldn't allow this; it doesn't make sense. 

We'll never form a CairoCachedSurface unless we already have a GdkPixbuf (since
the cached surface takes a GdkPixbuf and sends the data to the display server
(X11, Wayland, etc)). And we already handle GdkPixbuf to Skia.

(We can actually can read data off the display server, but  removing round trips
is why I'm adding this patch.)

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.h#newcode105
ui/gfx/image/image.h:105: GdkPixbuf* CopyGdkPixbuf() const;
On 2011/12/01 19:30:35, rsesek wrote:
> Are there copy semantics for CairoCachedSurface?

There could be (there isn't now), but I don't think it would make sense.

So you have an object that's purpose is to reference data in another process.
You can't mutate that data. You could tell the display server to duplicate the
data, but I don't see why you would.

[Robert Sesek, 2011-12-01 20:56:14]

gfx/image/ lgtm

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.cc#newcode374
ui/gfx/image/image.cc:374: }
On 2011/12/01 20:30:11, Elliot Glaysher wrote:
> On 2011/12/01 19:30:35, rsesek wrote:
> > What about going from Cairo to Skia?
> 
> We shouldn't allow this; it doesn't make sense. 
> 
> We'll never form a CairoCachedSurface unless we already have a GdkPixbuf
(since
> the cached surface takes a GdkPixbuf and sends the data to the display server
> (X11, Wayland, etc)). And we already handle GdkPixbuf to Skia.
> 
> (We can actually can read data off the display server, but  removing round
trips
> is why I'm adding this patch.)

Can you add a comment here to that effect so that it appears deliberate?

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/8769017/diff/1/ui/gfx/image/image.h#newcode105
ui/gfx/image/image.h:105: GdkPixbuf* CopyGdkPixbuf() const;
On 2011/12/01 20:30:11, Elliot Glaysher wrote:
> On 2011/12/01 19:30:35, rsesek wrote:
> > Are there copy semantics for CairoCachedSurface?
> 
> There could be (there isn't now), but I don't think it would make sense.
> 
> So you have an object that's purpose is to reference data in another process.
> You can't mutate that data. You could tell the display server to duplicate the
> data, but I don't see why you would.

Sounds fair.

[Elliot Glaysher, 2011-12-02 01:21:03]

rsesek: PTAL. I had to make some minor changes to Image because what I was doing
was wrong in practice.

estade: ping for the gtk stuff

[Evan Stade, 2011-12-02 02:30:52]

lgtm

[Robert Sesek, 2011-12-02 16:36:16]

http://codereview.chromium.org/8769017/diff/2005/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/8769017/diff/2005/ui/gfx/image/image.cc#newcod...
ui/gfx/image/image.cc:390: else if (rep_type == Image::kImageRepCairoCache) {
// Handle to-Cairo conversion. This may recursively create an intermediate
pixbuf.

[Robert Sesek, 2011-12-02 16:36:22]

lgtm

[commit-bot: I haz the power, 2011-12-02 19:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/8769017/52

[commit-bot: I haz the power, 2011-12-02 19:05:27]

Presubmit check for 8769017-52 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
ui/gfx/image/image.h,ui/gfx/image/image_unittest.cc,ui/ui.gyp,ui/gfx/image/cairo_cached_surface.h,ui/gfx/image/cairo_cached_surface.cc,ui/gfx/image/image.cc

Presubmit checks took 2.6s to calculate.

[Elliot Glaysher, 2011-12-02 19:14:51]

tbr: ben (rsesek wrote all the code in that directory and I have his lgtm stamp;
with his permission, I'm going to create an OWNERs file for him for that
directory)

[commit-bot: I haz the power, 2011-12-02 19:15:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/8769017/52

[commit-bot: I haz the power, 2011-12-02 20:54:21]

Change committed as 112778