Add the xrender backend to the window manager.

Add the xrender backend to the window manager.

Change-Id: Ica06bee5746e536b31d7f7c524f9770276699a91

BUG=None
TEST=Define backend=XRENDER in the ebuild, build ChromeOS. Run ChromeOS, check that everything is fine, in particular that the window manager animations work. Open the wrench menu, check that the shadows are there.

[Daniel Erat, 2011-04-02 14:54:36]

http://codereview.chromium.org/6793005/diff/1/SConstruct
File SConstruct (right):

http://codereview.chromium.org/6793005/diff/1/SConstruct#newcode42
SConstruct:42: base_env.ParseConfig('pkg-config --cflags --libs glib-2.0 x11
xrender')
nit: this should be in wm_env below, not base_env (the screenshot program
doesn't need it)

http://codereview.chromium.org/6793005/diff/1/compositor/real_compositor.h
File compositor/real_compositor.h (right):

http://codereview.chromium.org/6793005/diff/1/compositor/real_compositor.h#ne...
compositor/real_compositor.h:28: || defined(COMPOSITOR_XRENDER))
improve indenting?

#if !(defined(...) || \
      defined(...) || \
      defined(...))
#error
#endif

http://codereview.chromium.org/6793005/diff/1/compositor/texture_data.h
File compositor/texture_data.h (right):

