On Tue, 27 Mar 2018 12:43:36 -0500 Derek Foreman <der...@osg.samsung.com> wrote:
> Xwayland clients can offer multiple icon sizes in no particular order. > Previously xwm was selecting the first one unconditionally. This patch > selects the icon that matches the size closest to the target size. The > target size is hard coded to 16 since there is only one theme and the > data used to create the theme is hard coded. > > Co-authored-by: Scott Moreau <ore...@gmail.com> > Missing signed-off-by. Scott, are you happy with your attribution here? > --- > > Changed in v2: > > - Fix typo setting width to height > > Changed in v3: > > - Move checks for malformed input into data handling function > > Changed in v4: > > - #define XWM_ICON_SIZE in this patch and do not #undef it > - Start with first icon found before choosing icon\ > - Check for NULL data in get_icon_size_from_data() > > Changed in v5: > - remove allocations, process in a single pass > - rebase on top of memory leak fix > > xwayland/window-manager.c | 51 > ++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 7 deletions(-) > > diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c > index dad117fa..5209ac66 100644 > --- a/xwayland/window-manager.c > +++ b/xwayland/window-manager.c > @@ -127,6 +127,8 @@ struct motif_wm_hints { > #define _NET_WM_MOVERESIZE_MOVE_KEYBOARD 10 /* move via keyboard */ > #define _NET_WM_MOVERESIZE_CANCEL 11 /* cancel operation */ > > +#define XWM_ICON_SIZE 16 /* width and height of frame icon */ > + > struct weston_output_weak_ref { > struct weston_output *output; > struct wl_listener destroy_listener; > @@ -1352,6 +1354,44 @@ weston_wm_window_schedule_repaint(struct > weston_wm_window *window) > weston_wm_window_do_repaint, window); > } > > +static uint32_t * > +get_icon_size_from_data(int target_width, int target_height, > + uint32_t *width, uint32_t *height, > + uint32_t length, uint32_t *data) This may be a static function, but it really needs doxygen doc to explain all the arguments and return value, particularly for 'length', see below. > +{ > + uint32_t *d = data, *ret = NULL, w, h; > + int a1, a2, a3; I'd really like better names than a1, a2, a3. There are so many variables in this function that they need descriptive names. > + > + if (!data) > + return NULL; > + > + /* Choose closest match by comparing icon dimension areas */ > + a1 = target_width * target_height; > + > + *width = *height = 0; > + > + while (d - data < length / 4) { Maybe using uint32_t *end = data + length; would make this easier to read. Isn't length/4 wrong, because length is already in uint32_t units by the caller? > + w = *d++; > + h = *d++; > + > + /* Some checks against malformed input. */ > + if (w == 0 || h == 0 || length < 2 + w * h) Checking against 'length' is incorrect, because we need to be checking against the remaining length, not against the whole length, as we may have skipped an icon alrady. Checking against 'end' would help here too. > + return ret; > + > + a2 = w * h; > + a3 = *width * *height; > + if (!a3 || abs(a2 - a1) < abs(a3 - a1)) { > + *width = w; > + *height = h; > + ret = d; > + } > + > + d += a2; The computations here seem to be correct. > + } > + > + return ret; > +} > + > static void > handle_icon_surface_destroy(void *data) > { > @@ -1367,9 +1407,6 @@ weston_wm_handle_icon(struct weston_wm *wm, struct > weston_wm_window *window) > uint32_t *data, width, height; > cairo_surface_t *new_surface; > > - /* TODO: icons don’t have any specified order, we should pick the > - * closest one to the target dimension instead of the first one. */ > - > cookie = xcb_get_property(wm->conn, 0, window->id, > wm->atom.net_wm_icon, XCB_ATOM_ANY, 0, > UINT32_MAX); Type is XCB_ATOM_ANY. The context here is missing to show this: reply = xcb_get_property_reply(wm->conn, cookie, NULL); length = xcb_get_property_value_length(reply); /* This is in 32-bit words, not in bytes. */ if (length < 2) { free(reply); return; } data = xcb_get_property_value(reply); What are the units of 'length'? The comment here says words, but if you look at the example in 'man xcb_get_property', it is in bytes. If they are both right, then the length unit must depend on something. The type is not checked from the reply, and neither is 'format' checked from the reply. My wild guess would be that 'format' determines the units of 'length', and so it is controllable by the X11 client that set the property. I think weston_wm_window_read_properties() is equally very much broken against badly formatted properties, because it assumes a certain length from just the type or even atom name(!) without actually checking the length. dump_property() looks suspicious as well. It's probably everything that ever parses an xcb_get_property_reply_t in XWM. The EWMH spec may say that a certain property needs to have certain format and type, but I don't believe there is anything actually guaranteeing to follow the spec, so it's up to us to verify the data is proper. > @@ -1383,11 +1420,11 @@ weston_wm_handle_icon(struct weston_wm *wm, struct > weston_wm_window *window) > } > > data = xcb_get_property_value(reply); > - width = *data++; > - height = *data++; > > - /* Some checks against malformed input. */ > - if (width == 0 || height == 0 || length < 2 + width * height) { > + data = get_icon_size_from_data(XWM_ICON_SIZE, XWM_ICON_SIZE, > + &width, &height, length, data); > + > + if (!data) { > free(reply); > return; > } Thanks, pq
pgpPxGs_tJttX.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel