Code review comment for lp:~smspillaz/unity/unity.less-paint-insanity

Sam,

This code isn't really that readable. The code it is replacing
wasn't really readable either to anyone who doesn't deeply
understand the internals of the compiz drawing machinary.

I do agree that "more diff size != less maintainability", however
in this case, the class names and method names do not express
intent. I am not able to clearly follow what is supposed to be
happening here.

As an example, we have "FBOBindingQueryInterface", which has a
single pure virtual function called "WasBound". However this
method reflects the internal state of some object. It does not
describe the intent, or why a bound FBO is an important thing.

There is no documentation in the header files for any of these
classes compounding their confusion.

I'd suggest that we put all the methods and headers here into a
nested namespace inside unity, as it is directly related to
compiz painting. So unity::compiz or unity::plugin. Givin there
is a ::compiz namespace, it probably makes sense to have it in
namespace unity::plugin.

The constants inside ShellPaintRequestorInterface should really
be an enum. Also it isn't specified that the paint requestors
are supposed to be masks, or that the values will be used as
masks. Also, if we have a nested namespace with an enum, the
enum could be moved outside the class itself.

The fact that I had to do many searchs in the diff to work out
who called what with which parameters is an indicator of how hard
this code is to follow.

On matters of style, this code should follow the unity style
guidelines, not compiz, so * and & next to type not variable
names, no spaces before ().

Also unity has a strong preference for sigc::slot definitions for
functions, not boost::functions.

The fundamental objection I have to this is that the new code and
interfaces don't sufficiently describe the intent, but instead
leak implementation details into the class and method names.