Closed Bug 759799 Opened 7 years ago Closed 7 years ago

Hal WakeLocks have no affect on Android

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox14 --- verified
firefox15 --- verified
firefox16 --- verified

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(1 file, 1 obsolete file)

We have support for manipulating wake locks in Hal, but it isn't hooked up on Android. We should make that work so we have a common place to do that sort of thing.
Attached patch Hook up Hal WakeLocks on Android (obsolete) — Splinter Review
Attachment #628363 - Flags: review?(blassey.bugs)
Comment on attachment 628363 [details] [diff] [review]
Hook up Hal WakeLocks on Android

Actually maybe kanru is a better reviewer for this, since he wrote the Hal parts.
Attachment #628363 - Flags: review?(blassey.bugs) → review?(kchen)
Comment on attachment 628363 [details] [diff] [review]
Hook up Hal WakeLocks on Android

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

::: widget/android/AndroidBridge.cpp
@@ +2299,5 @@
>  
> +void
> +AndroidBridge::NotifyWakeLockChanged(const nsAString& topic, int numLocked, int numHidden)
> +{
> +#if MOZ_JAVA_COMPOSITOR

I'd rather you either implement this in embedding/android/GeckoAppShell.java or at least stub it out and print something to the log if it gets called
Comment on attachment 628363 [details] [diff] [review]
Hook up Hal WakeLocks on Android

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

I think it is better to use PowerManagerService to manage the wake locks instead of hooking up hal directly. You can add a wake lock listener to the PowerManagerService and acquire the real android wake lock there.
Attachment #628363 - Flags: review?(kchen) → feedback-
Attachment #628363 - Attachment is obsolete: true
Comment on attachment 629212 [details] [diff] [review]
Hook up Hal WakeLocks on Android

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

Looks good to me. blassey could you review the android/jni part?

::: widget/android/AndroidBridge.cpp
@@ +205,5 @@
>  
> +    if (jniFrame.CheckForException()) {
> +        // We likely failed to lookup a method or something above. We're going to
> +        // have a bad time, so just bail out now.
> +        abort();

I'm not sure if we should call abort() directly here.
Attachment #629212 - Flags: review?(kchen)
Attachment #629212 - Flags: review?(blassey.bugs)
Attachment #629212 - Flags: review+
Comment on attachment 629212 [details] [diff] [review]
Hook up Hal WakeLocks on Android

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

::: widget/android/AndroidBridge.cpp
@@ +205,5 @@
>  
> +    if (jniFrame.CheckForException()) {
> +        // We likely failed to lookup a method or something above. We're going to
> +        // have a bad time, so just bail out now.
> +        abort();

at least log why you're aborting. Even better would be to wrap each one of these GetStaticMethodID() calls in a macro, check for exception after each and log the method that failed.
Attachment #629212 - Flags: review?(blassey.bugs) → review+
(In reply to Brad Lassey [:blassey] from comment #7)
> Comment on attachment 629212 [details] [diff] [review]
> Hook up Hal WakeLocks on Android
> 
> Review of attachment 629212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/AndroidBridge.cpp
> @@ +205,5 @@
> >  
> > +    if (jniFrame.CheckForException()) {
> > +        // We likely failed to lookup a method or something above. We're going to
> > +        // have a bad time, so just bail out now.
> > +        abort();
> 
> at least log why you're aborting. Even better would be to wrap each one of
> these GetStaticMethodID() calls in a macro, check for exception after each
> and log the method that failed.

The log will have the exception in it so I didn't really think anything more was necessary. The difference between this and the current behavior is that we would now crash much closer to the actual cause (instead of whenever the VM next checks for exceptions). Regardless, I can add a log line.

I like the macro idea, though, so I'll remove this and work on another patch that does that instead.
https://hg.mozilla.org/mozilla-central/rev/21ff29da4c41
Assignee: nobody → snorp
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Comment on attachment 629212 [details] [diff] [review]
Hook up Hal WakeLocks on Android

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #629212 - Flags: approval-mozilla-beta?
Attachment #629212 - Flags: approval-mozilla-aurora?
Comment on attachment 629212 [details] [diff] [review]
Hook up Hal WakeLocks on Android

we're going to take this for the .N release, so please land on mozilla-aurora and the mozilla-beta default branch but not the beta 7 release branch on beta
Attachment #629212 - Flags: approval-mozilla-beta?
Attachment #629212 - Flags: approval-mozilla-beta+
Attachment #629212 - Flags: approval-mozilla-aurora?
Attachment #629212 - Flags: approval-mozilla-aurora+
Flash videos in fullscreen keep the device awake with the timeout set to a short period of time (15 sec) for the duration of the video. Marking the issue as verified.

Verified fixed on:
Aurora 15.0a2 2012-07-09/Nightly 16.0a1 2012-07-09/Firefox Mobile 14.0b11
HTC Desire
Android 2.2.2
You need to log in before you can comment on or make changes to this bug.