On Mon, 24 Aug 2015 19:26:48 -0700 Bryce Harrington <br...@osg.samsung.com> wrote:
> On Thu, Aug 13, 2015 at 01:53:05PM +0300, Pekka Paalanen wrote: > > From: Giulio Camuffo <giuliocamu...@gmail.com> > > This is a big patch with a lot of changes, and I'm worried about landing > it right as we're on the eve of beta. If it could be broken up into > smaller easy-to-review bits, it might make it more digestible... > > That said, I like where this is going. I'd be totally okay with landing > it post-release. > > > --- > > Makefile.am | 3 + > > src/compositor-drm.c | 234 > > ++++++++++++++++----------------------------------- > > src/compositor-drm.h | 89 ++++++++++++++++++++ > > src/main.c | 127 +++++++++++++++++++++++++++- > > 4 files changed, 292 insertions(+), 161 deletions(-) > > create mode 100644 src/compositor-drm.h ... > Nice refactoring. With this change, this function also becomes quite > simple to write a test case for... just pass in various strings and > verify the correct int is returned. > > > @@ -2127,31 +2086,29 @@ get_gbm_format_from_section(struct > > weston_config_section *section, > > * Find the most suitable mode to use for initial setup (or > > reconfiguration on > > * hotplug etc) for a DRM output. > > * > > + * @param backend The DRM backend object > > * @param output DRM output to choose mode for > > - * @param kind Strategy and preference to use when choosing mode > > - * @param width Desired width for this output > > - * @param height Desired height for this output > > + * @param config Desired configuration for the output > > * @param current_mode Mode currently being displayed on this output > > - * @param modeline Manually-entered mode (may be NULL) > > * @returns A mode from the output's mode list, or NULL if none available > > */ > > static struct drm_mode * > > -drm_output_choose_initial_mode(struct drm_output *output, > > - enum output_config kind, > > - int width, int height, > > - const drmModeModeInfo *current_mode, > > - const drmModeModeInfo *modeline) > > +drm_output_choose_initial_mode(struct drm_backend *backend, > > + struct drm_output *output, > > + struct weston_drm_backend_output_config *config, > > + const drmModeModeInfo *current_mode) > > This function has a number of exit points, making it a really good > candidate for writing a thorough collection of test cases. The tricky > bits would be the two drm_output_add_mode() calls, which will need > mocks; initially though a first cut test could leave those two branches > as TODO. Hi Bryce and Jon, leaving all the other comments for later, there is one thing I'm curious about: How would you suggest we arrange the source code such that we can pick arbitrary functions for unit tests without making a copy of that function? It should be non-disruptive for the source code used for production, meaning that we cannot e.g. put #ifdef wrappers around every function. I think putting one function per file is also too much. I wouldn't want to make a copy and then later find out that we have been testing the copy while the code actually used in production has already changed. Maintaining a copy manually is a no-go. Thanks, pq
pgpKc6REv8cFc.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel