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

> Minor fixes done.
>
> Alan -
> I can put "common.h" anywhere of course. Just remember:
> 1. It's not part of the "toolkit". "toolkit" means client-only but
> "common.h" is also used in the server and shell.
> 2. common.h has to be installed under "include/mir/..." to make it clear to
> external users that it's part of mir. And it is implicitly in the "mir"
> namespace by virtue of the "Mir" type prefixes. We can't use "namespace mir"
> however because it is C code, not C++.

As it is included by toolkit I think it is part of toolkit. If it were really "part of mir", then there would be no reason for it to be C code. (Is it part of some hypothetical "common" that we've just identified - if so, is "common.h" isn't a good name.)

> Alan & Kevin -
> I agree that "type" is not a deep server-level attribute. It only makes sense
> at the shell level. I could move it out of surfaces::Surface now, and into a
> proxy class. However I think that should be done later because the "Proxy"
> classes clearly need some refactoring, everyone seems to agree. I'd rather not
> make assumptions about what form that will take and move any members into
> *ProxySurface until that's done.

Perhaps it is best to wait on that refactoring and reassess after that?

> Also keep in mind that _most_ future configurable attributes (state, hidden,
> width, height etc) will be low-level and live in the server surfaces::Surface
> rather than the shell.

Are you envisaging using the same mechanism?