Adding Wayland support for ui/gfx

Adding Wayland support for ui/gfx

* Added GL surface and context support for Wayland.
* Updated ui/gfx files to allow Wayland support
* All Wayland code is behind the use_wayland gyp flag

This CL depends on http://codereview.chromium.org/7457023

BUG=
TEST=Compiled Chrome with use_wayland disabled to verify that
the changes didn't break anything


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

[jonathan.backer, 2011-07-26 14:56:26]

I have a pretty good handle of the GL changes. I'm not so sure of the gtk/pango
stuff.

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/canvas_skia_linux.cc
File ui/gfx/canvas_skia_linux.cc (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/canvas_skia_linux.cc#...
ui/gfx/canvas_skia_linux.cc:42: GtkSettings* gtk_settings =
gtk_settings_get_default();
move this down to where it's used and save an #if block

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/egl_util.cc
File ui/gfx/gl/egl_util.cc (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/egl_util.cc#newcode9
ui/gfx/gl/egl_util.cc:9: #else
Could we update the angle headers instead?

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl.gyp
File ui/gfx/gl/gl.gyp (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl.gyp#newcode133
ui/gfx/gl/gl.gyp:133: }],
My understanding is that setting chromium_code to 1 makes sure that win.cc files
are only compiled when OS==win (for example). Maybe you need something like that
for wayland. I think it's around line 913 in build/common.gypi

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_context_wayland.cc
File ui/gfx/gl/gl_context_wayland.cc (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_context_wayland...
ui/gfx/gl/gl_context_wayland.cc:15: 
Do you need all of these includes? I doubt you need logging or gl_bindings (not
sure about stubs).

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_implementation_...
File ui/gfx/gl/gl_implementation_wayland.cc (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_implementation_...
ui/gfx/gl/gl_implementation_wayland.cc:4: 
This seems very similar to gl_implementation_linux. I think your code will be
more robust to future changes if you #ifdef gl_implementation_linux rather than
introduce a separate file.

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_surface_egl.cc
File ui/gfx/gl/gl_surface_egl.cc (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_surface_egl.cc#...
ui/gfx/gl/gl_surface_egl.cc:22: #if defined(USE_X11) && !defined(USE_WAYLAND)
Is it consistent to have USE_X11 and USE_WAYLAND defined? They seem exclusive of
each other.

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gtk_util.cc
File ui/gfx/gtk_util.cc (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gtk_util.cc#newcode173
ui/gfx/gtk_util.cc:173: PangoContext* default_context =
pango_font_map_create_context(font_map);
why is this necessary?

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:106: // TODO This should be replaced with a cairo
region or maybe
TODO almost always has an LDAP (e.g. TODO(dnicoara))

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:168: #elif defined(USE_WAYLAND)
Isn't this exactly the same as above? (e.g. roll it in)

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/surface/surface.gyp
File ui/gfx/surface/surface.gyp (right):

http://codereview.chromium.org/7467007/diff/4001/ui/gfx/surface/surface.gyp#n...
ui/gfx/surface/surface.gyp:55: ['include', 'transport_dib_linux.cc'],
why is it necessary to explicitly include this. doesn't wayland==1 imply that
OS=="linux"?

[dnicoara, 2011-07-26 20:03:56]

I'll look into updating the angle header files.

On 2011/07/26 14:56:26, jonathan.backer wrote:
> I have a pretty good handle of the GL changes. I'm not so sure of the
gtk/pango
> stuff.
> 

I've looked into the gtk calls and re-added the calls. I've added the pango
calls as #ifdefs for Wayland. gtk essentially does the same calls, but it also
adds some extra properties. So, for now, the gtk calls are needed.

> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/canvas_skia_linux.cc
> File ui/gfx/canvas_skia_linux.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/canvas_skia_linux.cc#...
> ui/gfx/canvas_skia_linux.cc:42: GtkSettings* gtk_settings =
> gtk_settings_get_default();
> move this down to where it's used and save an #if block
> 

Done

> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/egl_util.cc
> File ui/gfx/gl/egl_util.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/egl_util.cc#newcode9
> ui/gfx/gl/egl_util.cc:9: #else
> Could we update the angle headers instead?
> 

Looking into it. It would definitely make everything look better.

> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl.gyp
> File ui/gfx/gl/gl.gyp (right):
> 
> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl.gyp#newcode133
> ui/gfx/gl/gl.gyp:133: }],
> My understanding is that setting chromium_code to 1 makes sure that win.cc
files
> are only compiled when OS==win (for example). Maybe you need something like
that
> for wayland. I think it's around line 913 in build/common.gypi
> 

Right. Thanks for mentioning it. I've moved that stuff in common.gypi

>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_context_wayland.cc
> File ui/gfx/gl/gl_context_wayland.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_context_wayland...
> ui/gfx/gl/gl_context_wayland.cc:15: 
> Do you need all of these includes? I doubt you need logging or gl_bindings
(not
> sure about stubs).
> 

Removed in favor of #ifdefs in gl_context_linux.

>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_implementation_...
> File ui/gfx/gl/gl_implementation_wayland.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_implementation_...
> ui/gfx/gl/gl_implementation_wayland.cc:4: 
> This seems very similar to gl_implementation_linux. I think your code will be
> more robust to future changes if you #ifdef gl_implementation_linux rather
than
> introduce a separate file.
> 

Right, removed and added #ifdefs in gl_implementation_linux.cc

> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_surface_egl.cc
> File ui/gfx/gl/gl_surface_egl.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gl/gl_surface_egl.cc#...
> ui/gfx/gl/gl_surface_egl.cc:22: #if defined(USE_X11) && !defined(USE_WAYLAND)
> Is it consistent to have USE_X11 and USE_WAYLAND defined? They seem exclusive
of
> each other.
> 

Right, normally USE_X11 would be undefined when USE_WAYLAND is defined. However
removing USE_X11, TOOLKIT_USES_GTK and OS_POSIX are used interchangeably. Which
makes it hard to remove any of them. I think this should be solved incrementally
later on.

> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gtk_util.cc
> File ui/gfx/gtk_util.cc (right):
> 
> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/gtk_util.cc#newcode173
> ui/gfx/gtk_util.cc:173: PangoContext* default_context =
> pango_font_map_create_context(font_map);
> why is this necessary?
> 

Yep, required to get the resolution.

> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/native_widget_types.h
> File ui/gfx/native_widget_types.h (right):
> 
>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/native_widget_types.h...
> ui/gfx/native_widget_types.h:106: // TODO This should be replaced with a cairo
> region or maybe
> TODO almost always has an LDAP (e.g. TODO(dnicoara))
> 

Done

>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/native_widget_types.h...
> ui/gfx/native_widget_types.h:168: #elif defined(USE_WAYLAND)
> Isn't this exactly the same as above? (e.g. roll it in)
> 

Done

> http://codereview.chromium.org/7467007/diff/4001/ui/gfx/surface/surface.gyp
> File ui/gfx/surface/surface.gyp (right):
> 
>
http://codereview.chromium.org/7467007/diff/4001/ui/gfx/surface/surface.gyp#n...
> ui/gfx/surface/surface.gyp:55: ['include', 'transport_dib_linux.cc'],
> why is it necessary to explicitly include this. doesn't wayland==1 imply that
> OS=="linux"?

Yes, but linux sometimes implies X and sometimes GTK in Chrome code.

[jonathan.backer, 2011-07-27 12:36:02]

LGTM

Made one general comment (does not need to be addressed here). Still need OWNERS
approval.

http://codereview.chromium.org/7467007/diff/10038/ui/gfx/surface/surface.gyp
File ui/gfx/surface/surface.gyp (right):

http://codereview.chromium.org/7467007/diff/10038/ui/gfx/surface/surface.gyp#...
ui/gfx/surface/surface.gyp:48: ],
not required here, but is there a suffix that would be mutually exclusive of
wayland (say _x11) so that you could rename the files and use a rule based
exclusion?

[sky, 2011-07-27 15:51:05]

Mostly just style nits.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/egl_util.cc
File ui/gfx/gl/egl_util.cc (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/egl_util.cc#newcode7
ui/gfx/gl/egl_util.cc:7: #if defined(USE_WAYLAND)
Condition includes should be after other includes.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/gl.gyp
File ui/gfx/gl/gl.gyp (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/gl.gyp#newcode61
ui/gfx/gl/gl.gyp:61: 'gl_surface_wayland.cc',
Does this need to be excluded if wayland isn't defined?

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/gl_surface_egl.cc
File ui/gfx/gl/gl_surface_egl.cc (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/gl_surface_egl.cc...
ui/gfx/gl/gl_surface_egl.cc:10: #if defined(USE_WAYLAND)
conditional includes after non-conditional includes.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/platform_font_gtk.cc
File ui/gfx/platform_font_gtk.cc (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/platform_font_gtk.cc...
ui/gfx/platform_font_gtk.cc:9: #if !defined(USE_WAYLAND)
conditionals after non-conditionals.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/platform_font_gtk.cc...
ui/gfx/platform_font_gtk.cc:63: PangoFontMap *font_map =
pango_cairo_font_map_get_default();
This looks to be the same as in gtk_util. Create a function in gtk_util that
both places use and has the ifdef.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/platform_font_gtk.cc...
ui/gfx/platform_font_gtk.cc:131: #if defined(USE_WAYLAND)
This code is a bit hard to read. Create a function that returns the name, as a
std::string, so you don't need the free around 150.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/screen_wayland.cc
File ui/gfx/screen_wayland.cc (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/screen_wayland.cc#ne...
ui/gfx/screen_wayland.cc:24: 
That's a lot of empty space. Also, how about just:

return screens.empty() ? gfx::Rect : ...

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/screen_wayland.cc#ne...
ui/gfx/screen_wayland.cc:34: // TODO(beng): use |view|.
Remove beng here.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_wayland.cc (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_wayland.cc:11: #include
"ui/gfx/gl/gl_surface_egl.h"
sort

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_wayland.cc:16: : size_(size) {
member initialize all non-object fields (meaning image_, pixmap_ and texture_.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_wayland.h (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_wayland.h:20: struct wl_egl_pixmap* pixmap()
const { return pixmap_; }
Do you really need struct here and on line 28?

Do you really want to return a raw modifiable pointer here? Wouldn't a const
wl_egl_pixmap& be less error prone? At a minimum, document ownership.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_wayland.h:31: friend class
base::RefCounted<AcceleratedSurface>;
friends are generally first in the private section.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/surface.gyp
File ui/gfx/surface/surface.gyp (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/surface.gyp#...
ui/gfx/surface/surface.gyp:34: 'accelerated_surface_wayland.cc',
Don't you need to exclude these everywhere unless wayland is defined?

http://codereview.chromium.org/7467007/diff/13003/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7467007/diff/13003/ui/ui.gyp#newcode328
ui/ui.gyp:328: ['include', 'base/dragdrop/os_exchange_data_provider_gtk.cc'],
sort these

[dnicoara, 2011-07-27 18:59:08]

On 2011/07/27 15:51:05, sky wrote:
> Mostly just style nits.
> 
> http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/egl_util.cc
> File ui/gfx/gl/egl_util.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/egl_util.cc#newcode7
> ui/gfx/gl/egl_util.cc:7: #if defined(USE_WAYLAND)
> Condition includes should be after other includes.
> 

Can't due to compile time errors if EGL headers aren't included first. I placed
a comment to indicate that.

> http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/gl.gyp
> File ui/gfx/gl/gl.gyp (right):
> 
> http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/gl.gyp#newcode61
> ui/gfx/gl/gl.gyp:61: 'gl_surface_wayland.cc',
> Does this need to be excluded if wayland isn't defined?
> 

Not in here. I added some changes to common.gypi that will exclude _wayland
files if use_wayland isn't defined.

> http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/gl_surface_egl.cc
> File ui/gfx/gl/gl_surface_egl.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/gl_surface_egl.cc...
> ui/gfx/gl/gl_surface_egl.cc:10: #if defined(USE_WAYLAND)
> conditional includes after non-conditional includes.
> 

Done

> http://codereview.chromium.org/7467007/diff/13003/ui/gfx/platform_font_gtk.cc
> File ui/gfx/platform_font_gtk.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/platform_font_gtk.cc...
> ui/gfx/platform_font_gtk.cc:9: #if !defined(USE_WAYLAND)
> conditionals after non-conditionals.
> 

Done

>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/platform_font_gtk.cc...
> ui/gfx/platform_font_gtk.cc:63: PangoFontMap *font_map =
> pango_cairo_font_map_get_default();
> This looks to be the same as in gtk_util. Create a function in gtk_util that
> both places use and has the ifdef.
> 

Done

>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/platform_font_gtk.cc...
> ui/gfx/platform_font_gtk.cc:131: #if defined(USE_WAYLAND)
> This code is a bit hard to read. Create a function that returns the name, as a
> std::string, so you don't need the free around 150.
> 

Done

> http://codereview.chromium.org/7467007/diff/13003/ui/gfx/screen_wayland.cc
> File ui/gfx/screen_wayland.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/screen_wayland.cc#ne...
> ui/gfx/screen_wayland.cc:24: 
> That's a lot of empty space. Also, how about just:
> 
> return screens.empty() ? gfx::Rect : ...
> 

Done

>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/screen_wayland.cc#ne...
> ui/gfx/screen_wayland.cc:34: // TODO(beng): use |view|.
> Remove beng here.
> 

Done

>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
> File ui/gfx/surface/accelerated_surface_wayland.cc (right):
> 
>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
> ui/gfx/surface/accelerated_surface_wayland.cc:11: #include
> "ui/gfx/gl/gl_surface_egl.h"
> sort
> 

Done

>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
> ui/gfx/surface/accelerated_surface_wayland.cc:16: : size_(size) {
> member initialize all non-object fields (meaning image_, pixmap_ and texture_.
> 
>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
> File ui/gfx/surface/accelerated_surface_wayland.h (right):
> 
>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
> ui/gfx/surface/accelerated_surface_wayland.h:20: struct wl_egl_pixmap*
pixmap()
> const { return pixmap_; }
> Do you really need struct here and on line 28?
> 
> Do you really want to return a raw modifiable pointer here? Wouldn't a const
> wl_egl_pixmap& be less error prone? At a minimum, document ownership.
> 

Right, there's no need for struct. I've documented ownership, but I think we may
need the pointer. 

>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/accelerated_...
> ui/gfx/surface/accelerated_surface_wayland.h:31: friend class
> base::RefCounted<AcceleratedSurface>;
> friends are generally first in the private section.
> 

Done

> http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/surface.gyp
> File ui/gfx/surface/surface.gyp (right):
> 
>
http://codereview.chromium.org/7467007/diff/13003/ui/gfx/surface/surface.gyp#...
> ui/gfx/surface/surface.gyp:34: 'accelerated_surface_wayland.cc',
> Don't you need to exclude these everywhere unless wayland is defined?
> 

Yes, common.gypi has the exclude rules.

> http://codereview.chromium.org/7467007/diff/13003/ui/ui.gyp
> File ui/ui.gyp (right):
> 
> http://codereview.chromium.org/7467007/diff/13003/ui/ui.gyp#newcode328
> ui/ui.gyp:328: ['include', 'base/dragdrop/os_exchange_data_provider_gtk.cc'],
> sort these

Done

[sky, 2011-07-27 22:22:22]

LGTM with the following changes.

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/egl_util.cc
File ui/gfx/gl/egl_util.cc (right):

http://codereview.chromium.org/7467007/diff/13003/ui/gfx/gl/egl_util.cc#newcode7
ui/gfx/gl/egl_util.cc:7: #if defined(USE_WAYLAND)
On 2011/07/27 15:51:05, sky wrote:
> Condition includes should be after other includes.

In that case you should have the condition system includes after the main
header. The general layout is:

include header_of_file
system includes
conditional system includes
non-system includes
conditional non-system-includes

With newline between each.

http://codereview.chromium.org/7467007/diff/23001/ui/gfx/platform_font_gtk.cc
File ui/gfx/platform_font_gtk.cc (right):

http://codereview.chromium.org/7467007/diff/23001/ui/gfx/platform_font_gtk.cc...
ui/gfx/platform_font_gtk.cc:118: std::string default_font("sans 10");
simplify this to:
#if defined(WAYLAND)
  return "sans 10";
#else
....
#endif

[commit-bot: I haz the power, 2011-08-10 16:50:46]

Change committed as 96192