Mir

Merge lp:~vanvugt/mir/deduplicate-NullDisplayReport into lp:mir

Proposed by Daniel van Vugt on 2016-09-05
Status: Work in progress
Proposed branch: lp:~vanvugt/mir/deduplicate-NullDisplayReport
Merge into: lp:mir
Diff against target: 272 lines (+40/-63)
9 files modified
include/platform/mir/graphics/null_display_report.h (+9/-12)
src/platform/graphics/CMakeLists.txt (+1/-0)
src/platform/graphics/null_display_report.cpp (+19/-17)
src/platform/symbols.map (+3/-0)
src/platforms/android/server/device_quirks.cpp (+2/-14)
src/platforms/android/utils/render_overlays.cpp (+2/-15)
src/server/report/null/CMakeLists.txt (+0/-1)
src/server/report/null/null_report_factory.cpp (+2/-2)
tests/unit-tests/platforms/nested/test_nested_display.cpp (+2/-2)
To merge this branch: bzr merge lp:~vanvugt/mir/deduplicate-NullDisplayReport
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve on 2016-09-07
Alan Griffiths 2016-09-05 Disapprove on 2016-09-05
Review via email: mp+304905@code.launchpad.net

Commit message

Deduplicate three null DisplayReports into one.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3685
https://mir-jenkins.ubuntu.com/job/mir-ci/1634/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2044/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2106
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2097
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2097
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2097
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2070/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2070/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2070/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2070/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2070/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2070/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1634/rebuild

