Adding a Wayland basic toolkit (issue7457023)

53 views
Skip to first unread message

ev...@chromium.org

unread,
Jul 21, 2011, 12:37:56 PM7/21/11
to dnic...@chromium.org, chromium...@chromium.org
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.h#newcode24
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#newcode13
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.

http://codereview.chromium.org/7457023/

dnic...@chromium.org

unread,
Jul 22, 2011, 2:00:46 PM7/22/11
to ev...@chromium.org, chromium...@chromium.org
Reviewers: Evan Martin,

Message:
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#newcode24
> 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#newcode13
> 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.

Description:
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=
TEST=


Please review this at http://codereview.chromium.org/7457023/

SVN Base: http://git.chromium.org/git/chromium.git@trunk

Affected files:
A ui/wayland/events/wayland_event.h
A ui/wayland/wayland.gyp
A ui/wayland/wayland_buffer.h
A ui/wayland/wayland_buffer.cc
A ui/wayland/wayland_cursor.h
A ui/wayland/wayland_cursor.cc
A ui/wayland/wayland_display.h
A ui/wayland/wayland_display.cc
A ui/wayland/wayland_geometry_utils.h
A ui/wayland/wayland_input_device.h
A ui/wayland/wayland_input_device.cc
A ui/wayland/wayland_message_pump.h
A ui/wayland/wayland_message_pump.cc
A ui/wayland/wayland_screen.h
A ui/wayland/wayland_screen.cc
A ui/wayland/wayland_shm_buffer.h
A ui/wayland/wayland_shm_buffer.cc
A ui/wayland/wayland_widget.h
A ui/wayland/wayland_window.h
A ui/wayland/wayland_window.cc


ev...@chromium.org

unread,
Jul 22, 2011, 2:21:22 PM7/22/11
to dnic...@chromium.org, chromium...@chromium.org
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_event.h
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/4001/ui/wayland/events/wayland_event.h#newcode5
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_event.h#newcode13
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_event.h#newcode17
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_event.h#newcode51
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#newcode14
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#newcode18
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#newcode5
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#newcode13
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#newcode19
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#newcode8
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.cc#newcode54
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#newcode29
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.

http://codereview.chromium.org/7457023/

dnic...@chromium.org

unread,
Jul 22, 2011, 4:45:26 PM7/22/11
to ev...@chromium.org, chromium...@chromium.org
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_event.h#newcode5
> 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_event.h#newcode13
> 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_event.h#newcode17
> 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_event.h#newcode51
> 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#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#newcode14
> 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#newcode18
> 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#newcode5
> 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#newcode13
> 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#newcode19
> 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#newcode8
> 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#newcode54
> 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#newcode29
> 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 :)


http://codereview.chromium.org/7457023/

ev...@chromium.org

