580588 - Notifications of screen state changes in Maemo 6 platform needs to be added.

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Jon Hemming]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060309 Ubuntu/8.04 (hardy) Firefox/3.0.11
Build Identifier: http://hg.mozilla.org/mozilla-central

Maemo 6 platform informs with signals about screen state changes: on, dimmed and off. Listener for these signals need to be implemented and notifications about these screen state changes needs to be sent to observers. 

Reproducible: Always

Comment 1

[Jon Hemming]

Attachment: Notifies observers about display state changes.

Comment 2

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

DougT might want to take a look at this too.

Comment 3

[Alon Zakai (:azakai)]

Jon, how does this bug relate to bug 581314? The patches share code.

Comment 4

[Jon Hemming]

(In reply to comment #3)
> Jon, how does this bug relate to bug 581314? The patches share code.

Yes, that is true. I just wasn't sure how to do this, since at the moment I think these are just proposals, especially bug 581314. I guess I should have put that bug to be dependent of this bug and diff against these changes so that they could have been used together. In bug 581314 we are using enable() function instead of start() function of nsNativeAppSupportQt which is used later in start-up and this enables us to use such services like nsObserveService. This connecting of signals could have still be done in start() function and then we could have done rest of the steps in enable() function, so this patch should be valid also.

Comment 5

[Oleg Romashin (:romaxa)]

Comment on attachment 458982 [details] [diff] [review]
Notifies observers about display state changes.

Please add qmsystem/qmdisplaystate.h to [js/src/]config/system-headers
otherwise it fail compile on scratchbox X86

Comment 6

[Jon Hemming]

Attachment: Updated patch

Comment 7

[Oleg Romashin (:romaxa)]

Comment on attachment 469040 [details] [diff] [review]
Updated patch

Style is seems to be broken here:
>+#if defined(MOZ_WIDGET_QT)
>+#if ( MOZ_PLATFORM_MAEMO == 6 )

+    MozMaemo6SignalHandler( QObject *parent = 0 );
+  Maemo::QmDisplayState *displayState = new Maemo::QmDisplayState(QApplication::instance());

and in many other places...
see https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

printfs are not allowed without ifdefs... use NS_WARN/ PR_LOG functionality

>--- a/js/src/config/system-headers	Thu Aug 05 22:13:07 2010 -0400
>+++ b/js/src/config/system-headers	Wed Aug 25 16:47:24 2010 +0300

you are adding to js/src, it should be also in a/config/system-headers

>+  if ( !stateChangeHandler || !displayState ) return NS_ERROR_FAILURE;
move return to the next line


>+  mStateChangeHandler = new MozMaemo6SignalHandler(QApplication::instance());
>+  mDisplayState = new Maemo::QmDisplayState(QApplication::instance());
You creating it here, where are you deleting these objects?

Comment 8

[Jon Hemming]

Attachment: Updated patch

Yes, sorry for my hasty patch before, this should now be inline with style. There were also some other mistakes in previous patch that are now fixed. Here we wont create anything with new anymore. Previously I just let Qt to handle deleting of those member variables, but that wasn't appropriate way in this case.

Comment 9

[Jon Hemming]

Attachment: Adds also MozMaemo6SignalHandler class that was missing from previous patch

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 470387 [details] [diff] [review]
Adds also MozMaemo6SignalHandler class that was missing from previous patch


>+ * The Original Code is Mozilla Communicator client code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.

Should be something like Nokia here.

>+
>+  nsCOMPtr<nsISupportsPRBool> observer_retval;

Why do you need it here? just set nsnull as object argument

>+
>+  if (aState == Maemo::QmDisplayState::On)
I think it make sense to use switch here

>+    appObserverService->NotifyObservers(observer_retval, "system-display-on", NULL);
>+  else if (aState == Maemo::QmDisplayState::Dimmed)
>+    appObserverService->NotifyObservers(observer_retval, "system-display-dimmed", NULL);
>+  else if (aState == Maemo::QmDisplayState::Off)
>+    appObserverService->NotifyObservers(observer_retval, "system-display-off", NULL);
>+}


