unity-mir

Merge lp://qastaging/~nick-dedekind/unity-mir/thready-screenshotting into lp://qastaging/unity-mir

thready-screenshotting
Merge into trunk

Proposed by Nick Dedekind on 2014-07-10

Status:	Merged
Approved by:	Gerry Boland on 2014-07-15
Approved revision:	246
Merged at revision:	241
Proposed branch:	lp://qastaging/~nick-dedekind/unity-mir/thready-screenshotting
Merge into:	lp://qastaging/unity-mir
Diff against target:	212 lines (+145/-1) 3 files modified src/modules/Unity/Application/application.cpp (+17/-1) src/modules/Unity/Application/application.h (+3/-0) tests/application_manager_test.cpp (+125/-0)
To merge this branch:	bzr merge lp://qastaging/~nick-dedekind/unity-mir/thready-screenshotting
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Gerry Boland (community)		2014-07-10	Approve on 2014-07-15
PS Jenkins bot (community)	continuous-integration		Approve on 2014-07-10
Review via email: mp+226282@code.qastaging.launchpad.net

Commit message

Fixed threading issue when screen-shotting application about to be stopped.

Description of the change

* Are there any related MPs required for this MP to build/function as expected? Please list.
No

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

lp://qastaging/~nick-dedekind/unity-mir/thready-screenshotting updated on 2014-07-10

243. By Nick Dedekind on 2014-07-10: use QMutex
244. By Nick Dedekind on 2014-07-10: use qt shared/weak ptr

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-10:

PASSED: Continuous integration, rev:243
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/392/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/56
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/56
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/56/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/56

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/392/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-10:

PASSED: Continuous integration, rev:244
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/393/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/57
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/57
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/57/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/57

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/393/rebuild

review: Approve (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2014-07-10:

+QMutex screenshotMutex;
does this need to live outside the namespace?

+ QMutexLocker lk(&screenshotMutex);
a quick 1 line comment explaining why we're doing this would help the casual reader see why we block in a deconstructor

Code looks good otherwise, am getting ready to test

review: Needs Fixing

lp://qastaging/~nick-dedekind/unity-mir/thready-screenshotting updated on 2014-07-10

245. By Nick Dedekind on 2014-07-10: added comment

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-07-10:

> +QMutex screenshotMutex;
> does this need to live outside the namespace?

Yes, so it can be used in the callback when the application is deleted (member would be deleted).

>
> + QMutexLocker lk(&screenshotMutex);
> a quick 1 line comment explaining why we're doing this would help the casual
> reader see why we block in a deconstructor

Done.

>
> Code looks good otherwise, am getting ready to test

lp://qastaging/~nick-dedekind/unity-mir/thready-screenshotting updated on 2014-07-10

246. By Nick Dedekind on 2014-07-10: moved mutex into namespace

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-07-10:

> > +QMutex screenshotMutex;
> > does this need to live outside the namespace?
>
> Yes, so it can be used in the callback when the application is deleted (member
> would be deleted).

Sorry, misunderstood. no, moved it inside.

>
> >
> > + QMutexLocker lk(&screenshotMutex);
> > a quick 1 line comment explaining why we're doing this would help the casual
> > reader see why we block in a deconstructor
>
> Done.
>
> >
> > Code looks good otherwise, am getting ready to test