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

NEW
Unassigned

Status

8 years ago
8 years ago

People

(Reporter: jon.hemming, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
ARM
Maemo
Dependency tree / graph

Details

(Whiteboard: maemo6)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
OS: Linux → Maemo
Hardware: Other → ARM
(Reporter)

Comment 1

8 years ago
Created attachment 458982 [details] [diff] [review]
Notifies observers about display state changes.
Attachment #458982 - Flags: review?(azakai)
DougT might want to take a look at this too.

Comment 3

8 years ago
Jon, how does this bug relate to bug 581314? The patches share code.
(Reporter)

Comment 4

8 years ago
(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 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
Attachment #458982 - Flags: review-

Updated

8 years ago
Blocks: 583135

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 6

8 years ago
Created attachment 469040 [details] [diff] [review]
Updated patch
Attachment #458982 - Attachment is obsolete: true
Attachment #469040 - Flags: feedback?(azakai)
Attachment #458982 - Flags: review?(azakai)

Updated

8 years ago
Attachment #469040 - Flags: feedback?(azakai) → review?(azakai)

Updated

8 years ago
Attachment #469040 - Flags: review?(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?
Attachment #469040 - Flags: review?(romaxa) → review-
(Reporter)

Comment 8

8 years ago
Created attachment 469491 [details] [diff] [review]
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.
Attachment #469040 - Attachment is obsolete: true
Attachment #469491 - Flags: review?(romaxa)
Attachment #469040 - Flags: review?(azakai)
(Reporter)

Comment 9

8 years ago
Created attachment 470387 [details] [diff] [review]
Adds also MozMaemo6SignalHandler class that was missing from previous patch
Attachment #469491 - Attachment is obsolete: true
Attachment #470387 - Flags: review?(romaxa)
Attachment #469491 - Flags: review?(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?
Attachment #470387 - Flags: review?(romaxa) → review-
(Reporter)

Comment 11

8 years ago
Created attachment 471064 [details] [diff] [review]
Updated patch
Attachment #470387 - Attachment is obsolete: true
Attachment #471064 - Flags: review?(romaxa)
(Reporter)

Comment 12

8 years ago
(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.
can we get a new patch attached?
tracking-fennec: --- → ?
(Reporter)

Comment 14

8 years ago
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.
Can we get link to public SDK qmsystem headers? anywhere
(Reporter)

Comment 16

8 years ago
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.
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.

Updated

8 years ago
tracking-fennec: ? → 2.0+
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
Attachment #471064 - Flags: feedback?(doug.turner)
(Reporter)

Comment 20

8 years ago
(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.
(Reporter)

Updated

8 years ago
Blocks: 596546
right jon.  lets remove them then!

Updated

8 years ago
Attachment #471064 - Flags: review?(romaxa)
Attachment #471064 - Flags: review-
Attachment #471064 - Flags: feedback?(doug.turner)
Attachment #471064 - Flags: feedback-
(Reporter)

Comment 22

8 years ago
Created attachment 475781 [details] [diff] [review]
Updated to not notify about dimmed state anymore
Attachment #471064 - Attachment is obsolete: true
Attachment #475781 - Flags: review?(romaxa)
Attachment #475781 - Flags: feedback?(doug.turner)
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 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
Attachment #475781 - Flags: review?(romaxa)
Attachment #475781 - Flags: review-
Attachment #475781 - Flags: feedback?(doug.turner)
Attachment #475781 - Flags: feedback-
(Reporter)

Comment 25

8 years ago
(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?
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?
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.
(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..

Updated

8 years ago
Assignee: nobody → doug.turner
tracking-fennec: 2.0+ → 2.0b3+
I am pushing this out.
tracking-fennec: 2.0b3+ → 2.0+

Updated

8 years ago
Assignee: doug.turner → nobody

Updated

8 years ago
Whiteboard: maemo6

Updated

8 years ago
tracking-fennec: 2.0+ → 2.0-
You need to log in before you can comment on or make changes to this bug.