review: Needs Fixing (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

Needing NullDisplayReport in mirplatform seems more like a problem than a solution.

review: Disapprove
Daniel van Vugt (vanvugt) wrote :

Logically you make a good point. This isn't addressing the android code at the root of the issue.

However it's also worth remembering that DisplayReport is a report for Display. And Display is implemented in src/platforms/* so it makes sense that any commonality should exist in libmirplatform rather than libmirserver.

3686. By Daniel van Vugt on 2016-09-06

Ping Jenkins

Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3686
https://mir-jenkins.ubuntu.com/job/mir-ci/1636/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2049/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2111/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2102/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2102/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2102/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2075/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1636/rebuild

review: Needs Fixing (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

Come to think of it, any report class that legitimately has a null use case and that might only need a partial implementation in many cases really should not be pure virtual at all. A possibly cleaner solution to all this is to merge DisplayReport with null::DisplayReport.

Daniel van Vugt (vanvugt) wrote :

And it looks like I might be proposing four more places where NullDisplayReport gets used in future:
   http://bazaar.launchpad.net/~vanvugt/mir/physical-frame/revision/3605

3687. By Daniel van Vugt on 2016-09-07

Merge latest trunk

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3687
https://mir-jenkins.ubuntu.com/job/mir-ci/1657/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2072
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2134
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2125
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2125
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2125
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2100/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2100/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2100/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2100/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2100/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1657/rebuild

review: Approve (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

On second thoughts, mirserver is the only place in production where the report should be selected and constructed.

So ideally we should only see a null DisplayReport in mirserver and tests. I guess the current placement is correct if we can clean up and remove the two occurrences from src/platforms/android/. But that's an "if".

Unmerged revisions

3687. By Daniel van Vugt on 2016-09-07

Merge latest trunk

3686. By Daniel van Vugt on 2016-09-06

Ping Jenkins

3685. By Daniel van Vugt on 2016-09-05

It's LGPL, not GPL

3684. By Daniel van Vugt on 2016-09-05

First attempt

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== renamed file 'src/server/report/null/display_report.h' => 'include/platform/mir/graphics/null_display_report.h'
2--- src/server/report/null/display_report.h 2015-04-28 07:54:10 +0000
3+++ include/platform/mir/graphics/null_display_report.h 2016-09-07 03:00:58 +0000
4@@ -2,23 +2,23 @@
5 * Copyright © 2012 Canonical Ltd.
6 *
7 * This program is free software: you can redistribute it and/or modify it
8- * under the terms of the GNU General Public License version 3,
9+ * under the terms of the GNU Lesser General Public License version 3,
10 * as published by the Free Software Foundation.
11 *
12 * This program is distributed in the hope that it will be useful,
13 * but WITHOUT ANY WARRANTY; without even the implied warranty of
14 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15- * GNU General Public License for more details.
16+ * GNU Lesser General Public License for more details.
17 *
18- * You should have received a copy of the GNU General Public License
19+ * You should have received a copy of the GNU Lesser General Public License
20 * along with this program. If not, see <http://www.gnu.org/licenses/>.
21 *
22 * Authored by: Alan Griffiths <alan@octopull.co.uk>
23 */
24
25
26-#ifndef MIR_REPORT_NULL_DISPLAY_REPORT_H_
27-#define MIR_REPORT_NULL_DISPLAY_REPORT_H_
28+#ifndef MIR_GRAPHICS_NULL_DISPLAY_REPORT_H_
29+#define MIR_GRAPHICS_NULL_DISPLAY_REPORT_H_
30
31
32 #include "mir/graphics/display_report.h"
33@@ -26,11 +26,9 @@
34
35 namespace mir
36 {
37-namespace report
38-{
39-namespace null
40-{
41-class DisplayReport : public graphics::DisplayReport
42+namespace graphics
43+{
44+class NullDisplayReport : public graphics::DisplayReport
45 {
46 public:
47
48@@ -47,6 +45,5 @@
49 };
50 }
51 }
52-}
53
54-#endif /* MIR_REPORT_NULL_DISPLAY_REPORT_H_ */
55+#endif /* MIR_GRAPHICS_NULL_DISPLAY_REPORT_H_ */
56
57=== modified file 'src/platform/graphics/CMakeLists.txt'
58--- src/platform/graphics/CMakeLists.txt 2016-06-02 05:33:50 +0000
59+++ src/platform/graphics/CMakeLists.txt 2016-09-07 03:00:58 +0000
60@@ -11,6 +11,7 @@
61 pixel_format_utils.cpp
62 overlapping_output_grouping.cpp
63 platform_probe.cpp
64+ null_display_report.cpp
65 )
66
67 add_library(mirplatformgraphicscommon OBJECT
68
69=== renamed file 'src/server/report/null/display_report.cpp' => 'src/platform/graphics/null_display_report.cpp'
70--- src/server/report/null/display_report.cpp 2015-04-28 07:54:10 +0000
71+++ src/platform/graphics/null_display_report.cpp 2016-09-07 03:00:58 +0000
72@@ -2,31 +2,33 @@
73 * Copyright © 2013 Canonical Ltd.
74 *
75 * This program is free software: you can redistribute it and/or modify it
76- * under the terms of the GNU General Public License version 3,
77+ * under the terms of the GNU Lesser General Public License version 3,
78 * as published by the Free Software Foundation.
79 *
80 * This program is distributed in the hope that it will be useful,
81 * but WITHOUT ANY WARRANTY; without even the implied warranty of
82 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
83- * GNU General Public License for more details.
84+ * GNU Lesser General Public License for more details.
85 *
86- * You should have received a copy of the GNU General Public License
87+ * You should have received a copy of the GNU Lesser General Public License
88 * along with this program. If not, see <http://www.gnu.org/licenses/>.
89 *
90 * Authored by: Alan Griffiths <alan@octopull.co.uk>
91 */
92
93-#include "display_report.h"
94-
95-namespace mrn = mir::report::null;
96-
97-void mrn::DisplayReport::report_successful_setup_of_native_resources() {}
98-void mrn::DisplayReport::report_successful_egl_make_current_on_construction() {}
99-void mrn::DisplayReport::report_successful_egl_buffer_swap_on_construction() {}
100-void mrn::DisplayReport::report_successful_drm_mode_set_crtc_on_construction() {}
101-void mrn::DisplayReport::report_successful_display_construction() {}
102-void mrn::DisplayReport::report_drm_master_failure(int) {}
103-void mrn::DisplayReport::report_vt_switch_away_failure() {}
104-void mrn::DisplayReport::report_vt_switch_back_failure() {}
105-void mrn::DisplayReport::report_egl_configuration(EGLDisplay, EGLConfig) {}
106-void mrn::DisplayReport::report_vsync(unsigned int) {}
107+#include "mir/graphics/null_display_report.h"
108+
109+namespace mir { namespace graphics {
110+
111+void NullDisplayReport::report_successful_setup_of_native_resources() {}
112+void NullDisplayReport::report_successful_egl_make_current_on_construction() {}
113+void NullDisplayReport::report_successful_egl_buffer_swap_on_construction() {}
114+void NullDisplayReport::report_successful_drm_mode_set_crtc_on_construction() {}
115+void NullDisplayReport::report_successful_display_construction() {}
116+void NullDisplayReport::report_drm_master_failure(int) {}
117+void NullDisplayReport::report_vt_switch_away_failure() {}
118+void NullDisplayReport::report_vt_switch_back_failure() {}
119+void NullDisplayReport::report_egl_configuration(EGLDisplay, EGLConfig) {}
120+void NullDisplayReport::report_vsync(unsigned int) {}
121+
122+}} // namespace mir::graphics
123
124=== modified file 'src/platform/symbols.map'
125--- src/platform/symbols.map 2016-08-22 14:36:40 +0000
126+++ src/platform/symbols.map 2016-09-07 03:00:58 +0000
127@@ -270,6 +270,9 @@
128 android::isEligibleBuiltInKeyboard*;
129 __android_log_assert;
130 __android_log_print;
131+
132+ vtable?for?mir::graphics::NullDisplayReport*;
133+ mir::graphics::NullDisplayReport*;
134 };
135 local: *;
136 };
137
138=== modified file 'src/platforms/android/server/device_quirks.cpp'
139--- src/platforms/android/server/device_quirks.cpp 2016-08-07 20:38:35 +0000
140+++ src/platforms/android/server/device_quirks.cpp 2016-09-07 03:00:58 +0000
141@@ -18,7 +18,7 @@
142
143 #include <GLES2/gl2.h>
144 #include <EGL/egl.h>
145-#include "mir/graphics/display_report.h"
146+#include "mir/graphics/null_display_report.h"
147 #include "mir/graphics/gl_config.h"
148 #include "gl_context.h"
149 #include "device_quirks.h"
150@@ -100,19 +100,7 @@
151 return false;
152 }
153
154-struct NullReport : mg::DisplayReport
155-{
156- void report_successful_setup_of_native_resources() override {};
157- void report_successful_egl_make_current_on_construction() override {};
158- void report_successful_egl_buffer_swap_on_construction() override {};
159- void report_successful_display_construction() override {};
160- void report_egl_configuration(EGLDisplay, EGLConfig) override {};
161- void report_vsync(unsigned int) override {};
162- void report_successful_drm_mode_set_crtc_on_construction() override {};
163- void report_drm_master_failure(int) override {};
164- void report_vt_switch_away_failure() override {};
165- void report_vt_switch_back_failure() override {};
166-} report;
167+mg::NullDisplayReport report;
168
169 struct Config : mg::GLConfig
170 {
171
172=== modified file 'src/platforms/android/utils/render_overlays.cpp'
173--- src/platforms/android/utils/render_overlays.cpp 2016-08-16 19:34:28 +0000
174+++ src/platforms/android/utils/render_overlays.cpp 2016-09-07 03:00:58 +0000
175@@ -26,7 +26,7 @@
176 #include "mir/graphics/graphic_buffer_allocator.h"
177 #include "mir/graphics/buffer_properties.h"
178 #include "mir/graphics/gl_config.h"
179-#include "mir/graphics/display_report.h"
180+#include "mir/graphics/null_display_report.h"
181 #include "mir/renderer/gl/render_target.h"
182 #include "mir/renderer/sw/pixel_source.h"
183 #include "mir_image.h"
184@@ -226,19 +226,6 @@
185 int stencil_buffer_bits() const override { return 0; }
186 };
187
188-struct DisplayReport : mg::DisplayReport
189-{
190- void report_successful_setup_of_native_resources() override {}
191- void report_successful_egl_make_current_on_construction() override {}
192- void report_successful_egl_buffer_swap_on_construction() override {}
193- void report_successful_display_construction() override {}
194- void report_egl_configuration(EGLDisplay, EGLConfig) override {}
195- void report_vsync(unsigned int) override {}
196- void report_successful_drm_mode_set_crtc_on_construction() override {}
197- void report_drm_master_failure(int) override {}
198- void report_vt_switch_away_failure() override {}
199- void report_vt_switch_back_failure() override {}
200-};
201 }
202
203 int main(int argc, char const** argv)
204@@ -265,7 +252,7 @@
205
206 auto platform_fn = platform_library->load_function<mg::CreateHostPlatform>(
207 "create_host_platform", MIR_SERVER_GRAPHICS_PLATFORM_VERSION);
208- auto platform = platform_fn(options, nullptr, std::make_shared<DisplayReport>());
209+ auto platform = platform_fn(options, nullptr, std::make_shared<mg::NullDisplayReport>());
210
211 //Strange issues going on here with dlopen() + hybris (which uses gnu_indirect_functions)
212 //https://github.com/libhybris/libhybris/issues/315
213
214=== modified file 'src/server/report/null/CMakeLists.txt'
215--- src/server/report/null/CMakeLists.txt 2016-07-28 23:00:22 +0000
216+++ src/server/report/null/CMakeLists.txt 2016-09-07 03:00:58 +0000
217@@ -3,7 +3,6 @@
218
219 compositor_report.cpp
220 connector_report.cpp
221- display_report.cpp
222 input_report.cpp
223 message_processor_report.cpp
224 null_report_factory.cpp
225
226=== modified file 'src/server/report/null/null_report_factory.cpp'
227--- src/server/report/null/null_report_factory.cpp 2016-07-28 23:00:22 +0000
228+++ src/server/report/null/null_report_factory.cpp 2016-09-07 03:00:58 +0000
229@@ -22,11 +22,11 @@
230 #include "connector_report.h"
231 #include "message_processor_report.h"
232 #include "session_mediator_report.h"
233-#include "display_report.h"
234 #include "input_report.h"
235 #include "seat_report.h"
236 #include "shell_report.h"
237 #include "scene_report.h"
238+#include "mir/graphics/null_display_report.h"
239 #include "mir/logging/null_shared_library_prober_report.h"
240
241 std::shared_ptr<mir::compositor::CompositorReport> mir::report::NullReportFactory::create_compositor_report()
242@@ -36,7 +36,7 @@
243
244 std::shared_ptr<mir::graphics::DisplayReport> mir::report::NullReportFactory::create_display_report()
245 {
246- return std::make_shared<null::DisplayReport>();
247+ return std::make_shared<mir::graphics::NullDisplayReport>();
248 }
249
250 std::shared_ptr<mir::scene::SceneReport> mir::report::NullReportFactory::create_scene_report()
251
252=== modified file 'tests/unit-tests/platforms/nested/test_nested_display.cpp'
253--- tests/unit-tests/platforms/nested/test_nested_display.cpp 2016-05-04 20:53:03 +0000
254+++ tests/unit-tests/platforms/nested/test_nested_display.cpp 2016-09-07 03:00:58 +0000
255@@ -19,7 +19,7 @@
256 #include "src/server/graphics/nested/display.h"
257 #include "src/server/graphics/nested/host_connection.h"
258 #include "src/server/graphics/nested/host_surface.h"
259-#include "src/server/report/null/display_report.h"
260+#include "mir/graphics/null_display_report.h"
261 #include "mir/graphics/default_display_configuration_policy.h"
262 #include "src/server/input/null_input_dispatcher.h"
263 #include "mir_display_configuration_builder.h"
264@@ -77,7 +77,7 @@
265
266 testing::NiceMock<mtd::MockEGL> mock_egl;
267 mi::NullInputDispatcher null_input_dispatcher;
268- mir::report::null::DisplayReport null_display_report;
269+ mg::NullDisplayReport null_display_report;
270 mg::CloneDisplayConfigurationPolicy default_conf_policy;
271 mtd::StubGLConfig stub_gl_config;
272 std::shared_ptr<mtd::NullPlatform> null_platform{

Subscribers

People subscribed via source and target branches