unread,
Jul 22, 2011, 5:08:54 PM7/22/11
to dnic...@chromium.org, chromium...@chromium.org
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_event.h
File ui/wayland/events/wayland_event.h (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/events/wayland_event.h#newcode11
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_event.h#newcode123
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#newcode54
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#newcode26
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.cc#newcode24
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.cc#newcode148
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#newcode49
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#newcode63
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_utils.h
File ui/wayland/wayland_geometry_utils.h (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_geometry_utils.h#newcode6
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_device.cc
File ui/wayland/wayland_input_device.cc (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_device.cc#newcode169
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_device.cc#newcode192
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_device.h
File ui/wayland/wayland_input_device.h (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_input_device.h#newcode51
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_buffer.cc
File ui/wayland/wayland_shm_buffer.cc (right):

http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_shm_buffer.cc#newcode26
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#newcode15
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...

http://codereview.chromium.org/7457023/

ev...@chromium.org

unread,
Jul 22, 2011, 5:09:22 PM7/22/11
to dnic...@chromium.org, chromium...@chromium.org

dnic...@chromium.org

unread,
Jul 22, 2011, 6:17:31 PM7/22/11
to ev...@chromium.org, chromium...@chromium.org
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_event.h#newcode11
> 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_event.h#newcode123
> 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#newcode54
> ui/wayland/wayland.gyp:54: 'USE_SYSTEM_LIBWAYLAND',
> Does anyone use this?


Right, not needed anymore. Removed.

> } // namespace

> (which conveys it's closing the anonymous namespace)


Done


http://codereview.chromium.org/7457023/diff/9002/ui/wayland/wayland_display.cc#newcode24
> 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.cc#newcode148
> 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#newcode49
> 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#newcode63
> 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_utils.h#newcode6
> 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_device.cc#newcode169
> 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_device.cc#newcode192
> 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_device.h#newcode51
> 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_buffer.cc#newcode26
> 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#newcode15
> 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.

http://codereview.chromium.org/7457023/

ev...@chromium.org

unread,
Jul 22, 2011, 6:41:45 PM7/22/11
to dnic...@chromium.org, chromium...@chromium.org
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#newcode13
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#newcode38
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_device.cc
File ui/wayland/wayland_input_device.cc (right):

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_input_device.cc#newcode21
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

http://codereview.chromium.org/7457023/

ev...@chromium.org

unread,
Jul 22, 2011, 6:42:11 PM7/22/11
to dnic...@chromium.org, chromium...@chromium.org
Oh and regarding the namespace, we can keep it in mind and do it if it
becomes
necessary

http://codereview.chromium.org/7457023/

tfa...@chromium.org

unread,
Jul 22, 2011, 6:59:30 PM7/22/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org
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.

http://codereview.chromium.org/7457023/

tfa...@chromium.org

unread,
Jul 22, 2011, 7:06:45 PM7/22/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org

http://codereview.chromium.org/7457023/diff/4006/ui/wayland/wayland_cursor.h#newcode17
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#newcode48
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#newcode51
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#newcode55
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

http://codereview.chromium.org/7457023/

dnic...@chromium.org

unread,
Jul 25, 2011, 11:58:50 AM7/25/11
to ev...@chromium.org, tfa...@chromium.org, chromium...@chromium.org
I've added the ui namespace and fixed the mentioned issues.


http://codereview.chromium.org/7457023/

ev...@chromium.org

unread,
Jul 25, 2011, 12:16:01 PM7/25/11
to dnic...@chromium.org, tfa...@chromium.org, chromium...@chromium.org

ev...@chromium.org

unread,
Jul 25, 2011, 12:16:09 PM7/25/11
to dnic...@chromium.org, tfa...@chromium.org, chromium...@chromium.org
On 2011/07/25 16:16:01, Evan Martin wrote:
> LGTM+=

LGTM++ rather

tfa...@chromium.org

unread,
Jul 25, 2011, 12:27:13 PM7/25/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_event.h#newcode32
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_event.h#newcode115
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_event.h#newcode125
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.cc#newcode20
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.cc#newcode21
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.cc#newcode21
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.cc#newcode43
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_device.cc
File ui/wayland/wayland_input_device.cc (right):

http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_input_device.cc#newcode16
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_device.cc#newcode17
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_device.cc#newcode96
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_device.cc#newcode162
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.

http://codereview.chromium.org/7457023/

dnic...@chromium.org

unread,
Jul 25, 2011, 2:03:43 PM7/25/11
to ev...@chromium.org, tfa...@chromium.org, chromium...@chromium.org
On 2011/07/25 16:27:13, tfarina wrote:


http://codereview.chromium.org/7457023/diff/11004/ui/wayland/events/wayland_event.h#newcode32
> 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_event.h#newcode115
> 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_event.h#newcode125
> ui/wayland/events/wayland_event.h:125: typedef _WaylandEvent WaylandEvent;
> Why?


http://codereview.chromium.org/7457023/diff/11004/ui/wayland/wayland_cursor.cc#newcode20
> 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.cc#newcode21
> 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.cc#newcode21
> 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.cc#newcode43
> 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_device.cc#newcode16
> 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_device.cc#newcode17
> 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_device.cc#newcode96
> 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_device.cc#newcode162
> 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

http://codereview.chromium.org/7457023/

tfa...@chromium.org

unread,
Jul 25, 2011, 2:34:27 PM7/25/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org
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.cc#newcode68
ui/wayland/wayland_display.cc:68: it != screen_list_.end(); it++) {
++i

http://codereview.chromium.org/7457023/diff/12001/ui/wayland/wayland_display.cc#newcode158
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.h#newcode53
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.h#newcode69
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.cc#newcode16
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.cc#newcode34
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.cc#newcode35
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.cc#newcode35
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.cc#newcode89
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#newcode25
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#newcode27
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#newcode30
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#newcode42
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#newcode44
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#newcode47
ui/wayland/wayland_window.h:47: wl_surface* GetSurface() const;
And here.

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

http://codereview.chromium.org/7457023/

dnic...@chromium.org

unread,
Jul 25, 2011, 3:22:51 PM7/25/11
to ev...@chromium.org, tfa...@chromium.org, chromium...@chromium.org
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#newcode68
> ui/wayland/wayland_display.cc:68: it != screen_list_.end(); it++) {
> ++i


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


Nope, the 3 values are the only values.


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

tfa...@chromium.org

unread,
Jul 25, 2011, 5:10:09 PM7/25/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org

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#newcode21
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#newcode24
ui/wayland/wayland_buffer.h:24: wl_buffer* GetBuffer() const { return
buffer_; }
Nope, I said the preferred way is:

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

http://codereview.chromium.org/7457023/

tfa...@chromium.org

unread,
Jul 25, 2011, 5:11:46 PM7/25/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org

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.cc#newcode16


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.

http://codereview.chromium.org/7457023/

dnic...@chromium.org

unread,
Jul 26, 2011, 9:14:15 AM7/26/11
to ev...@chromium.org, tfa...@chromium.org, chromium...@chromium.org
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#newcode21
> 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#newcode24
> ui/wayland/wayland_buffer.h:24: wl_buffer* GetBuffer() const { return
> buffer_;
}
> Nope, I said the preferred way is:

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

Done.

http://codereview.chromium.org/7457023/

tfa...@chromium.org

unread,
Jul 26, 2011, 10:36:50 AM7/26/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org
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#newcode9
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#newcode17
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#newcode19
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#newcode43
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#newcode55
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#newcode28
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_pump.cc
File ui/wayland/wayland_message_pump.cc (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_message_pump.cc#newcode39
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_buffer.h
File ui/wayland/wayland_shm_buffer.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_shm_buffer.h#newcode22
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_buffer.h#newcode31
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#newcode66
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 ....

http://codereview.chromium.org/7457023/

tfa...@chromium.org

unread,
Jul 26, 2011, 10:49:28 AM7/26/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org

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#newcode24
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_pump.h
File ui/wayland/wayland_message_pump.h (right):

http://codereview.chromium.org/7457023/diff/28001/ui/wayland/wayland_message_pump.h#newcode22
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#newcode64
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#newcode13
ui/wayland/wayland_widget.h:13: class WaylandWidget {
I don't know, would be a better name WaylandObserver/WaylandListener?

http://codereview.chromium.org/7457023/

dnic...@chromium.org

unread,
Jul 26, 2011, 12:07:08 PM7/26/11
to ev...@chromium.org, tfa...@chromium.org, chromium...@chromium.org
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#newcode9
> 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#newcode17
> 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#newcode19
> 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#newcode43
> 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#newcode55
> 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

tfa...@chromium.org

unread,
Jul 26, 2011, 12:14:41 PM7/26/11
to dnic...@chromium.org, ev...@chromium.org, chromium...@chromium.org
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#newcode472
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#newcode34
ui/wayland/wayland.gyp:34: 'wayland_message_pump.cc',
this should be right after input_device.h

http://codereview.chromium.org/7457023/

dnic...@chromium.org

unread,
Jul 26, 2011, 12:47:53 PM7/26/11
to ev...@chromium.org, tfa...@chromium.org, chromium...@chromium.org
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#newcode472
> 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#newcode34
> ui/wayland/wayland.gyp:34: 'wayland_message_pump.cc',
> this should be right after input_device.h

Done

http://codereview.chromium.org/7457023/

Reply all
Reply to author
Forward
0 new messages