Merge lp://qastaging/~smspillaz/compiz-core/compiz-core.decor_928173 into lp://qastaging/compiz-core

I can't say for sure whether or not this will fix bug 928173 but it will certainly help

There's a lot of shared_(ptr|array) where scoped_(ptr|array) would be more appropriate. E.g. "boost::shared_array <decor_quad_t> quad" - scoped_array would be more appropriate.

"throw std::exception ();" - this doesn't give the client code much chance of identifying the problem.

There's inconsistent use of "Decoration::Ptr (new ...)" and "Decoration::Ptr = Decoration::create (...)" - have you come across make_shared<>()? (Part of boost and C++11)

> There's a lot of shared_(ptr|array) where scoped_(ptr|array) would be more
> appropriate. E.g. "boost::shared_array <decor_quad_t> quad" - scoped_array
> would be more appropriate.
>

scoped_* is noncopyable, and I need it to be copyable so that I can store it in DecorWindow

> "throw std::exception ();" - this doesn't give the client code much chance of
> identifying the problem.

There are no users except internally and the users of that code only need to know if creating the decoration failed for /some/ reason, eg pixmap binding failed or whatever.

>
> There's inconsistent use of "Decoration::Ptr (new ...)" and "Decoration::Ptr =
> Decoration::create (...)" - have you come across make_shared<>()? (Part of
> boost and C++11)

I agree that the combination of Decoration::Ptr (new) and Decoration::create (...) is awkward - I would have made the constructor protected in favor of the named constructor, however the default "window" type decoration is just initialized from some property data. Maybe a named constructor there (eg defaultWindow () defaultPixmap () would be useful ?)

Nice. It works and bug 928173 is no more. Two stylistic comments though...

1. Shouldn't these:
    compiz::window::extents::Extents::Extents () {}

    compiz::window::extents::Extents::Extents (int left, int right, int top, int bottom) :
    ...
instead be written as:
    namespace compiz::window::extents {

    Extents::Extents () {}

    Extents::Extents (int left, int right, int top, int bottom) :
    ...
    } // namespace compiz::window::extents

2. Unnecessary namespaces: Shouldn't "compiz::window::extents" simply be "compiz::window::extents"? Or even just "compiz"? Namespaces should really only change across different build targets. So that would mean core uses "compiz" and plugin Foo uses "compiz::Foo". Though, this point is purely opinion.

If only the prerequisite branch didn't still cause a regression, I would merge this.

On Thu, 9 Feb 2012, Daniel van Vugt wrote:

> Review: Approve
>
> Nice. It works and bug 928173 is no more. Two stylistic comments though...
>

Good to hear.

> 1. Shouldn't these:
> compiz::window::extents::Extents::Extents () {}
>
> compiz::window::extents::Extents::Extents (int left, int right, int top, int bottom) :
> ...
> instead be written as:
> namespace compiz::window::extents {
>
> Extents::Extents () {}
>
> Extents::Extents (int left, int right, int top, int bottom) :
> ...
> } // namespace compiz::window::extents
>
> 2. Unnecessary namespaces: Shouldn't "compiz::window::extents" simply be "compiz::window::extents"? Or even just "compiz"? Namespaces should really only change across different build targets. So that would mean core uses "compiz" and plugin Foo uses "compiz::Foo". Though, this point is purely opinion.
> --
> https://code.launchpad.net/~smspillaz/compiz-core/compiz-core.decor_928173/+merge/91986
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.decor_928173.
>

Also, as we have discussed before, memcmp on structures is usually unsafe:

70 + if (memcmp (&cFrame, frame, sizeof (decor_extents_t)) ||
71 + memcmp (&cBorder, border, sizeof (decor_extents_t)) ||
72 + memcmp (&cMax_frame, max_frame, sizeof (decor_extents_t)) ||
73 + memcmp (&cMax_border, max_border, sizeof (decor_extents_t)))
74 + continue;

However in this case, I believe it will be safe with decor_extents_t on any architecture. Because 4 ints should always add up to a multiple of the struct alignment. There *should* be no padding bytes to worry about.

> If only the prerequisite branch didn't still cause a regression, I would merge
> this.

I found that if I only cherry pick revisions 2984+2985 then the fix for bug 928173 continues to work without the regression introduced by the prerequisite branch. So I'll do that.