http://codereview.chromium.org/6793005/diff/1/compositor/texture_data.h#newco...
compositor/texture_data.h:13: unsigned texture() const { return texture_; }
"unsigned" makes me nervous.  please use uint32_t from cstdint instead, unless
the size of this needs to be architecture-dependent for some reason (didn't seem
to be the case from the pages i found)

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
File compositor/xrender/xrender_visitor.cc (right):

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:7: #include <X11/extensions/Xrender.h>
nit: move this down just under where the gflags.h include is

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:9: #include <sys/time.h>
i don't think you need this include, algorithm, ctime, string, or gflags, do
you?

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:21: #include "window_manager/profiler.h"
if you didn't use the profiling code while developing this, do you mind deleting
the calls to it from this file?  i'd like to remove it soon; i believe that we
already get better profiling by virtue of using tcmalloc

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:30: using window_manager::util::XidStr;
delete this; you don't use it

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:42: xconn_->RenderFreePicture (texture());
delete space between method name and paren

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:46: // passed-in actor's pixmap.  False is
returned if the process fails (in
this comment is obsolete; no actor is being passed in and we always return true
(should you check the return value fo RenderCreatePicture and return false if
it's 0?).  if you duplicated it from another file, can you fix the other file
too?

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:49: set_texture(
xconn_->RenderCreatePicture(pixmap, bpp) );
no spaces around function args

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:62: Compositor::StageActor* stage) {
make left edges of parameters line up

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:65: // check for the XRender extension
nit: fix capitalization and punctuation of this comment and others, i.e. "Check
for the XRender extension."

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:66: if (!xconn_->RenderQueryExtension ())
{
no space between method name and paren

you should just do CHECK(xconn_->...); we're screwed if it's not there

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:75: backPixmap = xconn_->CreatePixmap
(root_window,
fix style:
- no space between method name and paren
- one argument per line if they don't all fit on the same line
- align arguments

i think that indenting it like this may be preferred:

back_pixmap_ =
    xconn_->CreatePixmap(
        root_window,
        Size(...),
        etc.);

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:78: backPicture =
xconn_->RenderCreatePicture(backPixmap, 24);
pull the "24" to a const at the top of the file

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:87: RealCompositor::ImageActor* actor) {
align with param on previous line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:88: XPixmap pixmap =
xconn_->CreatePixmapFromData( (char*)container->data(),
fix indenting

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:89: Size(container->width(),
container->height()));
i added an Actor::GetBounds() method, so you should be able to do
container->GetBounds().size() (or feel free to add a GetSize() method if you
want)

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:119: data->Init(actor->pixmap(), 
actor->pixmap_is_opaque() ? 24:32);
fix spacing: only one space after comma, and add spaces on either side of ':'

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:141: float actor_opacity =
actor->is_opaque() ? 1.0f :
nit: move "1.0f :" to its own line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:161: xconn_->RenderComposite
(!actor->is_opaque(),
remove space before paren

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:162:
(XPicture)actor->texture_data()->texture(),
use c++ style casts (static_cast<XPicture>(...))

make these line up with the first param.  if they don't fit in 80 columns, move
the first param to its own line and indent it 4 spaces.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:163: (XPicture)None, backPicture,
one arg per line except for stuff like x, y and width, height

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:167: Size(actor->width(), actor->height())
GetBounds().size()

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:168: );
move to end of previous line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:175: if (!actor->IsVisible()) return;
move 'return' to its own line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:180: if (actor->was_resized()) {
chrome style is to not use curly braces for ifs with single-line bodies

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:186: {
move '{' to end of previous line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:189: Point(0, 0),
fix indenting

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:190: Size(root_geometry.bounds.width,
root_geometry.bounds.height)
root_geometry.bounds.size()

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:191: );
move to end of previous line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:210: if ( !damaged_region_.empty() ) {
remove extra spaces

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:216: - damaged_region_.y -
damaged_region_.height;
move initial '-' to end of previous line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:218: backPicture,
fix indenting

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:222: root_geometry.bounds.height
fix indenting

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:223: - damaged_region_.y
put operators at end of lines instead of beginnings

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:227: Size(damaged_region_.width,
damaged_region_.height)
damaged_region_.size()

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:228: );
move to end of previous line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:231: {
move to end of previous line

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:236: backPicture,
fix indenting

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:242: Size(root_geometry.bounds.width,
root_geometry.bounds.height)
size()

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:243: );
you get the idea

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:270: {
this should be indented one more space, but do you even need the extra scope?

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
File compositor/xrender/xrender_visitor.h (right):

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:5: #ifndef
WINDOW_MANAGER_XRENDER_XRENDER_VISITOR_H_
this should be WINDOW_MANAGER_COMPOSITOR_XRENDER_XRENDER_VISITOR_H_

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:21: // This class visits an actor tree and
draws it using OpenGL.
s/OpenGL/the X Render extension/

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:25: Compositor::StageActor* stage);
nit: should be indented one more space

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:50: XPicture backPicture;
these should be named back_picture_, back_pixmap_, and stage_picture_.  please
also add a comment describing what 'back' refers to (a back buffer, i'm
assuming)

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:56: friend class XRenderPixmapData;
move this to the top of the private section

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:59: // display.  Sets
framebuffer_config_rgba_ and framebuffer_config_rgb_.
nit: put pipes around variable references in comments, like
|framebuffer_config_rgba_|

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:74: // This keeps track of the number of
frames drawn so we can draw the
update this comment; you don't draw a debugging needle

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:83: // actor so we can optimize by not
clearing the COLOR_BUFFER_BIT.
update this comment so it doesn't refer to GL

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc
File x11/real_x_connection.cc (right):

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1350: if (!XRenderQueryExtension (display_,
&render_event, &render_error))
why not do "return XRenderQueryExtension(...)"?

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1359: format = XRenderFindStandardFormat(display_,
nit: move 'display_' to the beginning of the next line

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1366: XPixmap
RealXConnection::CreatePixmapFromData(char* data, const Size& size) {
see other comment about using ImageContainer instead

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1368: char* pixmap_data = (char*)malloc(data_size);
use static_cast

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1372: for(int i = 0; i<size.width * size.height; i++) {
add space between for and ( and around <

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1381: DefaultVisual(display_, DefaultScreen(display_)),
fix indenting

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1382: 32,
nit: mind labeling the ambiguous arguments with trailing comments aligned along
the right?

    32,  // depth
    ...
    0,   // whatever
    pixmap_data,
    size.width, size.height,
    32,  // ???
    0);  // ???

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1389: GC gc = XCreateGC (display_, pixmap, 0, NULL);
remove space

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1391: XDestroyImage (image);
remove space

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1392: XFreePixmap (display_, pixmap);
remove space

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1396: XPutImage (display_, pixmap, gc, image, 0, 0, 0,
0, size.width, size.height);
ditto

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1405: XPicture dst, Point srcpos, Point maskpos,
Matrix4 transform, Size size)
fix indenting

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1406: {
move to end of previous line

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1408: { XDoubleToFixed(
size.width/(float)transform[0][0] ),
fix indenting/spacing/casts

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1422: if ( (size.width != transform[0][0]) ||
(size.height != transform[1][1] ) ) {
remove extra spaces here and below

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1429: (int)srcpos.x, (int)srcpos.y, (int)maskpos.x,
(int)maskpos.y,
c++ casts, and group related params together on lines:

  srcpos.x, srcpos.y,
  maskpos.x, maskpos.y,
  dstpos.x, dstpos.y,
  transform...)

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1435: {
...

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1445: {
...

http://codereview.chromium.org/6793005/diff/1/x11/x_connection.h
File x11/x_connection.h (right):

http://codereview.chromium.org/6793005/diff/1/x11/x_connection.h#newcode549
x11/x_connection.h:549: // Query the presence of the Render extension
nit: add period at the end of this comment and the other ones.

http://codereview.chromium.org/6793005/diff/1/x11/x_connection.h#newcode555
x11/x_connection.h:555: // Create an XRender Picture from |data|
nit: describe format of |data|

http://codereview.chromium.org/6793005/diff/1/x11/x_connection.h#newcode556
x11/x_connection.h:556: virtual XPixmap CreatePixmapFromData(char* data, const
Size& size) = 0;
can you just make this take an ImageContainer instead?

[marcheu, 2011-04-02 23:31:38]

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc
File x11/real_x_connection.cc (right):

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1368: char* pixmap_data = (char*)malloc(data_size);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> use static_cast

Note to self: add a comment that XDestroyImage will free() this.

[marcheu, 2011-04-04 19:55:58]

http://codereview.chromium.org/6793005/diff/1/compositor/real_compositor.h
File compositor/real_compositor.h (right):

http://codereview.chromium.org/6793005/diff/1/compositor/real_compositor.h#ne...
compositor/real_compositor.h:28: || defined(COMPOSITOR_XRENDER))
On 2011/04/02 14:54:36, Daniel Erat wrote:
> improve indenting?
> 
> #if !(defined(...) || \
>       defined(...) || \
>       defined(...))
> #error
> #endif

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/texture_data.h
File compositor/texture_data.h (right):

http://codereview.chromium.org/6793005/diff/1/compositor/texture_data.h#newco...
compositor/texture_data.h:13: unsigned texture() const { return texture_; }
On 2011/04/02 14:54:36, Daniel Erat wrote:
> "unsigned" makes me nervous.  please use uint32_t from cstdint instead, unless
> the size of this needs to be architecture-dependent for some reason (didn't
seem
> to be the case from the pages i found)

GLuint is actually defined as an unsigned, but I still change to uint32_t.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
File compositor/xrender/xrender_visitor.cc (right):

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:7: #include <X11/extensions/Xrender.h>
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: move this down just under where the gflags.h include is

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:9: #include <sys/time.h>
On 2011/04/02 14:54:36, Daniel Erat wrote:
> i don't think you need this include, algorithm, ctime, string, or gflags, do
> you?

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:21: #include "window_manager/profiler.h"
On 2011/04/02 14:54:36, Daniel Erat wrote:
> if you didn't use the profiling code while developing this, do you mind
deleting
> the calls to it from this file?  i'd like to remove it soon; i believe that we
> already get better profiling by virtue of using tcmalloc

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:30: using window_manager::util::XidStr;
On 2011/04/02 14:54:36, Daniel Erat wrote:
> delete this; you don't use it

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:42: xconn_->RenderFreePicture (texture());
On 2011/04/02 14:54:36, Daniel Erat wrote:
> delete space between method name and paren

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:46: // passed-in actor's pixmap.  False is
returned if the process fails (in
On 2011/04/02 14:54:36, Daniel Erat wrote:
> this comment is obsolete; no actor is being passed in and we always return
true
> (should you check the return value fo RenderCreatePicture and return false if
> it's 0?).  if you duplicated it from another file, can you fix the other file
> too?

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:49: set_texture(
xconn_->RenderCreatePicture(pixmap, bpp) );
On 2011/04/02 14:54:36, Daniel Erat wrote:
> no spaces around function args

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:62: Compositor::StageActor* stage) {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> make left edges of parameters line up

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:65: // check for the XRender extension
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: fix capitalization and punctuation of this comment and others, i.e.
"Check
> for the XRender extension."

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:66: if (!xconn_->RenderQueryExtension ())
{
On 2011/04/02 14:54:36, Daniel Erat wrote:
> no space between method name and paren
> 
> you should just do CHECK(xconn_->...); we're screwed if it's not there

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:75: backPixmap = xconn_->CreatePixmap
(root_window,
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix style:
> - no space between method name and paren
> - one argument per line if they don't all fit on the same line
> - align arguments
> 
> i think that indenting it like this may be preferred:
> 
> back_pixmap_ =
>     xconn_->CreatePixmap(
>         root_window,
>         Size(...),
>         etc.);

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:78: backPicture =
xconn_->RenderCreatePicture(backPixmap, 24);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> pull the "24" to a const at the top of the file

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:87: RealCompositor::ImageActor* actor) {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> align with param on previous line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:88: XPixmap pixmap =
xconn_->CreatePixmapFromData( (char*)container->data(),
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:89: Size(container->width(),
container->height()));
On 2011/04/02 14:54:36, Daniel Erat wrote:
> i added an Actor::GetBounds() method, so you should be able to do
> container->GetBounds().size() (or feel free to add a GetSize() method if you
> want)

ImageContainer is not an actor, so I added a GetSize().

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:119: data->Init(actor->pixmap(), 
actor->pixmap_is_opaque() ? 24:32);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix spacing: only one space after comma, and add spaces on either side of ':'

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:141: float actor_opacity =
actor->is_opaque() ? 1.0f :
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: move "1.0f :" to its own line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:161: xconn_->RenderComposite
(!actor->is_opaque(),
On 2011/04/02 14:54:36, Daniel Erat wrote:
> remove space before paren

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:162:
(XPicture)actor->texture_data()->texture(),
On 2011/04/02 14:54:36, Daniel Erat wrote:
> use c++ style casts (static_cast<XPicture>(...))
> 
> make these line up with the first param.  if they don't fit in 80 columns,
move
> the first param to its own line and indent it 4 spaces.

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:163: (XPicture)None, backPicture,
On 2011/04/02 14:54:36, Daniel Erat wrote:
> one arg per line except for stuff like x, y and width, height

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:167: Size(actor->width(), actor->height())
On 2011/04/02 14:54:36, Daniel Erat wrote:
> GetBounds().size()

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:168: );
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move to end of previous line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:175: if (!actor->IsVisible()) return;
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move 'return' to its own line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:180: if (actor->was_resized()) {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> chrome style is to not use curly braces for ifs with single-line bodies

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:186: {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move '{' to end of previous line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:189: Point(0, 0),
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:190: Size(root_geometry.bounds.width,
root_geometry.bounds.height)
On 2011/04/02 14:54:36, Daniel Erat wrote:
> root_geometry.bounds.size()

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:191: );
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move to end of previous line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:210: if ( !damaged_region_.empty() ) {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> remove extra spaces

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:216: - damaged_region_.y -
damaged_region_.height;
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move initial '-' to end of previous line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:218: backPicture,
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:222: root_geometry.bounds.height
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:223: - damaged_region_.y
On 2011/04/02 14:54:36, Daniel Erat wrote:
> put operators at end of lines instead of beginnings

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:227: Size(damaged_region_.width,
damaged_region_.height)
On 2011/04/02 14:54:36, Daniel Erat wrote:
> damaged_region_.size()

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:228: );
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move to end of previous line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:231: {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move to end of previous line

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:236: backPicture,
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:242: Size(root_geometry.bounds.width,
root_geometry.bounds.height)
On 2011/04/02 14:54:36, Daniel Erat wrote:
> size()

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:243: );
On 2011/04/02 14:54:36, Daniel Erat wrote:
> you get the idea

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.cc:270: {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> this should be indented one more space, but do you even need the extra scope?

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
File compositor/xrender/xrender_visitor.h (right):

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:5: #ifndef
WINDOW_MANAGER_XRENDER_XRENDER_VISITOR_H_
On 2011/04/02 14:54:36, Daniel Erat wrote:
> this should be WINDOW_MANAGER_COMPOSITOR_XRENDER_XRENDER_VISITOR_H_

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:21: // This class visits an actor tree and
draws it using OpenGL.
On 2011/04/02 14:54:36, Daniel Erat wrote:
> s/OpenGL/the X Render extension/

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:25: Compositor::StageActor* stage);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: should be indented one more space

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:50: XPicture backPicture;
On 2011/04/02 14:54:36, Daniel Erat wrote:
> these should be named back_picture_, back_pixmap_, and stage_picture_.  please
> also add a comment describing what 'back' refers to (a back buffer, i'm
> assuming)

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:56: friend class XRenderPixmapData;
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move this to the top of the private section

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:59: // display.  Sets
framebuffer_config_rgba_ and framebuffer_config_rgb_.
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: put pipes around variable references in comments, like
> |framebuffer_config_rgba_|

Or delete the whole thing since it's useless...

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:74: // This keeps track of the number of
frames drawn so we can draw the
On 2011/04/02 14:54:36, Daniel Erat wrote:
> update this comment; you don't draw a debugging needle

Done.

http://codereview.chromium.org/6793005/diff/1/compositor/xrender/xrender_visi...
compositor/xrender/xrender_visitor.h:83: // actor so we can optimize by not
clearing the COLOR_BUFFER_BIT.
On 2011/04/02 14:54:36, Daniel Erat wrote:
> update this comment so it doesn't refer to GL

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc
File x11/real_x_connection.cc (right):

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1350: if (!XRenderQueryExtension (display_,
&render_event, &render_error))
On 2011/04/02 14:54:36, Daniel Erat wrote:
> why not do "return XRenderQueryExtension(...)"?

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1359: format = XRenderFindStandardFormat(display_,
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: move 'display_' to the beginning of the next line

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1366: XPixmap
RealXConnection::CreatePixmapFromData(char* data, const Size& size) {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> see other comment about using ImageContainer instead

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1368: char* pixmap_data = (char*)malloc(data_size);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> use static_cast

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1372: for(int i = 0; i<size.width * size.height; i++) {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> add space between for and ( and around <

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1381: DefaultVisual(display_, DefaultScreen(display_)),
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1382: 32,
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: mind labeling the ambiguous arguments with trailing comments aligned
along
> the right?
> 
>     32,  // depth
>     ...
>     0,   // whatever
>     pixmap_data,
>     size.width, size.height,
>     32,  // ???
>     0);  // ???

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1389: GC gc = XCreateGC (display_, pixmap, 0, NULL);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> remove space

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1391: XDestroyImage (image);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> remove space

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1392: XFreePixmap (display_, pixmap);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> remove space

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1396: XPutImage (display_, pixmap, gc, image, 0, 0, 0,
0, size.width, size.height);
On 2011/04/02 14:54:36, Daniel Erat wrote:
> ditto

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1405: XPicture dst, Point srcpos, Point maskpos,
Matrix4 transform, Size size)
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1406: {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> move to end of previous line

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1408: { XDoubleToFixed(
size.width/(float)transform[0][0] ),
On 2011/04/02 14:54:36, Daniel Erat wrote:
> fix indenting/spacing/casts

Tried to do something about it, not sure exactly how it should be.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1422: if ( (size.width != transform[0][0]) ||
(size.height != transform[1][1] ) ) {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> remove extra spaces here and below

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1429: (int)srcpos.x, (int)srcpos.y, (int)maskpos.x,
(int)maskpos.y,
On 2011/04/02 14:54:36, Daniel Erat wrote:
> c++ casts, and group related params together on lines:
> 
>   srcpos.x, srcpos.y,
>   maskpos.x, maskpos.y,
>   dstpos.x, dstpos.y,
>   transform...)

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1435: {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> ...

Done.

http://codereview.chromium.org/6793005/diff/1/x11/real_x_connection.cc#newcod...
x11/real_x_connection.cc:1445: {
On 2011/04/02 14:54:36, Daniel Erat wrote:
> ...

Done.

http://codereview.chromium.org/6793005/diff/1/x11/x_connection.h
File x11/x_connection.h (right):

http://codereview.chromium.org/6793005/diff/1/x11/x_connection.h#newcode549
x11/x_connection.h:549: // Query the presence of the Render extension
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: add period at the end of this comment and the other ones.

Done.

http://codereview.chromium.org/6793005/diff/1/x11/x_connection.h#newcode555
x11/x_connection.h:555: // Create an XRender Picture from |data|
On 2011/04/02 14:54:36, Daniel Erat wrote:
> nit: describe format of |data|

Done.

http://codereview.chromium.org/6793005/diff/1/x11/x_connection.h#newcode556
x11/x_connection.h:556: virtual XPixmap CreatePixmapFromData(char* data, const
Size& size) = 0;
On 2011/04/02 14:54:36, Daniel Erat wrote:
> can you just make this take an ImageContainer instead?

Done.

[Daniel Erat, 2011-04-04 21:16:35]

Thanks, this is looking pretty good now!

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.cc (right):

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:29: pixmap_ = 0;
move these to an initialization list?

XRenderPictureData(...)
    : pixmap_(0),
      xconn_(visitor->xconn()) {
}

i don't think the set_texture(0) is necessary unless you really want it;
TextureData() already sets it to 0 in its c'tor.

i'd also make this just take an XConnection instead of an XRenderDrawVisitor;
that seems to be all you need, and then you can get rid of
XRenderDrawVisitor::xconn() if nothing else is using it.

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:41: XPicture r =
xconn_->RenderCreatePicture(pixmap, bpp);
nit: s/r/picture/

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:56: xconn_ = compositor->x_conn();
nit: move this to an initialization list.  i'd also prefer that you initialized
all of the other POD-type members to 0 in a list as well, even if you're going
to assign something else to them in the c'tor body -- it makes it easier to
figure out what's going on if someone does some refactoring in the body later
and forgets to initialize one

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:69: Size(root_geometry.bounds.width,
root_geometry.bounds.height),
nit: root_geometry.bounds.size()

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:88: XRenderPictureData* data = new
XRenderPictureData(this);
mind using scoped_ptr here and then data.release() in the set_texture_data call?

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:110: XRenderPictureData* data = new
XRenderPictureData(this);
scoped_ptr here too please

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:171: if (actor->was_resized())
don't you need to throw out your pictures and pixmap here and create new resized
ones?

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:199: Matrix4 identity =
Vectormath::Aos::Matrix4::identity();
you can just use "Matrix4::identity()" here, right?

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:258: DLOG(INFO) << "Drawing transparent
child " << child->name()
nit: transparent/opaque here and in the next log statement are inaccurate

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.h (right):

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:25: Compositor::StageActor* stage);
not your fault (the gl and gles implementations do it too), but just pointing it
out in case you feel like fixing it: it's weird that this is a
Compositor::StageActor instead of a RealCompositor::StageActor

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:32: void set_damaged_region(Rect
damaged_region) {
nit: s/Rect/const Rect&/ (i'm cleaning this up in the other visitors in
http://codereview.chromium.org/6691037/)

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:50: XWindow root_window;
add trailing underscore

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:51: XConnection::WindowGeometry
root_geometry;
add trailing underscore

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:53: XPicture back_picture_;
these should still have comments with brief descriptions of their purposes

http://codereview.chromium.org/6793005/diff/5001/image_container.h
File image_container.h (right):

http://codereview.chromium.org/6793005/diff/5001/image_container.h#newcode44
image_container.h:44: Size GetSize() const { return Size(width_,height_); }
nit: just name this size() (some of the other code probably doesn't match up
with this yet, but chrome style is to use lowercase for inline methods)

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc
File x11/real_x_connection.cc (right):

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1366: const ImageContainer* container) {
nit: make this be a reference instead of a pointer (makes it obvious from the
signature that it's required instead of optional)

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1416: XPicture dst, Point srcpos, Point maskpos,
use const references for non-POD types, and use separate lines for each if they
don't all fit on a single line:

     ...(bool blend,
         XPicture src,
         XPicture mask,
         XPicture dest,
         const Point& src_pos,
         const Point& mask_pos,
         const Matrix4& transform,
         const Size& size) {

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1422: if ((size.width != transform[0][0]) ||
(size.height != transform[1][1])) {
might be simpler as this (may require static_casts, though, so it's your call):

if (size != Size(transform[0][0], transform[1][1])) {

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1442: int(srcpos.x), (int)(srcpos.y),
nit: use static_cast (also, you're mixing int() and (int) here) :-P

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1456: Point pos,
const Point&, const Size&

http://codereview.chromium.org/6793005/diff/5001/x11/x_connection.h
File x11/x_connection.h (right):

http://codereview.chromium.org/6793005/diff/5001/x11/x_connection.h#newcode18
x11/x_connection.h:18: #include "window_manager/image_container.h"
nit: this should come before image_enums

[marcheu, 2011-04-05 00:23:40]

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.cc (right):

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:29: pixmap_ = 0;
On 2011/04/04 21:16:35, Daniel Erat wrote:
> move these to an initialization list?
> 
> XRenderPictureData(...)
>     : pixmap_(0),
>       xconn_(visitor->xconn()) {
> }
> 
> i don't think the set_texture(0) is necessary unless you really want it;
> TextureData() already sets it to 0 in its c'tor.
> 

Done.

> i'd also make this just take an XConnection instead of an XRenderDrawVisitor;
> that seems to be all you need, and then you can get rid of
> XRenderDrawVisitor::xconn() if nothing else is using it.

Erm, I don't get it. We need xconn all the time to call x11 functions. Why do
you say nothing else uses it?

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:41: XPicture r =
xconn_->RenderCreatePicture(pixmap, bpp);
On 2011/04/04 21:16:35, Daniel Erat wrote:
> nit: s/r/picture/

Done.

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:56: xconn_ = compositor->x_conn();
On 2011/04/04 21:16:35, Daniel Erat wrote:
> nit: move this to an initialization list.  i'd also prefer that you
initialized
> all of the other POD-type members to 0 in a list as well, even if you're going
> to assign something else to them in the c'tor body -- it makes it easier to
> figure out what's going on if someone does some refactoring in the body later
> and forgets to initialize one

Done.

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:69: Size(root_geometry.bounds.width,
root_geometry.bounds.height),
On 2011/04/04 21:16:35, Daniel Erat wrote:
> nit: root_geometry.bounds.size()

Done.

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:88: XRenderPictureData* data = new
XRenderPictureData(this);
On 2011/04/04 21:16:35, Daniel Erat wrote:
> mind using scoped_ptr here and then data.release() in the set_texture_data
call?

Done.

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:110: XRenderPictureData* data = new
XRenderPictureData(this);
On 2011/04/04 21:16:35, Daniel Erat wrote:
> scoped_ptr here too please

Done.

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:171: if (actor->was_resized())
On 2011/04/04 21:16:35, Daniel Erat wrote:
> don't you need to throw out your pictures and pixmap here and create new
resized
> ones?

Done.

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:199: Matrix4 identity =
Vectormath::Aos::Matrix4::identity();
On 2011/04/04 21:16:35, Daniel Erat wrote:
> you can just use "Matrix4::identity()" here, right?

Done.

http://codereview.chromium.org/6793005/diff/5001/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:258: DLOG(INFO) << "Drawing transparent
child " << child->name()
On 2011/04/04 21:16:35, Daniel Erat wrote:
> nit: transparent/opaque here and in the next log statement are inaccurate

Done.

http://codereview.chromium.org/6793005/diff/5001/image_container.h
File image_container.h (right):

http://codereview.chromium.org/6793005/diff/5001/image_container.h#newcode44
image_container.h:44: Size GetSize() const { return Size(width_,height_); }
On 2011/04/04 21:16:35, Daniel Erat wrote:
> nit: just name this size() (some of the other code probably doesn't match up
> with this yet, but chrome style is to use lowercase for inline methods)

Done.

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc
File x11/real_x_connection.cc (right):

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1366: const ImageContainer* container) {
On 2011/04/04 21:16:35, Daniel Erat wrote:
> nit: make this be a reference instead of a pointer (makes it obvious from the
> signature that it's required instead of optional)

Done.

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1416: XPicture dst, Point srcpos, Point maskpos,
On 2011/04/04 21:16:35, Daniel Erat wrote:
> use const references for non-POD types, and use separate lines for each if
they
> don't all fit on a single line:
> 
>      ...(bool blend,
>          XPicture src,
>          XPicture mask,
>          XPicture dest,
>          const Point& src_pos,
>          const Point& mask_pos,
>          const Matrix4& transform,
>          const Size& size) {

Done.

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1422: if ((size.width != transform[0][0]) ||
(size.height != transform[1][1])) {
On 2011/04/04 21:16:35, Daniel Erat wrote:
> might be simpler as this (may require static_casts, though, so it's your
call):
> 
> if (size != Size(transform[0][0], transform[1][1])) {

Done.

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1442: int(srcpos.x), (int)(srcpos.y),
On 2011/04/04 21:16:35, Daniel Erat wrote:
> nit: use static_cast (also, you're mixing int() and (int) here) :-P

Done.

http://codereview.chromium.org/6793005/diff/5001/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1456: Point pos,
On 2011/04/04 21:16:35, Daniel Erat wrote:
> const Point&, const Size&

Done.

http://codereview.chromium.org/6793005/diff/5001/x11/x_connection.h
File x11/x_connection.h (right):

http://codereview.chromium.org/6793005/diff/5001/x11/x_connection.h#newcode18
x11/x_connection.h:18: #include "window_manager/image_container.h"
On 2011/04/04 21:16:35, Daniel Erat wrote:
> nit: this should come before image_enums

Done.

[Daniel Erat, 2011-04-05 01:35:06]

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.cc (right):

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:53: bool
XRenderDrawVisitor::FreeXResources() {
move this and AllocateXResources() down.  the methods in the .cc file should be
in the same order as in the .h file

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:55:
xconn_->RenderFreePicture(back_picture_) &&
fix indenting: line this up with the xconn_ on the previous line

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:88: root_geometry_(),
nit: no need to list objects with reasonable default c'tors

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:103:
XRenderDrawVisitor::~XRenderDrawVisitor() {
call FreeXResources() here

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.h (right):

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:8: #include <vector>
delete this; you're not using it

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:55: XPicture back_picture_;
third time i've asked: please add a comment describing how these are used :-P

http://codereview.chromium.org/6793005/diff/2004/x11/real_x_connection.cc
File x11/real_x_connection.cc (right):

http://codereview.chromium.org/6793005/diff/2004/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1419: const Point srcpos,
make this and everything below it a reference

[marcheu, 2011-04-05 02:07:42]

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.cc (right):

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:53: bool
XRenderDrawVisitor::FreeXResources() {
On 2011/04/05 01:35:06, Daniel Erat wrote:
> move this and AllocateXResources() down.  the methods in the .cc file should
be
> in the same order as in the .h file

Done.

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:55:
xconn_->RenderFreePicture(back_picture_) &&
On 2011/04/05 01:35:06, Daniel Erat wrote:
> fix indenting: line this up with the xconn_ on the previous line

Done.

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:88: root_geometry_(),
On 2011/04/05 01:35:06, Daniel Erat wrote:
> nit: no need to list objects with reasonable default c'tors

Done.

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:103:
XRenderDrawVisitor::~XRenderDrawVisitor() {
On 2011/04/05 01:35:06, Daniel Erat wrote:
> call FreeXResources() here

Done.

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.h (right):

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:8: #include <vector>
On 2011/04/05 01:35:06, Daniel Erat wrote:
> delete this; you're not using it

Done.

http://codereview.chromium.org/6793005/diff/2004/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:55: XPicture back_picture_;
On 2011/04/05 01:35:06, Daniel Erat wrote:
> third time i've asked: please add a comment describing how these are used :-P

Done.

http://codereview.chromium.org/6793005/diff/2004/x11/real_x_connection.cc
File x11/real_x_connection.cc (right):

http://codereview.chromium.org/6793005/diff/2004/x11/real_x_connection.cc#new...
x11/real_x_connection.cc:1419: const Point srcpos,
On 2011/04/05 01:35:06, Daniel Erat wrote:
> make this and everything below it a reference

Done.

[Daniel Erat, 2011-04-05 22:45:19]

Thanks, LGTM now with two small changes!

Please also update the changelist description before committing (via the
codereview site) to have:

BUG=none
TEST=whatever you did to test it -- something like 'manual' is fine

http://codereview.chromium.org/6793005/diff/3007/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.cc (right):

http://codereview.chromium.org/6793005/diff/3007/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:79: data(new
XRenderPictureData(this->xconn_));
nit: variable name should come on same line as type:

scoped_ptr<window_manager::XRenderPictureData> data(
    new XRenderPictureData(this->xconn_));

http://codereview.chromium.org/6793005/diff/3007/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.h (right):

http://codereview.chromium.org/6793005/diff/3007/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:33: void BindImage(const ImageContainer*
container,
make this be a reference instead of a pointer

[marcheu, 2011-04-06 00:26:07]

http://codereview.chromium.org/6793005/diff/3007/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.cc (right):

http://codereview.chromium.org/6793005/diff/3007/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.cc:79: data(new
XRenderPictureData(this->xconn_));
On 2011/04/05 22:45:19, Daniel Erat wrote:
> nit: variable name should come on same line as type:
> 
> scoped_ptr<window_manager::XRenderPictureData> data(
>     new XRenderPictureData(this->xconn_));

Done.

http://codereview.chromium.org/6793005/diff/3007/compositor/xrender/xrender_v...
File compositor/xrender/xrender_visitor.h (right):

http://codereview.chromium.org/6793005/diff/3007/compositor/xrender/xrender_v...
compositor/xrender/xrender_visitor.h:33: void BindImage(const ImageContainer*
container,
On 2011/04/05 22:45:19, Daniel Erat wrote:
> make this be a reference instead of a pointer

Done.