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

Thanks for the review :)

> I do not understand this part:
>
> 1891 + /* We need scratchFbo to be set before doing this, and it is
> common
> 1892 + * to both the GLES and non-GLES codepaths, so using another
> #ifdef
> 1893 + */
> 1894 + priv->updateFrameProvider ();
> 1895 +
>
> Is configureLock not needed here anymore ?:
>

Yep, that was an error, possibly caused by the tons-of-re-merging I've had to do here. Fixed now.

> 2202 - icons (),
> 2203 - configureLock (w->obtainLockOnConfigureRequests ())
> 2204 + icons ()
>
> There is no #ifdef here ?

The comment itself was outdated, fixed.
>
> These changes seem to be unrelated, but okay:
>
> 2216 - case PlaceOptions::ModeCascade:
> 2217 + case PlaceOptions::ModeCascade:
>
> 2225 -
>
> 2359 - if (serverFrameGeometry.width () == xwc->width +
> serverGeometry.border () * 2
> 2360 + if (serverFrameGeometry.width () == xwc->width +
> serverGeometry.border () * 2
>

Reverted.

> Was this wrong ?:
>
> 2368 - /* Gravity here is assumed to be SouthEast, clients can update
> 2369 + /* Gravity here is assumed to be NorthWest, clients can update
>

Also reverted. I'll propose it separately, looks like it probably got in there by accident.

> What is the water change about ? Water needs FBOs to work AFAIK.
>

Painting the whole scene to an framebuffer object was turned off by default unless some plugin or opengl indicates that it is necessary on this frame. The water change basically hooks glPaintCompositedOutputRequired when water is active and always returns true.

> Here brackets could be removed:
>
> 1995 - CompRegion tmpRegion = (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
> ?
> 1996 - screen->region () : region;
> 1997 + CompRegion paintRegion ((mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
> ?
> 1998 + screen->region () : region);
>

Bracketing bitwise operations is usually a good idea when they are mixed with other parts of if statements.

> Here also:
>
> 2006 + if ((GL::fboEnabled && postprocessRequiredForCurrentFrame ()))
>

Fixed, thanks.

> Why not 7 ?:
>
> 1469 -#define COMPIZ_OPENGL_ABI 6
> 1470 +#define COMPIZ_OPENGL_ABI 8
>

Fixed.

> Is this intended, it looks strange ?:
>
> 626 + ${CMAKE_THREAD_LIBS_INIT} # Link in pthread.
> 627 + )
>

Fixed. Its left over from when I removed all of the references but wasn't able to do so here yet.

> The rest LGTM AFAICT.

I might self-review this a little later, so for now.