Adding a Wayland basic toolkit

Adding a Wayland basic toolkit

This is essentially a OO wrapper over the Wayland library. It will be used to
add Wayland support for Chrome.

This was written with the intent of being as standalone as possible and it 
should not require any external Chrome dependencies.

BUG=None
TEST=None

R=evan@chromium.org

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

[Evan Martin, 2011-07-21 16:37:56]

Here are some high-level comments (mostly style stuff):

- Every class, function, and member variable needs docs describing what it does.
 You don't have to doc every last argument to the function, but someone who
opens one of these files shouldn't need to guess or Google to figure out what a
given thing is supposed to do.  For member variables, one valuable doc comment
is whether this object owns a pointer or not.

- Use C++-style comments, C++ style type decls.

- Forward-declare types rather than including headers whenever possible.

- Spamming stderr when problems occur is antisocial.  If you really want this
code to be separate from Chrome, then I suggest checking it into third_party;
otherwise, you should use probably use our logging macros.

My experience from trying to write code as "separate from Chrome" is that it is
a wasted effort unless you are willing to both fight for it and argue that it's
worth rewriting bits of code that we already have, but you're welcome to try it
again -- maybe this is small enough that it doesn't matter.

http://codereview.chromium.org/7457023/diff/1/ui/wayland/events/wayland_event.h
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/1/ui/wayland/events/wayland_event...
ui/wayland/events/wayland_event.h:24: SCROLL_DOWN     = 276,
Is this not in some wayland header somewhere?

http://codereview.chromium.org/7457023/diff/1/ui/wayland/wayland_buffer.h
File ui/wayland/wayland_buffer.h (right):

http://codereview.chromium.org/7457023/diff/1/ui/wayland/wayland_buffer.h#new...
ui/wayland/wayland_buffer.h:13: struct wl_buffer* GetBuffer() const;
Typically we forward declare these if possible.

I.e. at the top of the file:
struct wl_buffer;

and then use wl_buffer without "struct" everywhere else.

[dnicoara, 2011-07-22 18:00:46]

Overall I went over all the code and added more comments. Hopefully it makes it
easier to read and understand the code now.

On 2011/07/21 16:37:56, Evan Martin wrote:
> Here are some high-level comments (mostly style stuff):
> 
> - Every class, function, and member variable needs docs describing what it
does.
>  You don't have to doc every last argument to the function, but someone who
> opens one of these files shouldn't need to guess or Google to figure out what
a
> given thing is supposed to do.  For member variables, one valuable doc comment
> is whether this object owns a pointer or not.
> 
> - Use C++-style comments, C++ style type decls.
> 
> - Forward-declare types rather than including headers whenever possible.
> 

Sure, I went over the code and the Chrome style guide and formatted and added
more comments.

> - Spamming stderr when problems occur is antisocial.  If you really want this
> code to be separate from Chrome, then I suggest checking it into third_party;
> otherwise, you should use probably use our logging macros.
> 

You're right. I've replaced them with LOG calls.

> My experience from trying to write code as "separate from Chrome" is that it
is
> a wasted effort unless you are willing to both fight for it and argue that
it's
> worth rewriting bits of code that we already have, but you're welcome to try
it
> again -- maybe this is small enough that it doesn't matter.
> 
>
http://codereview.chromium.org/7457023/diff/1/ui/wayland/events/wayland_event.h
> File ui/wayland/events/wayland_event.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/1/ui/wayland/events/wayland_event...
> ui/wayland/events/wayland_event.h:24: SCROLL_DOWN     = 276,
> Is this not in some wayland header somewhere?
> 

Nope, these aren't defined in Wayland. I did update them to use the evdev
defined constants. And, in anticipation, I am aware that the right and middle
buttons are inverted. I tested on multiple types of mice and this mapping is as
expected.

> http://codereview.chromium.org/7457023/diff/1/ui/wayland/wayland_buffer.h
> File ui/wayland/wayland_buffer.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/1/ui/wayland/wayland_buffer.h#new...
> ui/wayland/wayland_buffer.h:13: struct wl_buffer* GetBuffer() const;
> Typically we forward declare these if possible.
> 
> I.e. at the top of the file:
> struct wl_buffer;
> 
> and then use wl_buffer without "struct" everywhere else.

Yes, I agree, I must have missed some. I went over the files and I think I
updated all of them.

[Evan Martin, 2011-07-22 18:21:22]

