AppMenu GTK+

Merge lp://staging/~charlesk/appmenu-gtk/lp-787736 into lp://staging/appmenu-gtk/12.10

  1. lp-787736
  2. Merge into trunk.12.10
Proposed by Charles Kerr
Status: Merged
Merged at revision: 158
Proposed branch: lp://staging/~charlesk/appmenu-gtk/lp-787736
Merge into: lp://staging/appmenu-gtk/12.10
Diff against target: 218 lines (+90/-79)
1 file modified
src/bridge.c (+90/-79)
To merge this branch: bzr merge lp://staging/~charlesk/appmenu-gtk/lp-787736
Reviewer Review Type Date Requested Status
jenkins (community) continuous-integration Needs Fixing
Lars Karlitski (community) Approve
Indicator Applet Developers Pending
Review via email: mp+103709@code.staging.launchpad.net

Commit message

Fix a memory leak -- RebuildStructs were being leaked. (LP #787736)

Description of the change

Putting this fix in a standalone branch, without a half-dozen surrounding interwoven cleanup commits, to make it easier to cherry pick

To post a comment you must log in.
Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:154
http://s-jenkins:8080/job/appmenu-gtk-ci/1/

review: Approve (continuous-integration)
Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:156
http://jenkins.qa.ubuntu.com/job/appmenu-gtk-ci/6/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/appmenu-gtk-ci/./label=precise/6/console

review: Approve (continuous-integration)
Revision history for this message
Lars Karlitski (larsu) wrote :

That's much cleaner, I like it.

* rebuild_list_clear is called in both dispose and in finalize

* rebuild_timer_func is the callback for the timer to rebuild widgets. It calls rebuild_list_clear, which removes the timer source with g_source_remove. Is removing sources in their callbacks allowed?

* you could be making life easier for this particular reviewer if you return G_SOURCE_REMOVE instead of FALSE in rebuild_timer_func (he has trouble remembering if TRUE or FALSE remove a source)

review: Needs Information
Revision history for this message
Charles Kerr (charlesk) wrote :

* wrt rebuild_list_clear being called twice, yes. I was continuing the double-clear behavior that's been in place since http://bazaar.launchpad.net/~canonical-dx-team/appmenu-gtk/trunk.12.10/revision/119.3.3 but it's not clear to me if the original behavior was necessary or accidental.

* removing sources in their callbacks works fine.

* Ah, G_SOURCE_REMOVE is a nice mnemonic device. I didn't know about that!

Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:158
http://jenkins.qa.ubuntu.com/job/appmenu-gtk-ci/7/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/appmenu-gtk-ci/./label=precise/7/console

review: Approve (continuous-integration)
Revision history for this message
Lars Karlitski (larsu) wrote :

Thanks for the info!

review: Approve
Revision history for this message
jenkins (martin-mrazik+qa) wrote :

FAILED: Autolanding.
No commit message was specified.
http://jenkins.qa.ubuntu.com/job/appmenu-gtk-autolanding/3/

review: Needs Fixing (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) wrote :

(That failure message from jenkins looks like a false negative. Plus, the same code passed twelve hours ago.)

Revision history for this message
jenkins (martin-mrazik+qa) wrote :

FAILED: Autolanding.
No commit message was specified.
http://jenkins.qa.ubuntu.com/job/appmenu-gtk-autolanding/4/

review: Needs Fixing (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) wrote :

:/

Revision history for this message
Allan LeSage (allanlesage) wrote :

Charles here's the link to that dput-failure: go ahead and post a commit message and I'll re-start the job: http://s-jenkins/job/generic-land/346/console

Revision history for this message
Charles Kerr (charlesk) wrote :

Landing this manually after talking over the CI problem with Allan.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches