<html> <head> <base href="https://bugzilla.gnome.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - memory leak" href="https://bugzilla.gnome.org/show_bug.cgi?id=761312#c46">Comment # 46</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - memory leak" href="https://bugzilla.gnome.org/show_bug.cgi?id=761312">bug 761312</a> from <a href="page.cgi?id=describeuser.html&login=rstrode%40redhat.com" title="Ray Strode [halfline] <rstrode@redhat.com>"> Ray Strode [halfline]</a> <pre>(In reply to Jonas Ådahl from <a href="show_bug.cgi?id=761312#c45">comment #45</a>) > I do however think that the two patches before this would be > better squashed into this one, since the intermediate state > isn't really an improvement (looks like we'll just always > draw on the live buffer, but maybe I'm wrong). Well, I split it out because the patch is kind of unwieldy. The cursor changes are really just a casualty of the other code, so i'd rather it didn't get thrown into the same patch. Cutting out the set_busy stuff up front doesn't make the code worse, we're already always drawing on a live buffer we aren't supposed to. That's because the temporary buffer it allocated in begin_paint and blitted out in end_paint, which are always bracketed together when used, so there's no chance a release will ever before the shared buffer is drawn to. So i'm just cutting out broken code, and having that rescission in the same patch as the fix makes the fix harder to follow. Let me propose a compromise, I'll do it as separate commits, but when I push them I'll first do: $ git rebase master $ git checkout master $ git merge --no-ff --no-commit mybranch $ git commit and give a merge commit with a summary message of what's changed. Then when users look at the merge commit with cgit or git they will see all the patches squashed together, alongside a nice message, but they can also look at the individual commits separately, if they need to. sound good? > +static const cairo_user_data_key_t gdk_wayland_cairo_key; > > Please call this something other than the gdk_wayland_cairo_key in > gdkdisplay-wayland.c. Sure. It doesn't matter in practice, since the keys are static, of course, but I don't have a problem tidying this up for clarity sake. > @@ +668,3 @@ > If we get a wl_buffer.release after a ..update_size() we'd set the staging > cairo surface to the committed surface which has the wrong size. > > Simply unsetting impl->committed_cairo_surface on update_size() should fix > that I suppose, Yup, will make that fix. > but you'd also have to unset the user_data so it can destroy > gracefully even on GdkWindow tear down. > > Doing that I think could just drop impl->backfill_cairo_surface and just > always use impl->committed_surface (and always "detach" it i.e. unset > user_data) when we've read back its content. Not sure that's better than > using a separate reference ala fillback_cairo_surface though. It's an interesting question. Thinking about it, I think having two different members for the two different references makes sense, if for no other reason than, it makes inspecting what's going on in gdb easier. You can infer more about the state of things at a breakpoint with the separate committed surface reference. We could go a step further and stuff all unreleased references in a hash table, but maybe that's overkill. Though, rather than unsetting user data and detaching the surface from the buffer we could just ref the impl so it sticks around until all outstanding releases are processed. Then if we do want to do the hash table down the road, we'll be positioned to be able to do it.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>