Code review comment for lp:~compiz-team/compiz/compiz.fix_1119608

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Overall looks good, a few things:

Some warning when compiling:
http://paste.ubuntu.com/1683356/

CI had some test seg fault (which I don't get here), so possibly taking a look into that.

Just some minor things here:

78 - default_frames[i].d->pixmap = create_pixmap (default_frames[i].d->width, default_frames[i].d->height, frame->style_window_rgba);
79 + default_frames[i].d->pixmap = create_native_pixmap_and_wrap (default_frames[i].d->width, default_frames[i].d->height, frame->style_window_rgba);

Just looks like a small indentation error.

119 + if (w == 0 || h == 0)
120 + abort ();

I've seen w or h on windows get to -1 before, should we possibly change it to w <= 0 || h <= 0?

446 + foreach (const Decoration::Ptr &d, mList)

Though I do love this macro, I would love a range for loop but compiz doesn't use c++0x :(. (just a comment)

622 +#include <boost/noncopyable.hpp>

I don't see you inherit from this noncopyable class, do we need this header?

695 + int destroyUnusedPixmap (Pixmap pixmap) { return decor_post_delete_pixmap (mDisplay,
696 + 0,
697 + pixmap); }

Just needs little bit of lining up of the params :).

review: Needs Fixing

« Back to merge proposal