Merge lp://qastaging/~vanvugt/mir/resize-protocol into lp://qastaging/mir

PASSED: Continuous integration, rev:1209
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/348/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/140
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/137
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/77/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/77/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/348/rebuild

160 +#ifdef ANDROID
161 +#include <system/window.h> // for ANativeWindowBuffer AKA MirNativeBuffer
162 +#endif
163 +

Not needed? (And as Daniel implies, conditional compilation in code that shouldn't depend on the platform suggests issues needing to be resolved.)

171 +#ifndef ANDROID
172 +// FIXME: It would be nice to run this on all platforms, but our Android test
173 +// infrastructure isn't ready yet. And I don't know how to fix it.
174 +
175 +TEST_F(DefaultDisplayServerTestFixture, client_gets_buffer_dimensions)
...
236 +#endif

I'd prefer to compile (and disable) tests if they are currently expected to fail on android. Vis:

+#ifndef ANDROID
+// FIXME: It would be nice to run this on all platforms, but our Android test
+// infrastructure isn't ready yet. And I don't know how to fix it.
+
+TEST_F(DefaultDisplayServerTestFixture, client_gets_buffer_dimensions)
+#else
+TEST_F(DefaultDisplayServerTestFixture, DISABLED_client_gets_buffer_dimensions)
+#endif

the test itself seems pretty platform specific, we're testing that the native type for the buffer is updated correctly. I'd be happy just having this in a directory that is gbm-only.

although its not necessary, is it slightly safer to bump libmirprotobuf after every change?
would uint32 be better for the width/height?

looks okay otherwise

Alan:
Fixed. The disabled tests are now compiled on Android. This also means <system/window.h> is actually used.

Kevin:
I'm undecided about libmirprotobuf bumps. We have a choice of detecting optional fields at runtime (and then throwing exceptions if they're not present), or simply bumping the libmirprotobuf ABI. We've so far failed to do either, so I'm open.

As for int width, height; yes that's intentional. In graphics dimensional values are always intermingled with positional values, like:
    int left = x - width;
    int bottom = y + height;
where at least two of the operands need to be signed. So using a signed width/height clarifies that they're all compatible and the reader doesn't have to worry about the (complicated) implicit conversion rules. The other reason for using signed width/height is that in many cases you will want to specify an unknown/special width/height, so negative values are very useful there.

That all said, C will happily convert between signed/unsigned ints as required. So either will work. It only becomes a real pain if you impose your own strong typing rules which lack implicit conversions (like our geometry classes).

PASSED: Continuous integration, rev:1212
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/359/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/156
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/153
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/88
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/88/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/88
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/88/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/359/rebuild

I'm concerned that "Mir's Android test infrastructure isn't quite ready for this yet.". But OK for now

Why can't the test run on android? Is it only because of:
233 + bool use_real_graphics(mir::options::Option const&) override
234 + {
235 + return true;
236 + }

I think the solution is then to appropriately stub the graphics platform for this test.

> I'm concerned that "Mir's Android test infrastructure isn't quite ready for
> this yet.". But OK for now

I don't see how it is not ready, which is why I'd rather have the test in question in a directory that is only compiled into the gbm tests until the test can be made to work. Overall though, okay, so i'll unblock

Kevin,

If you think the test should work on Android, please try enabling it and look at the failure. Maybe it's a simple problem, but I still get lost in our test infrastructure...

28 -enum { mir_buffer_package_max = 31 };
29 +enum { mir_buffer_package_max = 30 };

Note that this is technically an ABI break too. For example:

memset(pkg.data, 0, mir_buffer_package_max * sizeof(int))

would overwrite 'width' if compiled with the current trunk version, but run with the proposed one.

Although I don't think it affects us in practice, I propose we play it safe and bump the ABIs anyway.

Other than that, looks good.