>+ *
>+ * The Original Code is Mozilla Communicator client code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.

Same problem with license

>+
>+public:
>+  MozMaemo6SignalHandler(QObject* parent = 0);
Why do you need parent here?



>+#if (MOZ_PLATFORM_MAEMO == 6)
>+#include "MozMaemo6SignalHandler.h"
>+#include <QApplication>

I think QApplication should be outside of maemo6 define... why do you need qapplication here?


> public:
>+#if (MOZ_PLATFORM_MAEMO == 6)
>+  nsNativeAppSupportQt();
>+#endif
I think define is not needed here

>+
>   NS_IMETHOD Start(PRBool* aRetVal);
>   NS_IMETHOD Stop(PRBool* aResult);
>+#if (MOZ_PLATFORM_MAEMO == 6)
>+  NS_IMETHOD Enable();
Make this method available for Qt too. (move define inside Enable method)


>+
>+#if (MOZ_PLATFORM_MAEMO == 6)
>+nsNativeAppSupportQt::nsNativeAppSupportQt()
>+  : mStateChangeHandler(QApplication::instance()), mDisplayState(QApplication::instance())

Why do you need parent object here?

Comment 11

[Jon Hemming]

Attachment: Updated patch

Comment 12

[Jon Hemming]

(In reply to comment #10)
> >+ * The Original Code is Mozilla Communicator client code.
> >+ *
> >+ * The Initial Developer of the Original Code is
> >+ * Netscape Communications Corporation.
> 
> Should be something like Nokia here.
License corrected now.

> 
> >+
> >+  nsCOMPtr<nsISupportsPRBool> observer_retval;
> 
> Why do you need it here? just set nsnull as object argument
> 
removed that one.

> >+
> >+  if (aState == Maemo::QmDisplayState::On)
> I think it make sense to use switch here
> 
changed it to use switch

> >+
> >+public:
> >+  MozMaemo6SignalHandler(QObject* parent = 0);
> Why do you need parent here?
> 
Constructor and destructor removed since not needed.

> 
> >+#if (MOZ_PLATFORM_MAEMO == 6)
> >+#include "MozMaemo6SignalHandler.h"
> >+#include <QApplication>
> 
> I think QApplication should be outside of maemo6 define... why do you need
> qapplication here?
> 
Removed

> > public:
> >+#if (MOZ_PLATFORM_MAEMO == 6)
> >+  nsNativeAppSupportQt();
> >+#endif
> I think define is not needed here
> 
Removed constructor since not needed.

> >+
> >   NS_IMETHOD Start(PRBool* aRetVal);
> >   NS_IMETHOD Stop(PRBool* aResult);
> >+#if (MOZ_PLATFORM_MAEMO == 6)
> >+  NS_IMETHOD Enable();
> Make this method available for Qt too. (move define inside Enable method)
> 
done

> 
> >+
> >+#if (MOZ_PLATFORM_MAEMO == 6)
> >+nsNativeAppSupportQt::nsNativeAppSupportQt()
> >+  : mStateChangeHandler(QApplication::instance()), mDisplayState(QApplication::instance())
> 
> Why do you need parent object here?
I was developing this for MeeGo before and there was some bug before which needed application instance to be passed as parent for these objects before signals could be observed. I mistakenly adopted that to Qt side also, but that is not needed anymore even on MeeGo side. Removed.

Comment 13

[Doug Turner (:dougt)]

can we get a new patch attached?

Comment 14

[Jon Hemming]

Sorry for adding that patch and comment with a bit unclear way but that new patch is attachment 471064 [details] [diff] [review] and it has changes stated in comment 12.

Comment 15

[Oleg Romashin (:romaxa)]

Can we get link to public SDK qmsystem headers? anywhere

Comment 16

[Jon Hemming]

Hopefully. Btw, in unix "system-display-dimmed-or-off" is used and I'm suggesting "system-display-off" and "system-display-dimmed" to be sent separately. Is this ok? Other thing is that information about these state changes should also go to content since there is most probably situations where content would need to know about those also. Should that be added to this patch or do we wait until we find that kind of situation? I have tried about it by sending message to content and then notifying observer on that side also about display state changes.

Comment 17

[Doug Turner (:dougt)]

If you would like to expose this sort of event to web content, please file a separate bug and in it please outline use cases.  It isn't perfectly clear what a site would do when they got a "screen is dimmed" event.

Comment 18

[Oleg Romashin (:romaxa)]

Comment on attachment 471064 [details] [diff] [review]
Updated patch

>if you would like to expose this sort of event to web content, please file a
doug we are talking here about NotifyObserver events, not web-content events...

...

>separate bug and in it please outline use cases.  It isn't perfectly clear what
>a site would do when they got a "screen is dimmed" event.

I think this is not about web page, this is about what browser should do in dimmed state.
Do we know where and why dimmed state going to be used?

are we using existing notifications "system-display-on"... anywhere?  is it documented anywhere? can we easily change ObserverNotification event names without breaking other functionality?

dougt where we are using it on Maemo5?

+  if (currentDisplayState == Maemo::QmDisplayState::Off)
+    mStateChangeHandler.reactToDisplayStateChange(Maemo::QmDisplayState::Off);
{} here

Comment 19

[Doug Turner (:dougt)]

osso provided something in their hw callbacks.  We rebroadcast using the nsIObserverService:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsNativeAppSupportUnix.cpp

Specifically:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsNativeAppSupportUnix.cpp#299

Comment 20

[Jon Hemming]

(In reply to comment #18)
> Do we know where and why dimmed state going to be used?
> 
Dimmed state is not used anywhere at the moment and most probably it wont be needed either, so we don't need make notification about that.

Comment 21

[Doug Turner (:dougt)]

right jon.  lets remove them then!

Comment 22

[Jon Hemming]

Attachment: Updated to not notify about dimmed state anymore

Comment 23

[Oleg Romashin (:romaxa)]

Comment on attachment 475781 [details] [diff] [review]
Updated to not notify about dimmed state anymore

>#include <qmsystem/qmdisplaystate.h>


maybe I'm too annoying... but I still cannot find place where I can grab qmdisplaystate.h and related library sources...

Comment 24

[Doug Turner (:dougt)]

Comment on attachment 475781 [details] [diff] [review]
Updated to not notify about dimmed state anymore

i think we should remove all code related to detecting or retransmitting

Comment 25

[Jon Hemming]

(In reply to comment #24)
> Comment on attachment 475781 [details] [diff] [review]
> Updated to not notify about dimmed state anymore
> 
> i think we should remove all code related to detecting or retransmitting
Sorry, I don't quite follow. I guess you were meaning "detecting and retransmitting", but still, what is wrong with it? Information of display state changes is going to be needed. At least for setting the topmost tab to inactive when display goes off. Detecting and retransmitting will let this be done in centralized way so that we can get information from platforms in one place and use same notification for all of them. Same thing is done in unix code so why can't we do it in Qt Maemo code? Can you please give an alternative suggestion? How can we react to this Qt signal with a better way?

Comment 26

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

GTK Maemo uses the dimmed notifications to control use of the accelerometer, so we don't use too much power. Should we do the same for Qt?

Comment 27

[Doug Turner (:dougt)]

I am not sure if screen dimming is the right trigger.  Imagine you have a tab that is playing audio.  If the screen dims, I do not want that audio to stop.

mfinkle, i think we added that power saving feature without much data behind it. We should determine if it is a real problem.

Comment 28

[Alon Zakai (:azakai)]

(In reply to comment #26)
> GTK Maemo uses the dimmed notifications to control use of the accelerometer, so
> we don't use too much power. Should we do the same for Qt?

Hmm, if we do want something like this, maybe it should just be in a single central place in the code, and not GTK/Qt/etc..

Comment 29

[Doug Turner (:dougt)]

I am pushing this out.

Comment 30

[BMO Automation]

Closing all opened bug in a graveyard component