649274 - MeeGo Display turns off after 60 seconds without touch

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeremias Bosch (:jbos)]

As a user I want to see a website without touching the display all the time.

i.e.: While watching a movie or reading a text.

MeeGo provides for this the qmsystem2 API. This allows us to set a Time (seconds) to tell the system when to DIM and finally turn of the display.

Comment 1

[Jeremias Bosch (:jbos)]

Attachment: Adds option to change display dim / blank time by adding / adjusting a pref.

you need to have libqmsystem2-dev installed. to use this feature. Ensure you also have sensor framework dependency installed when you face linker issues.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Adding preferences to control screen doesn't seem the way to fix it. See bug 616664 and try to add support for MeeGo.

Comment 3

[Jeremias Bosch (:jbos)]

thanks this looks pretty cool. Didn't know there are such interfaces. :) Indeed we need to support this for MeeGo. 

This here is more a general thing since the default dim time is actually not 60 but 30 seconds on meego. So increasing it by default is really one part of the puzzle.

Of course disable turning of in case of video is not solved by the code above and should be handled with the interface you show in 616664.

I would take action there tomorrow and implement those interfaces. 

It would be an option to think about a general Display Manager in qt widgets which is taking care not only about blanking / dimming, but also giving the hint and bridge in case user force to blank screen (i.e. reacting by not making screenupdates anymore and start saving power / cpu / gpu time)

What do you think?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)

> This here is more a general thing since the default dim time is actually not 60
> but 30 seconds on meego. So increasing it by default is really one part of the
> puzzle.

I don't think we want to change the default, do we? If the OS uses 30 sec, we should let the user use the OS controls to change it.

> It would be an option to think about a general Display Manager in qt widgets
> which is taking care not only about blanking / dimming, but also giving the
> hint and bridge in case user force to blank screen (i.e. reacting by not making
> screenupdates anymore and start saving power / cpu / gpu time)

Maybe. I just don't want to add more than we need to add. Adding support for power consumption is a good thing and we should have general code for all platforms to handle power consumption better.

Comment 5

[Jeremias Bosch (:jbos)]

The longer i think about it,... mhm yes  a) fix issue with dimming correctly by using interfaces makes much sense esp since you are able to react to the state/option the browser is currently in.

b) fixing powerconsumption /usetime in another bug and more general makes also lot of sense and would just create 'another monster bug :)' 

Next Action: Create Patch that impl. interfaces from 616664 for meego. 

:) can someone with more rights then myself obsolete the patch from above?

Comment 6

[Jeremias Bosch (:jbos)]

Attachment: Using the Intefaces

Its sad that there is no api interface to just keep display on. Also nothing to reset to old (system) values since you do overwrite them!

Only way to reset to system is to delete the object. I tested first with 1 day timeout, which resulted in non working. So I reduced to 3 hours which seems enough. And which does work.

Comment 7

[Jeremias Bosch (:jbos)]

Attachment: Fixed Chat Comments

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 526056 [details] [diff] [review]
Fixed Chat Comments


> nsScreenQt::nsScreenQt(int aScreen)
>     : mScreen(aScreen)
>+    , mDisplayState(nsnull)

I guess this should be inside ifdef

> {
>     // nothing else to do. I guess we could cache a bunch of information
>     // here, but we want to ask the device at runtime in case anything
>@@ -61,11 +71,16 @@
> 
> nsScreenQt::~nsScreenQt()
> {
>-    // nothing to see here.
>+    delete mDisplayState;
>+    mDisplayState = nsnull;
> }
> 
also this one

Because this one is inside:
>+#ifdef MOZ_ENABLE_QMSYSTEM2
>+  void ApplyMinimumBrightness(PRUint32 aType);
>+private:
>+  MeeGo::QmDisplayState* mDisplayState;
>+#endif
> private:
>   int mScreen;
> };

send patch to try and make sure that it is not breaking default builds and qt builds.

Comment 9

[Jeremias Bosch (:jbos)]

Attachment: Fixed Comments

Thanks,

i pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=3c8283280dcb

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 526201 [details] [diff] [review]
Fixed Comments

>+      MOZ_QT_LIBS="$MOZ_QT_LIBS $_QMSYSTEM2_LIBS"
>+      AC_DEFINE(MOZ_ENABLE_QMSYSTEM2)
please make it 4 spaces indent (rest of Qt configure stuff need to be restyled later)


>+
>+const int DISPLAY_BLANK_TIMEOUT = 10800; /*3 * 60 * 60 seconds*/
>+const int DISPLAY_DIM_TIMEOUT = 10620; /*3 * 59 * 60 seconds*/
                                          spaces              spaces
>+
>+#endif
>+
>+
>+    if( aType == BRIGHTNESS_FULL) {
      spaces
>+        mDisplayState = new MeeGo::QmDisplayState();
>+
>+        // no way to keep display from blanking than setting a huge timeout
>+        // parameter is seconds. setting timeout to huge time this should work for 99.9% of our usecases
>+        mDisplayState->setDisplayBlankTimeout( DISPLAY_BLANK_TIMEOUT /*in seconds*/ );
>+        mDisplayState->setDisplayDimTimeout( DISPLAY_DIM_TIMEOUT /*in seconds*/ );
                                                                         spaces
>+        mDisplayState->setDisplayBrightnessValue( mDisplayState->getMaxDisplayBrightnessValue() );
                                           no spaces
>+        mDisplayState->set(MeeGo::QmDisplayState::On);
>+     }

>@@ -53,7 +65,12 @@
> 
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSISCREEN
>-
>+  
here and in other places don't add whitespaces

>+#ifdef MOZ_ENABLE_QMSYSTEM2
>+  void ApplyMinimumBrightness(PRUint32 aType);
>+private:

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 526201 [details] [diff] [review]
Fixed Comments

ok, this is looks good except style nits,

Comment 12

[Oleg Romashin (:romaxa)]

Comment on attachment 526201 [details] [diff] [review]
Fixed Comments

need add review for configure part

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 526201 [details] [diff] [review]
Fixed Comments

Review of attachment 526201 [details] [diff] [review]:
-----------------------------------------------------------------

The build config part looks good.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 526201 [details] [diff] [review]
Fixed Comments

># HG changeset patch
># Parent d6bf54d13be214b0b005cf2abb0c19a22447c19c
># User Jeremias Bosch <[e-mail]>
                                           ^
That looks like a typo ____________________|

Would you mind posting a version with that fixed, and with a checkin comment added, per 
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
?

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: Fixed headers, and removed white spaces, PUSHME

Comment 16

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/projects/cedar/rev/17ecc803b4bd

Comment 17

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/17ecc803b4bd

Comment 18

[Anna (Waverley)]

can someone having a MeeGo phone verify this, please?

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

MeeGo work QA-