I'm really really sorry to nitpick every last bit of whitespace, but this kind
of nitpicking happens to everyone new to the codebase and is one of the reasons
our code looks like it was all written by one person.

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
ui/wayland/events/wayland_event.h:5: #ifndef WAYLAND_EVENT_H_
UI_WAYLAND_EVENTS_WAYLAND_EVENT_H_
(here and everywhere else)

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
ui/wayland/events/wayland_event.h:13: send.
Is this at the protocol level?  Or is it wrapping the decoded result of the
protocol?

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
ui/wayland/events/wayland_event.h:17: The time of the event. This should be
monotonically increasing.
What are the units? (Is this defined in some other doc?)
Should this use the Time object found in base?  (That one has an API that makes
the units explicit.)  Or is it opaque?

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
ui/wayland/events/wayland_event.h:51: /* WaylandEventButtonType defines some of
the values button can take */
C++-style comments, please.  Sorry to nag.

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp
File ui/wayland/wayland.gyp (right):

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp#newcode1
ui/wayland/wayland.gyp:1: {
Missing copyright block (copy it from another gyp file)

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp#newcode5
ui/wayland/wayland.gyp:5: 'pkg-config': '../../build/linux/pkg-config-wrapper
"<(sysroot)"',
Hm, is this not inherited from somewhere else?
...hmm.  I will ask about it separately.

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp#newco...
ui/wayland/wayland.gyp:14: 'target_name': 'libwayland',
'wayland', not 'libwayland'.  (the 'lib' is implicit from the type; in fact it's
inserted into the resulting .a file)

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp#newco...
ui/wayland/wayland.gyp:18: '../..',
This I think is implicit from our build system.

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_buffer.cc
File ui/wayland/wayland_buffer.cc (right):

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_buffer.cc...
ui/wayland/wayland_buffer.cc:5: #include "wayland_buffer.h"
This should be
ui/wayland/wayland_buffer.h

(and the gyp paths adjusted accordingly)

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.cc
File ui/wayland/wayland_cursor.cc (right):

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.cc...
ui/wayland/wayland_cursor.cc:13: struct PointerImage {
Use an anon namespace around all the file-local definitions, like this and the
pointer images

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.cc...
ui/wayland/wayland_cursor.cc:19: static const struct PointerImage
pointer_images[] = {
kPointerImages

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.h
File ui/wayland/wayland_cursor.h (right):

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.h#...
ui/wayland/wayland_cursor.h:8: #include "base/basictypes.h"
Is this needed?

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_display.cc
File ui/wayland/wayland_display.cc (right):

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_display.c...
ui/wayland/wayland_display.cc:54: }
Chrome style is for single-line "if" statements to not have curlies.  (If the
'if' condition is multiline, or if the body of the if is multline, then braces
are required.)

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_display.h
File ui/wayland/wayland_display.h (right):

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_display.h...
ui/wayland/wayland_display.h:29: of that connection.
Is a singleton necessary?  In general they make code harder to test and make it
easy to break abstraction boundaries, etc.

[dnicoara, 2011-07-22 20:45:26]

On 2011/07/22 18:21:22, Evan Martin wrote:
> I'm really really sorry to nitpick every last bit of whitespace, but this kind
> of nitpicking happens to everyone new to the codebase and is one of the
reasons
> our code looks like it was all written by one person.
> 

No problem, I'm expecting nitpicking, especially in this review.

>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
> File ui/wayland/events/wayland_event.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
> ui/wayland/events/wayland_event.h:5: #ifndef WAYLAND_EVENT_H_
> UI_WAYLAND_EVENTS_WAYLAND_EVENT_H_
> (here and everywhere else)
> 

Done

>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
> ui/wayland/events/wayland_event.h:13: send.
> Is this at the protocol level?  Or is it wrapping the decoded result of the
> protocol?
> 

Wayland provides the event details as parameters to the callbacks. Wayland*Event
just wraps them to something easier to use and something closer to the Chrome
model of an event.

I've updated the comments in the file to reflect that.

>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
> ui/wayland/events/wayland_event.h:17: The time of the event. This should be
> monotonically increasing.
> What are the units? (Is this defined in some other doc?)
> Should this use the Time object found in base?  (That one has an API that
makes
> the units explicit.)  Or is it opaque?
> 

So, I haven't traced through the Wayland code to be 100% certain. From what I've
seen, it doesn't really have a use in an application. I've only seen limited use
for it and it was mostly when making server calls. Though I've been reading on
it and the monotonic property is something they really want, which is really the
reason I mentioned it. I thought it might be useful in the future when looking
at a series of events and we need to keep track of timing.

I'd leave it as is for now since there is no use case for it.

>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_ev...
> ui/wayland/events/wayland_event.h:51: /* WaylandEventButtonType defines some
of
> the values button can take */
> C++-style comments, please.  Sorry to nag.
> 

Done, hopefully I didn't miss anything.

> http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp
> File ui/wayland/wayland.gyp (right):
> 
>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp#newcode1
> ui/wayland/wayland.gyp:1: {
> Missing copyright block (copy it from another gyp file)
> 

Done

>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp#newcode5
> ui/wayland/wayland.gyp:5: 'pkg-config': '../../build/linux/pkg-config-wrapper
> "<(sysroot)"',
> Hm, is this not inherited from somewhere else?
> ...hmm.  I will ask about it separately.
> 

I think I needed it when I wrote the .gyp file.

>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp#newco...
> ui/wayland/wayland.gyp:14: 'target_name': 'libwayland',
> 'wayland', not 'libwayland'.  (the 'lib' is implicit from the type; in fact
it's
> inserted into the resulting .a file)
> 

Done

>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland.gyp#newco...
> ui/wayland/wayland.gyp:18: '../..',
> This I think is implicit from our build system.
> 

Nope, I actually tried it without it at first. I needed to add it.

> http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_buffer.cc
> File ui/wayland/wayland_buffer.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_buffer.cc...
> ui/wayland/wayland_buffer.cc:5: #include "wayland_buffer.h"
> This should be
> ui/wayland/wayland_buffer.h
> 
> (and the gyp paths adjusted accordingly)
> 

Done

> http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.cc
> File ui/wayland/wayland_cursor.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.cc...
> ui/wayland/wayland_cursor.cc:13: struct PointerImage {
> Use an anon namespace around all the file-local definitions, like this and the
> pointer images
> 
>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.cc...
> ui/wayland/wayland_cursor.cc:19: static const struct PointerImage
> pointer_images[] = {
> kPointerImages
> 

Done

> http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.h
> File ui/wayland/wayland_cursor.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_cursor.h#...
> ui/wayland/wayland_cursor.h:8: #include "base/basictypes.h"
> Is this needed?
> 

Yes, DISALLOW_COPY_AND_ASSIGN is defined in basictypes.h

> http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_display.cc
> File ui/wayland/wayland_display.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_display.c...
> ui/wayland/wayland_display.cc:54: }
> Chrome style is for single-line "if" statements to not have curlies.  (If the
> 'if' condition is multiline, or if the body of the if is multline, then braces
> are required.)
> 

Done

> http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_display.h
> File ui/wayland/wayland_display.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/4001/ui/wayland/wayland_display.h...
> ui/wayland/wayland_display.h:29: of that connection.
> Is a singleton necessary?  In general they make code harder to test and make
it
> easy to break abstraction boundaries, etc.

I agree, I initially couldn't find a way to associate a WaylandDisplay to a
wl_display which made it necessary for the singleton. But since I found a way :)

[Evan Martin, 2011-07-22 21:08:54]

This is starting to look really good.

Do you happen to have a pointer to some wayland docs?  I'd like to review some
of the actual code you've written more carefully but I'm not too familiar with
the wayland API.

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/events/wayland_ev...
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/events/wayland_ev...
ui/wayland/events/wayland_event.h:11: // Wayland event information is being
passed in as arguments to the callbacks.
"is being passed in as arguments to the callbacks"
-- which callbacks? where is it being passed in?

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/events/wayland_ev...
ui/wayland/events/wayland_event.h:123: #endif
#endif  // UI_WAYLAND_EVENTS_WAYLAND_EVENT_H_

here and everywhere.  (i usually copy the top ifdef, change to endif, two
spaces, //)

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland.gyp
File ui/wayland/wayland.gyp (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland.gyp#newco...
ui/wayland/wayland.gyp:54: 'USE_SYSTEM_LIBWAYLAND',
Does anyone use this?

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_cursor.cc
File ui/wayland/wayland_cursor.cc (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_cursor.cc...
ui/wayland/wayland_cursor.cc:26: }
Write this one as

}  // namespace

(which conveys it's closing the anonymous namespace)

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.cc
File ui/wayland/wayland_display.cc (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.c...
ui/wayland/wayland_display.cc:24: wl_display_set_user_data(display->display_,
display);
Cool, this is a nice way to handle it.

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.c...
ui/wayland/wayland_display.cc:148: // need one.
Should we do some sort of NOTREACHED() / NOTIMPLEMENTED() in the other cases? 
Or will the compositor expose all of these in sequence and we just want to grab
the one we care about?

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.h
File ui/wayland/wayland_display.h (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.h...
ui/wayland/wayland_display.h:49: wl_shm* GetShm() const;
For simple getters, we inline them and change their casing to convey to callers
that they are cost-free.

E.g.
  wl_shm* shm() const { return shm_; }

(Why have a getter at all, rather than just making the member public?  Because
you can control *writing* to this member.)

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.h...
ui/wayland/wayland_display.h:63: // This handle resolves all server events used
in initialization. It also
Is this a typo (handle => handler)?  If not, I don't understand the comment.  :)

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_geometry_...
File ui/wayland/wayland_geometry_utils.h (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_geometry_...
ui/wayland/wayland_geometry_utils.h:6: struct Point {
I wonder if it's worth reusing our gfx::Rect and gfx::Point classes here.

If not, I wonder if these need to be in a namespace -- I wouldn't be surprised
if there was some other code in this enormous program that decides to create a
"Point" class.

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
File ui/wayland/wayland_input_device.cc (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
ui/wayland/wayland_input_device.cc:169: k++) {
I wonder if this loop would be easier to write as somethign like

uint32_t* codes = static_cast<uint32_t*>(keys->data);
int codes_size = keys->size / sizeof(uint32_t);
for (int i = 0; i < codes_size; ++i) {
  uint32_t code = codes[i] + xkb->min_key_code;
}

What do you think?

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
ui/wayland/wayland_input_device.cc:192:
WaylandInputDevice::input_device_listener_ = {
Maybe this should just be a const static within the ctor?  It's only used from
within that function.

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
File ui/wayland/wayland_input_device.h (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
ui/wayland/wayland_input_device.h:51: WaylandWindow* keyboard_focus_;
Does this mean that pointer and keyboard focus can be different from each otehr?

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_shm_buffe...
File ui/wayland/wayland_shm_buffer.cc (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_shm_buffe...
ui/wayland/wayland_shm_buffer.cc:26: LOG(ERROR) << "Failed to open";
You can use PLOG instead of LOG for situations where printing strerror(errno)
makes sense.  (It does for most libc functions, but not for e.g. dlopen)

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_widget.h
File ui/wayland/wayland_widget.h (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_widget.h#...
ui/wayland/wayland_widget.h:15: virtual void OnMotionNotify(WaylandEvent event)
= 0;
Should these be passed the more specific event type?
It seems the user will always just reach into the union...

[dnicoara, 2011-07-22 22:17:31]

I was actually thinking of having all this Wayland code under a wayland
namespace. Maybe ui::wayland. What do you think?

On 2011/07/22 21:08:54, Evan Martin wrote:
> This is starting to look really good.
> 
> Do you happen to have a pointer to some wayland docs?  I'd like to review some
> of the actual code you've written more carefully but I'm not too familiar with
> the wayland API.
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/events/wayland_ev...
> File ui/wayland/events/wayland_event.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/events/wayland_ev...
> ui/wayland/events/wayland_event.h:11: // Wayland event information is being
> passed in as arguments to the callbacks.
> "is being passed in as arguments to the callbacks"
> -- which callbacks? where is it being passed in?
> 

Updated with more information and pointed it to wayland_input_device for more
clarification.

>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/events/wayland_ev...
> ui/wayland/events/wayland_event.h:123: #endif
> #endif  // UI_WAYLAND_EVENTS_WAYLAND_EVENT_H_
> 
> here and everywhere.  (i usually copy the top ifdef, change to endif, two
> spaces, //)
> 

Done

> http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland.gyp
> File ui/wayland/wayland.gyp (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland.gyp#newco...
> ui/wayland/wayland.gyp:54: 'USE_SYSTEM_LIBWAYLAND',
> Does anyone use this?
> 

Right, not needed anymore. Removed.

> http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_cursor.cc
> File ui/wayland/wayland_cursor.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_cursor.cc...
> ui/wayland/wayland_cursor.cc:26: }
> Write this one as
> 
> }  // namespace
> 
> (which conveys it's closing the anonymous namespace)
> 

Done

> http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.cc
> File ui/wayland/wayland_display.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.c...
> ui/wayland/wayland_display.cc:24: wl_display_set_user_data(display->display_,
> display);
> Cool, this is a nice way to handle it.
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.c...
> ui/wayland/wayland_display.cc:148: // need one.
> Should we do some sort of NOTREACHED() / NOTIMPLEMENTED() in the other cases? 
> Or will the compositor expose all of these in sequence and we just want to
grab
> the one we care about?
> 

The compositor will expose all of them in sequence. We just want one we care
about.

> http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.h
> File ui/wayland/wayland_display.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.h...
> ui/wayland/wayland_display.h:49: wl_shm* GetShm() const;
> For simple getters, we inline them and change their casing to convey to
callers
> that they are cost-free.
> 
> E.g.
>   wl_shm* shm() const { return shm_; }
> 
> (Why have a getter at all, rather than just making the member public?  Because
> you can control *writing* to this member.)
> 

I don't want to make it public to control writing. I also didn't want to
optimize to soon since that might need to be wrapped in to an object if we need
extra functionality.

Some may even have OO wrappers at some point (mostly thinking of wl_shell). But
that mostly depends on the functionality we need.

I'm tempted to leave it as is for now.

>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.h...
> ui/wayland/wayland_display.h:63: // This handle resolves all server events
used
> in initialization. It also
> Is this a typo (handle => handler)?  If not, I don't understand the comment. 
:)
> 

Typo.

>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_geometry_...
> File ui/wayland/wayland_geometry_utils.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_geometry_...
> ui/wayland/wayland_geometry_utils.h:6: struct Point {
> I wonder if it's worth reusing our gfx::Rect and gfx::Point classes here.
> 
> If not, I wonder if these need to be in a namespace -- I wouldn't be surprised
> if there was some other code in this enormous program that decides to create a
> "Point" class.
> 

The Point struct is used internally but I have multiple classes using it so I
wanted to share its definition.

>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
> File ui/wayland/wayland_input_device.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
> ui/wayland/wayland_input_device.cc:169: k++) {
> I wonder if this loop would be easier to write as somethign like
> 
> uint32_t* codes = static_cast<uint32_t*>(keys->data);
> int codes_size = keys->size / sizeof(uint32_t);
> for (int i = 0; i < codes_size; ++i) {
>   uint32_t code = codes[i] + xkb->min_key_code;
> }
> 
> What do you think?
> 

Yes, that looks much cleaner.

>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
> ui/wayland/wayland_input_device.cc:192:
> WaylandInputDevice::input_device_listener_ = {
> Maybe this should just be a const static within the ctor?  It's only used from
> within that function.
> 

Done

>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
> File ui/wayland/wayland_input_device.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_dev...
> ui/wayland/wayland_input_device.h:51: WaylandWindow* keyboard_focus_;
> Does this mean that pointer and keyboard focus can be different from each
otehr?
> 

My understanding is yes, they can. I think it's similar to how Linux allows a
user to scroll in one window while typing in another.

>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_shm_buffe...
> File ui/wayland/wayland_shm_buffer.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_shm_buffe...
> ui/wayland/wayland_shm_buffer.cc:26: LOG(ERROR) << "Failed to open";
> You can use PLOG instead of LOG for situations where printing strerror(errno)
> makes sense.  (It does for most libc functions, but not for e.g. dlopen)
> 

Thanks, done.

> http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_widget.h
> File ui/wayland/wayland_widget.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_widget.h#...
> ui/wayland/wayland_widget.h:15: virtual void OnMotionNotify(WaylandEvent
event)
> = 0;
> Should these be passed the more specific event type?
> It seems the user will always just reach into the union...

Yes, but that would make it harder later on, since the Event class defined in
views expects a NativeEvent (in our case a WaylandEvent). So it would make life
harder in other places since we'd need to transform from a specific Wayland
event to a WaylandEvent object.

[Evan Martin, 2011-07-22 22:41:45]

This LGTM, I just have some minor remaining nits you can fix at your lesiure

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_buffer.cc
File ui/wayland/wayland_buffer.cc (right):

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_buffer.cc...
ui/wayland/wayland_buffer.cc:13: struct wl_buffer* WaylandBuffer::GetBuffer()
const {
I think you can delete 'struct' here

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_cursor.h
File ui/wayland/wayland_cursor.h (right):

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_cursor.h#...
ui/wayland/wayland_cursor.h:38: #endif // UI_WAYLAND_WAYLAND_CURSOR_H_
Two spaces before every "//".  (You can probably grep for something like "[^ ]
//" to find all instances)

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_input_dev...
File ui/wayland/wayland_input_device.cc (right):

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_input_dev...
ui/wayland/wayland_input_device.cc:21: const struct wl_input_device_listener
input_device_listener_ = {
kInputDeviceListener, now that it's no longer a field

[Evan Martin, 2011-07-22 22:42:11]

Oh and regarding the namespace, we can keep it in mind and do it if it becomes
necessary

[tfarina, 2011-07-22 22:59:30]

On 2011/07/22 22:42:11, Evan Martin wrote:
> Oh and regarding the namespace, we can keep it in mind and do it if it becomes
> necessary

I'd say it's better to get at least a namespace ui through all these files
before check in (it doesn't hurt) and prevents more work on future.

[tfarina, 2011-07-22 23:06:45]

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_cursor.h
File ui/wayland/wayland_cursor.h (right):

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_cursor.h#...
ui/wayland/wayland_cursor.h:17: enum WaylandCursorType {
I think just Type is enough.

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_display.h
File ui/wayland/wayland_display.h (right):

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_display.h...
ui/wayland/wayland_display.h:48: wl_shell* GetShell() const;
add blank lines between those methods.

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_display.h...
ui/wayland/wayland_display.h:51: private:
add a blank line above to separate public and private sections. Applies to other
files as well.

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_display.h...
ui/wayland/wayland_display.h:55: wl_display* display_;
member variables should be at the end of the private section.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

[dnicoara, 2011-07-25 15:58:50]

I've added the ui namespace and fixed the mentioned issues.

[Evan Martin, 2011-07-25 16:16:01]

LGTM+=

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
ui/wayland/events/wayland_event.h:13: // events are processed.
Missing )

[Evan Martin, 2011-07-25 16:16:09]

On 2011/07/25 16:16:01, Evan Martin wrote:
> LGTM+=

LGTM++ rather

> 
>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
> File ui/wayland/events/wayland_event.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
> ui/wayland/events/wayland_event.h:13: // events are processed.
> Missing )

[tfarina, 2011-07-25 16:27:13]

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
ui/wayland/events/wayland_event.h:32: typedef enum {
No need of typedef in C++ land! ;)

just:

enum WaylandEventType {...}; 

is fine.

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
ui/wayland/events/wayland_event.h:115: union _WaylandEvent {
Can this be just:

union WaylandEvent { ... };

?

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
ui/wayland/events/wayland_event.h:125: typedef _WaylandEvent WaylandEvent;
Why?

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.cc
File ui/wayland/wayland_cursor.cc (right):

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.c...
ui/wayland/wayland_cursor.cc:20: // TODO(dnicoara) Add more pointer images and
fix the path
end with period.

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.c...
ui/wayland/wayland_cursor.cc:21: static const struct PointerImage
kPointerImages[] = {
please, remove this static, since this is already in the unnamed namespace.

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.c...
ui/wayland/wayland_cursor.cc:21: static const struct PointerImage
kPointerImages[] = {
You can write this as:

struct PointerImage {
...
} kPointerImage[] = {
...
};

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.c...
ui/wayland/wayland_cursor.cc:43: const struct PointerImage *ptr =
&kPointerImages[type];
const PointerImage& image = kPointerImages[type] ?

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
File ui/wayland/wayland_input_device.cc (right):

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
ui/wayland/wayland_input_device.cc:16:
WaylandInputDevice::WaylandInputDevice(wl_display* display,
display in the next line.

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
ui/wayland/wayland_input_device.cc:17: uint32_t id) :
input_device_(wl_input_device_create(display, id, 1)),
the : should be on the next line indented 4 spaces.

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
ui/wayland/wayland_input_device.cc:96: void
WaylandInputDevice::OnKeyNotify(void* data,
void* data on the next line. And one arg per line.

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
ui/wayland/wayland_input_device.cc:162: void
WaylandInputDevice::OnKeyboardFocus(void* data,
One argument per line is more readable. And also void* data should be in the
next line.

[dnicoara, 2011-07-25 18:03:43]

On 2011/07/25 16:27:13, tfarina wrote:
>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
> File ui/wayland/events/wayland_event.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
> ui/wayland/events/wayland_event.h:32: typedef enum {
> No need of typedef in C++ land! ;)
> 
> just:
> 
> enum WaylandEventType {...}; 
> 
> is fine.
> 

Done

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
> ui/wayland/events/wayland_event.h:115: union _WaylandEvent {
> Can this be just:
> 
> union WaylandEvent { ... };
> 
> ?
> 

Done

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_e...
> ui/wayland/events/wayland_event.h:125: typedef _WaylandEvent WaylandEvent;
> Why?
> 
> http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.cc
> File ui/wayland/wayland_cursor.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.c...
> ui/wayland/wayland_cursor.cc:20: // TODO(dnicoara) Add more pointer images and
> fix the path
> end with period.
> 

Done

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.c...
> ui/wayland/wayland_cursor.cc:21: static const struct PointerImage
> kPointerImages[] = {
> please, remove this static, since this is already in the unnamed namespace.
> 

Done

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.c...
> ui/wayland/wayland_cursor.cc:21: static const struct PointerImage
> kPointerImages[] = {
> You can write this as:
> 
> struct PointerImage {
> ...
> } kPointerImage[] = {
> ...
> };
> 

I prefer to have the declaration separately since there are comments that refer
to the object. Plus I think it looks better.

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.c...
> ui/wayland/wayland_cursor.cc:43: const struct PointerImage *ptr =
> &kPointerImages[type];
> const PointerImage& image = kPointerImages[type] ?
> 

Done

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
> File ui/wayland/wayland_input_device.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
> ui/wayland/wayland_input_device.cc:16:
> WaylandInputDevice::WaylandInputDevice(wl_display* display,
> display in the next line.
> 

Done

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
> ui/wayland/wayland_input_device.cc:17: uint32_t id) :
> input_device_(wl_input_device_create(display, id, 1)),
> the : should be on the next line indented 4 spaces.
> 

Done

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
> ui/wayland/wayland_input_device.cc:96: void
> WaylandInputDevice::OnKeyNotify(void* data,
> void* data on the next line. And one arg per line.
> 

Done

>
http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_de...
> ui/wayland/wayland_input_device.cc:162: void
> WaylandInputDevice::OnKeyboardFocus(void* data,
> One argument per line is more readable. And also void* data should be in the
> next line.

Done

[tfarina, 2011-07-25 18:34:27]

More style nits below. It's almost OK to me though.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display.cc
File ui/wayland/wayland_display.cc (right):

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display....
ui/wayland/wayland_display.cc:68: it != screen_list_.end(); it++) {
++i

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display....
ui/wayland/wayland_display.cc:158: }
do you need a default case here?

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display.h
File ui/wayland/wayland_display.h (right):

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display....
ui/wayland/wayland_display.h:53: wl_shell* GetShell() const;
Same thing here for these simple getter accessors?

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display....
ui/wayland/wayland_display.h:69: // Used by the compositor initialization to
register the different visuals.
please, separate which method with a blank line between them.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.cc
File ui/wayland/wayland_screen.cc (right):

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.c...
ui/wayland/wayland_screen.cc:16: static const wl_output_listener kOutputListener
= {
This body should be indent just 2 spaces.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.c...
ui/wayland/wayland_screen.cc:34: // Find the active mode and pass its dimensions
period.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.c...
ui/wayland/wayland_screen.cc:35: for (Modes::const_iterator it = modes_.begin();
it != modes_.end(); it++) {
just |i| instead of |it| is fine.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.c...
ui/wayland/wayland_screen.cc:35: for (Modes::const_iterator it = modes_.begin();
it != modes_.end(); it++) {
++i

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.cc
File ui/wayland/wayland_window.cc (right):

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.c...
ui/wayland/wayland_window.cc:89: uint32_t time,
You can write this as:

void WaylandWindow::Configure(uint32_t time,
                              uint32_t edges,
                              ...) {
...
}

If it fits (and this is the preferred way too).

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h
File ui/wayland/wayland_window.h (right):

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
ui/wayland/wayland_window.h:25: // Creates a toplevel window
End with a period.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
ui/wayland/wayland_window.h:27: // Creates a transient window with an offset of
(x,y) from parent
period.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
ui/wayland/wayland_window.h:30: virtual ~WaylandWindow();
This needs to be virtual? This class doesn't have virtual methods.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
ui/wayland/wayland_window.h:42: WaylandWindow* GetParentWindow() const;
The way we prefer is:

WaylandWindow* parent_window() const { return parent_window_; }

Yes, inlined here in the header file and with unix_hacker style ;)

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
ui/wayland/wayland_window.h:44: WaylandWidget* GetWidget() const;
Same thing here:

WaylandWidget* widget() const { return widget_; }

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
ui/wayland/wayland_window.h:47: wl_surface* GetSurface() const;
And here.

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
ui/wayland/wayland_window.h:64: WaylandWindow* parent_window_;
Document this? I'd also move it near of widget_ and display_

[dnicoara, 2011-07-25 19:22:51]

Done. I've inlined a couple of comments bellow.

On 2011/07/25 18:34:27, tfarina wrote:
> More style nits below. It's almost OK to me though.
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display.cc
> File ui/wayland/wayland_display.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display....
> ui/wayland/wayland_display.cc:68: it != screen_list_.end(); it++) {
> ++i
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display....
> ui/wayland/wayland_display.cc:158: }
> do you need a default case here?
> 

Nope, the 3 values are the only values.

> http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display.h
> File ui/wayland/wayland_display.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display....
> ui/wayland/wayland_display.h:53: wl_shell* GetShell() const;
> Same thing here for these simple getter accessors?
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display....
> ui/wayland/wayland_display.h:69: // Used by the compositor initialization to
> register the different visuals.
> please, separate which method with a blank line between them.
> 
> http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.cc
> File ui/wayland/wayland_screen.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.c...
> ui/wayland/wayland_screen.cc:16: static const wl_output_listener
kOutputListener
> = {
> This body should be indent just 2 spaces.
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.c...
> ui/wayland/wayland_screen.cc:34: // Find the active mode and pass its
dimensions
> period.
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.c...
> ui/wayland/wayland_screen.cc:35: for (Modes::const_iterator it =
modes_.begin();
> it != modes_.end(); it++) {
> just |i| instead of |it| is fine.
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_screen.c...
> ui/wayland/wayland_screen.cc:35: for (Modes::const_iterator it =
modes_.begin();
> it != modes_.end(); it++) {
> ++i
> 
> http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.cc
> File ui/wayland/wayland_window.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.c...
> ui/wayland/wayland_window.cc:89: uint32_t time,
> You can write this as:
> 
> void WaylandWindow::Configure(uint32_t time,
>                               uint32_t edges,
>                               ...) {
> ...
> }
> 
> If it fits (and this is the preferred way too).
> 
> http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h
> File ui/wayland/wayland_window.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
> ui/wayland/wayland_window.h:25: // Creates a toplevel window
> End with a period.
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
> ui/wayland/wayland_window.h:27: // Creates a transient window with an offset
of
> (x,y) from parent
> period.
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
> ui/wayland/wayland_window.h:30: virtual ~WaylandWindow();
> This needs to be virtual? This class doesn't have virtual methods.
> 

Not anymore. At some point I thought I'd need to extend it.

>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
> ui/wayland/wayland_window.h:42: WaylandWindow* GetParentWindow() const;
> The way we prefer is:
> 
> WaylandWindow* parent_window() const { return parent_window_; }
> 
> Yes, inlined here in the header file and with unix_hacker style ;)
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
> ui/wayland/wayland_window.h:44: WaylandWidget* GetWidget() const;
> Same thing here:
> 
> WaylandWidget* widget() const { return widget_; }
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
> ui/wayland/wayland_window.h:47: wl_surface* GetSurface() const;
> And here.
> 
>
http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_window.h...
> ui/wayland/wayland_window.h:64: WaylandWindow* parent_window_;
> Document this? I'd also move it near of widget_ and display_

[tfarina, 2011-07-25 21:10:09]

http://codereview.chromium.org/7457023/diff/16003/ui/wayland/wayland_buffer.h
File ui/wayland/wayland_buffer.h (right):

http://codereview.chromium.org/7457023/diff/16003/ui/wayland/wayland_buffer.h...
ui/wayland/wayland_buffer.h:21: WaylandBuffer() {}
I think it's fine, but I prefer for ctor and dtor implement them on the source
file. Have you passed this through clang trybot?

See the section "Stop inlining constructors and destructors."

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

http://codereview.chromium.org/7457023/diff/16003/ui/wayland/wayland_buffer.h...
ui/wayland/wayland_buffer.h:24: wl_buffer* GetBuffer() const { return buffer_; }
Nope, I said the preferred way is:

wl_buffer* buffer() const { ... };

[tfarina, 2011-07-25 21:11:46]

http://codereview.chromium.org/7457023/diff/16003/ui/wayland/wayland_screen.cc
File ui/wayland/wayland_screen.cc (right):

http://codereview.chromium.org/7457023/diff/16003/ui/wayland/wayland_screen.c...
ui/wayland/wayland_screen.cc:16: static const wl_output_listener kOutputListener
= {
I mean, the whole body of this ctor should be indented less 2 spaces like the
methods below. Not just inside this object.

[dnicoara, 2011-07-26 13:14:15]

On 2011/07/25 21:10:09, tfarina wrote:
> http://codereview.chromium.org/7457023/diff/16003/ui/wayland/wayland_buffer.h
> File ui/wayland/wayland_buffer.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/16003/ui/wayland/wayland_buffer.h...
> ui/wayland/wayland_buffer.h:21: WaylandBuffer() {}
> I think it's fine, but I prefer for ctor and dtor implement them on the source
> file. Have you passed this through clang trybot?
> 
> See the section "Stop inlining constructors and destructors."
> 
> http://www.chromium.org/developers/coding-style/cpp-dos-and-donts
> 

Right, sorry, I got carried away.

>
http://codereview.chromium.org/7457023/diff/16003/ui/wayland/wayland_buffer.h...
> ui/wayland/wayland_buffer.h:24: wl_buffer* GetBuffer() const { return buffer_;
}
> Nope, I said the preferred way is:
> 
> wl_buffer* buffer() const { ... };

Done.

[tfarina, 2011-07-26 14:36:50]

It LG to me, minus the gyp stuff yet.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp
File ui/wayland/wayland.gyp (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
ui/wayland/wayland.gyp:9: 'pkg-config': '../../build/linux/pkg-config-wrapper
"<(sysroot)"',
Including 'chromium_code' variable instead, doesn't pull this pkg-config stuff
for you? I'd rather not duplicate this code here, since it's already implemented
else where.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
ui/wayland/wayland.gyp:17: 'targets': [{
the { should be on the next line right below the 'a' of 'targets'.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
ui/wayland/wayland.gyp:19: 'type': 'static_library',
Don't you need a 'dependencies': [ ../../base/base.gyp:base ], here?

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
ui/wayland/wayland.gyp:43: 'events/wayland_event.h',
this should be sort alphabetical, so it should be the first source above.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
ui/wayland/wayland.gyp:55: '<!@(<(pkg-config) --libs-only-L --libs-only-other
wayland-client wayland-egl xkbcommon)',
Not sure if we enforce 80 columns in .gyp files.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_buffer.h
File ui/wayland/wayland_buffer.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_buffer.h...
ui/wayland/wayland_buffer.h:28: wl_buffer* buffer_;
The style guide says no protected data members, they should be private, but
since you are using this in the subclass WaylandShmBuffer, I think it's fine
though.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_message_...
File ui/wayland/wayland_message_pump.cc (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_message_...
ui/wayland/wayland_message_pump.cc:39: while (mask_ & WL_DISPLAY_WRITABLE) {
no need of { } for single line statements.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_shm_buff...
File ui/wayland/wayland_shm_buffer.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_shm_buff...
ui/wayland/wayland_shm_buffer.h:22: // Creates a Wayland buffer with the given
width and height
period.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_shm_buff...
ui/wayland/wayland_shm_buffer.h:31: // The surface associated with the Wayland
buffer
period.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_window.h
File ui/wayland/wayland_window.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_window.h...
ui/wayland/wayland_window.h:66: // When creating a transient window,
parent_window_ is set to point to the
when referring to variables in comment, we prefer to wrap them in | | as:

// ..., |parent_window_| is set to point to ....

[tfarina, 2011-07-26 14:49:27]

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_cursor.h
File ui/wayland/wayland_cursor.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_cursor.h...
ui/wayland/wayland_cursor.h:24: WaylandCursor(WaylandDisplay* display);
please add a explicit here.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_message_...
File ui/wayland/wayland_message_pump.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_message_...
ui/wayland/wayland_message_pump.h:22: WaylandMessagePump(WaylandDisplay*
display);
please add a explicit here.

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_screen.h
File ui/wayland/wayland_screen.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_screen.h...
ui/wayland/wayland_screen.h:64: // The display that the output is associated
with
separating each member variable with blank lines between them makes this more
readable. ;)

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_widget.h
File ui/wayland/wayland_widget.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_widget.h...
ui/wayland/wayland_widget.h:13: class WaylandWidget {
I don't know, would be a better name WaylandObserver/WaylandListener?

[dnicoara, 2011-07-26 16:07:08]

On 2011/07/26 14:36:50, tfarina wrote:
> It LG to me, minus the gyp stuff yet.
> 
> http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp
> File ui/wayland/wayland.gyp (right):
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
> ui/wayland/wayland.gyp:9: 'pkg-config': '../../build/linux/pkg-config-wrapper
> "<(sysroot)"',
> Including 'chromium_code' variable instead, doesn't pull this pkg-config stuff
> for you? I'd rather not duplicate this code here, since it's already
implemented
> else where.
> 

'chromium_code' seems to have a different role. Anyways, the pkg-config is
defined in build/linux/system.gyp and based on my understanding there isn't be a
way to include system.gyp in order to have pkg-config defined. But I've asked
around and it seems that the consensus is to move these deps in system.gyp.

>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
> ui/wayland/wayland.gyp:17: 'targets': [{
> the { should be on the next line right below the 'a' of 'targets'.
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
> ui/wayland/wayland.gyp:19: 'type': 'static_library',
> Don't you need a 'dependencies': [ ../../base/base.gyp:base ], here?
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
> ui/wayland/wayland.gyp:43: 'events/wayland_event.h',
> this should be sort alphabetical, so it should be the first source above.
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland.gyp#newc...
> ui/wayland/wayland.gyp:55: '<!@(<(pkg-config) --libs-only-L --libs-only-other
> wayland-client wayland-egl xkbcommon)',
> Not sure if we enforce 80 columns in .gyp files.
> 

It seems that there is no 80 columns rule for .gyp

> http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_buffer.h
> File ui/wayland/wayland_buffer.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_buffer.h...
> ui/wayland/wayland_buffer.h:28: wl_buffer* buffer_;
> The style guide says no protected data members, they should be private, but
> since you are using this in the subclass WaylandShmBuffer, I think it's fine
> though.
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_message_...
> File ui/wayland/wayland_message_pump.cc (right):
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_message_...
> ui/wayland/wayland_message_pump.cc:39: while (mask_ & WL_DISPLAY_WRITABLE) {
> no need of { } for single line statements.
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_shm_buff...
> File ui/wayland/wayland_shm_buffer.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_shm_buff...
> ui/wayland/wayland_shm_buffer.h:22: // Creates a Wayland buffer with the given
> width and height
> period.
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_shm_buff...
> ui/wayland/wayland_shm_buffer.h:31: // The surface associated with the Wayland
> buffer
> period.
> 
> http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_window.h
> File ui/wayland/wayland_window.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_window.h...
> ui/wayland/wayland_window.h:66: // When creating a transient window,
> parent_window_ is set to point to the
> when referring to variables in comment, we prefer to wrap them in | | as:
> 
> // ..., |parent_window_| is set to point to ....

[tfarina, 2011-07-26 16:14:41]

The wayland.gyp now LGTM. Thanks.

http://codereview.chromium.org/7457023/diff/27003/build/linux/system.gyp
File build/linux/system.gyp (right):

http://codereview.chromium.org/7457023/diff/27003/build/linux/system.gyp#newc...
build/linux/system.gyp:472: '<!@(<(pkg-config) --cflags cairo wayland-client
wayland-egl xkbcommon)',
I think wayland-client and the others will be needed to be added to
build/install-build-deps.sh later.

http://codereview.chromium.org/7457023/diff/27003/ui/wayland/wayland.gyp
File ui/wayland/wayland.gyp (right):

http://codereview.chromium.org/7457023/diff/27003/ui/wayland/wayland.gyp#newc...
ui/wayland/wayland.gyp:34: 'wayland_message_pump.cc',
this should be right after input_device.h

[dnicoara, 2011-07-26 16:47:52]

On 2011/07/26 16:14:41, tfarina wrote:
> The wayland.gyp now LGTM. Thanks.
> 
> http://codereview.chromium.org/7457023/diff/27003/build/linux/system.gyp
> File build/linux/system.gyp (right):
> 
>
http://codereview.chromium.org/7457023/diff/27003/build/linux/system.gyp#newc...
> build/linux/system.gyp:472: '<!@(<(pkg-config) --cflags cairo wayland-client
> wayland-egl xkbcommon)',
> I think wayland-client and the others will be needed to be added to
> build/install-build-deps.sh later.

Right, currently some of these dependencies aren't in Ubuntu official repos
(wayland) and I'm using cairo from git since it has glesv2 support.

> 
> http://codereview.chromium.org/7457023/diff/27003/ui/wayland/wayland.gyp
> File ui/wayland/wayland.gyp (right):
> 
>
http://codereview.chromium.org/7457023/diff/27003/ui/wayland/wayland.gyp#newc...
> ui/wayland/wayland.gyp:34: 'wayland_message_pump.cc',
> this should be right after input_device.h

Done

[sky, 2011-07-27 15:56:11]

As Evan reviewed this I'm not going to review it all. I do have one question
though.

  -Scott

http://codereview.chromium.org/7457023/diff/30028/ui/wayland/events/wayland_e...
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/30028/ui/wayland/events/wayland_e...
ui/wayland/events/wayland_event.h:32: enum WaylandEventType {
This code starts looks like that in views/events/event.h. I'm wondering if we
should move the views event code into a place where wayland code could make use
of it instead of it?

[sky, 2011-07-27 16:06:02]

One more question. Is the goal to implement a NativeWidgetWayland on top of
these classes?

  -Scott

[dnicoara, 2011-07-27 17:30:23]

Regarding your event question, I'm not sure how much improvement that would
bring, in the current state. The views/events/event.* files are waiting for GTK
cleanup and it is obvious that from a Wayland and X perspective there is a lot
of code that needs to be removed in there. I'd see it useful if we were to
improve its state so that we can provide a Chromium Event much earlier. Perhaps
we could even get rid of NativeEvent all together. But then again, my experience
with Chromium code is pretty new so I might be missing the bigger picture.

On 2011/07/27 16:06:02, sky wrote:
> One more question. Is the goal to implement a NativeWidgetWayland on top of
> these classes?
> 
>   -Scott

Yes, actually, I have a CL for that at http://codereview.chromium.org/7464027

[sky, 2011-07-27 20:26:49]

On Wed, Jul 27, 2011 at 10:30 AM,  <dnicoara@chromium.org> wrote:
> Regarding your event question, I'm not sure how much improvement that would
> bring, in the current state. The views/events/event.* files are waiting for
> GTK
> cleanup and it is obvious that from a Wayland and X perspective there is a
> lot
> of code that needs to be removed in there.

I'm assuming if you're using Wayland you wouldn't be using views/event
with gtk events. Instead you would be creating the various views/event
classes directly (well, after we move them some where else).

With the approach you have here we end up creating the wayland events
than when you wire up the views side you'll need to convert them to
views/events.

  -Scott

>  I'd see it useful if we were to
> improve its state so that we can provide a Chromium Event much earlier.
> Perhaps
> we could even get rid of NativeEvent all together. But then again, my
> experience
> with Chromium code is pretty new so I might be missing the bigger picture.
>
> On 2011/07/27 16:06:02, sky wrote:
>>
>> One more question. Is the goal to implement a NativeWidgetWayland on top
>> of
>> these classes?
>
>>   -Scott
>
> Yes, actually, I have a CL for that at
> http://codereview.chromium.org/7464027
>
> http://codereview.chromium.org/7457023/
>

[Evan Martin, 2011-07-27 20:50:11]

On Wed, Jul 27, 2011 at 1:26 PM, Scott Violet <sky@chromium.org> wrote:
> On Wed, Jul 27, 2011 at 10:30 AM,  <dnicoara@chromium.org> wrote:
>> Regarding your event question, I'm not sure how much improvement that would
>> bring, in the current state. The views/events/event.* files are waiting for
>> GTK
>> cleanup and it is obvious that from a Wayland and X perspective there is a
>> lot
>> of code that needs to be removed in there.
>
> I'm assuming if you're using Wayland you wouldn't be using views/event
> with gtk events. Instead you would be creating the various views/event
> classes directly (well, after we move them some where else).
>
> With the approach you have here we end up creating the wayland events
> than when you wire up the views side you'll need to convert them to
> views/events.

When I had read it, it was my understanding that this intended to be a
layer below views.
(In the initial code review, the description included "could in theory
work outside of chrome".  Not that that's a design goal, but it does
seem useful from a modularity perspective.)

[dnicoara, 2011-07-27 21:11:17]

On 2011/07/27 20:50:11, Evan Martin wrote:
> On Wed, Jul 27, 2011 at 1:26 PM, Scott Violet <mailto:sky@chromium.org> wrote:
> > On Wed, Jul 27, 2011 at 10:30 AM, mailto: <dnicoara@chromium.org> wrote:
> >> Regarding your event question, I'm not sure how much improvement that would
> >> bring, in the current state. The views/events/event.* files are waiting for
> >> GTK
> >> cleanup and it is obvious that from a Wayland and X perspective there is a
> >> lot
> >> of code that needs to be removed in there.
> >
> > I'm assuming if you're using Wayland you wouldn't be using views/event
> > with gtk events. Instead you would be creating the various views/event
> > classes directly (well, after we move them some where else).
> >
> > With the approach you have here we end up creating the wayland events
> > than when you wire up the views side you'll need to convert them to
> > views/events.
> 
> When I had read it, it was my understanding that this intended to be a
> layer below views.
> (In the initial code review, the description included "could in theory
> work outside of chrome".  Not that that's a design goal, but it does
> seem useful from a modularity perspective.)

Right, that's a good point. That is my primary intent for this. Thanks for
reminding me :)

So, overall, I see views/events as a central point where we convert from
platform events to views/events. Moving that in here may save 1 translation, but
at what cost? If we change to logic of views/events, it's ok from a Wayland
perspective. We just need to change the way we transform the event, no need to
change this code.

There's also other kinds of events not covered in views/events (like geometry
change, focus change) that would need to be wrapped as they are in
wayland/events. If we mix them, I think we'd loose the 1 interface for all
Wayland events which makes it nicer to pass generic events around.

Plus, I hope that in the future the Wayland library will provide something
similar to this, and we'd just remove this.

[msw, 2011-07-27 21:52:02]

Yes, it these "native" WaylandEvents are akin to those provided in GTK/X/Win
with GDKEvents/XEvent/MSG. I'm surprised Wayland doesn't support a similar
paradigm. Constructing WaylandEvents and then transforming them into Views
events seems on par with other platforms, not that I'd call it ideal :)

http://codereview.chromium.org/7457023/diff/30028/ui/wayland/events/wayland_e...
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/30028/ui/wayland/events/wayland_e...
ui/wayland/events/wayland_event.h:46: MIDDLE_BUTTON   = BTN_RIGHT,
Should right and middle be swapped?

[dnicoara, 2011-07-27 22:36:15]

On 2011/07/27 21:52:02, msw wrote:
>
http://codereview.chromium.org/7457023/diff/30028/ui/wayland/events/wayland_e...
> File ui/wayland/events/wayland_event.h (right):
> 
>
http://codereview.chromium.org/7457023/diff/30028/ui/wayland/events/wayland_e...
> ui/wayland/events/wayland_event.h:46: MIDDLE_BUTTON   = BTN_RIGHT,
> Should right and middle be swapped?

That's what I thought. I tested it on different hardware and the events are
correct as is (inverted). The sample Wayland clients don't even use the defines,
they just use the integer values.

[sky, 2011-07-28 00:37:30]

On Wed, Jul 27, 2011 at 2:11 PM,  <dnicoara@chromium.org> wrote:
> On 2011/07/27 20:50:11, Evan Martin wrote:
>>
>> On Wed, Jul 27, 2011 at 1:26 PM, Scott Violet <mailto:sky@chromium.org>
>> wrote:
>> > On Wed, Jul 27, 2011 at 10:30 AM, mailto: <dnicoara@chromium.org> wrote:
>> >> Regarding your event question, I'm not sure how much improvement that
>> >> would
>> >> bring, in the current state. The views/events/event.* files are waiting
>> >> for
>> >> GTK
>> >> cleanup and it is obvious that from a Wayland and X perspective there
>> >> is a
>> >> lot
>> >> of code that needs to be removed in there.
>> >
>> > I'm assuming if you're using Wayland you wouldn't be using views/event
>> > with gtk events. Instead you would be creating the various views/event
>> > classes directly (well, after we move them some where else).
>> >
>> > With the approach you have here we end up creating the wayland events
>> > than when you wire up the views side you'll need to convert them to
>> > views/events.
>
>> When I had read it, it was my understanding that this intended to be a
>> layer below views.
>> (In the initial code review, the description included "could in theory
>> work outside of chrome".  Not that that's a design goal, but it does
>> seem useful from a modularity perspective.)
>
> Right, that's a good point. That is my primary intent for this. Thanks for
> reminding me :)
>
> So, overall, I see views/events as a central point where we convert from
> platform events to views/events. Moving that in here may save 1 translation,
> but
> at what cost? If we change to logic of views/events, it's ok from a Wayland
> perspective. We just need to change the way we transform the event, no need
> to
> change this code.
>
> There's also other kinds of events not covered in views/events (like
> geometry
> change, focus change) that would need to be wrapped as they are in
> wayland/events. If we mix them, I think we'd loose the 1 interface for all
> Wayland events which makes it nicer to pass generic events around.
>
> Plus, I hope that in the future the Wayland library will provide something
> similar to this, and we'd just remove this.
>
> http://codereview.chromium.org/7457023/
>

Ok, makes sense. LGTM

  -Scott

[commit-bot: I haz the power, 2011-07-28 17:38:26]

Change committed as 94492