Closed
Bug 977546
Opened 10 years ago
Closed 10 years ago
cleanup wake lock code in winrt widget
Categories
(Core Graveyard :: Widget: WinRT, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla30
People
(Reporter: jimm, Assigned: jimm)
References
Details
(Whiteboard: p=1 s=it-30c-29a-28b.3 r=ff30 [qa+])
Attachments
(1 file, 3 obsolete files)
7.38 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → jmathies
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8382972 -
Attachment is obsolete: true
Attachment #8382973 -
Flags: review?(tabraldes)
Updated•10 years ago
|
Blocks: metrobacklog
Whiteboard: p=0
Comment 3•10 years ago
|
||
Comment on attachment 8382973 [details] [diff] [review] patch Review of attachment 8382973 [details] [diff] [review]: ----------------------------------------------------------------- Let's see if we can get rid of the static sWakeLockListener. What about having InitWakeLock return the WakeLockListener that it creates and having ShutdownWakeLock take a WakeLockListener* as a param? WakeLockListener *wakeLockListener = nullptr; ... case GeckoProcessType_Default: { wakeLockListener = InitWakeLock(); ... ShutdownWakeLock(wakeLockListener);
Attachment #8382973 -
Flags: review?(tabraldes) → review+
Comment 4•10 years ago
|
||
Hey Jim, can you provide a point value.
Status: NEW → ASSIGNED
Flags: needinfo?(jmathies)
QA Contact: kamiljoz
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(jmathies)
Whiteboard: p=0 → p=1
Updated•10 years ago
|
Priority: -- → P1
Whiteboard: p=1 → p=1 s=it-30c-29a-28b.3 r=ff30
Assignee | ||
Comment 5•10 years ago
|
||
updated per comments, and I did some other cleanup including splitting this class out into its own files.
Attachment #8382973 -
Attachment is obsolete: true
Attachment #8384827 -
Flags: review?(tabraldes)
Comment 6•10 years ago
|
||
Comment on attachment 8384827 [details] [diff] [review] updated patch Review of attachment 8384827 [details] [diff] [review]: ----------------------------------------------------------------- Instead of having InitWakeLock and ShutdownWakeLock, it might make sense to have a WakeLockListener::Initialize function (to take the place of InitWakeLock) and WakeLockListener::~WakeLockListener (to take the place of ShutdownWakeLock). ::: widget/windows/winrt/WakeLockListener.cpp @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "WakeLockListener.h" Trailing whitespace
Attachment #8384827 -
Flags: review?(tabraldes) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8384827 -
Attachment is obsolete: true
Attachment #8384936 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6) > Comment on attachment 8384827 [details] [diff] [review] > updated patch > > Review of attachment 8384827 [details] [diff] [review]: > ----------------------------------------------------------------- > > Instead of having InitWakeLock and ShutdownWakeLock, it might make sense to > have a WakeLockListener::Initialize function (to take the place of > InitWakeLock) and WakeLockListener::~WakeLockListener (to take the place of > ShutdownWakeLock). We can get to this at some other point. > ::: widget/windows/winrt/WakeLockListener.cpp > @@ +1,5 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +#include "WakeLockListener.h" > > Trailing whitespace fixed. Thanks!
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f61c4ec14f0a
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Quick note for QA verification & testing: This issue mostly relates to cleaning up code. I talked to Jim via IRC and he suggested quickly testing and making sure that the screen doesn't get turned off when watching videos in the browser while in full screen mode.
Whiteboard: p=1 s=it-30c-29a-28b.3 r=ff30 → p=1 s=it-30c-29a-28b.3 r=ff30 [qa+]
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f61c4ec14f0a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 12•10 years ago
|
||
For testing and verification. Reopen if any defects found.
Flags: needinfo?(kamiljoz)
Comment 13•10 years ago
|
||
Went through the verification process using the following build: - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-05-03-02-01-mozilla-central/ - Left a clip running for about 45 minutes while in full screen mode via YouTube - Left a clip running for about 45 minutes while in full screen mode via Dailymotion - Went through all of above test cases using both the X1 Carbon and the Surface Pro 2
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•