977546 - cleanup wake lock code in winrt widget

Status: VERIFIED FIXED

(Core Graveyard :: Widget: WinRT, defect, P1)

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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);

Comment 4

[Marco Mucci [:MarcoM]]

Hey Jim, can you provide a point value.

Comment 5

[Jim Mathies [:jimm]]

Attachment: updated patch

updated per comments, and I did some other cleanup including splitting this class out into its own files.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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

Comment 7

[Jim Mathies [:jimm]]

Attachment: export

Comment 8

[Jim Mathies [:jimm]]

(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!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f61c4ec14f0a

Comment 10

[Kamil Jozwiak [:kjozwiak]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f61c4ec14f0a

Comment 12

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 13

[Kamil Jozwiak [:kjozwiak]]

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