Code review comment for lp:~vanvugt/mir/surface-types

Alan,

Extraneous "configure" methods:
I only added methods where I absolutely needed to, to make the code compile and run. However that was many revisions ago so I'll check to see if it's still required...

Trailing whitespace at line 90:
Will fix now.

216 === added file 'include/shared/mir/common.h'
"common.h" is not part of the "toolkit". It is used by the client toolkit and in the server.

SurfaceSetting vs SurfaceParameters
Yes they will always be different. SurfaceParameters are required information to create a surface. SurfaceSetting is something you might optionally change later; like type, state, hidden, width, height (most of which are not implemented yet but require this branch to land first).

Kevin,

Stress test takes too long:
I think 1.3 seconds is OK for a stress test, but I can shorten it if you really want.

"configure" is not self explanatory:
I kind of agree but have not yet thought of a better method name other than "set_attribute" (making the protocol call something like "set_surface_attribute"). I prefer "configure_surface". More suggestions?

Who cares about surface type?...
You're absolutely right that surface type is something only the shell will care about and act upon. However for consistency between different shells and avoiding duplication, it is reasonable for the server to store the type value in each server surface instance. Even though it will only be used by the shell. This will change as new attributes are added like "state" which will potentially be used by the server directly.

Test length:
Yes, I will always make code as concise as possible without hurting readability.