Add API to shut down and reboot device

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
8 years ago
4 months ago

People

(Reporter: cjones, Assigned: kanru)

Tracking

Trunk
mozilla13
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 25 obsolete attachments)

5.49 KB, patch
Details | Diff | Splinter Review
5.10 KB, patch
Details | Diff | Splinter Review
7.47 KB, patch
Details | Diff | Splinter Review
20.73 KB, patch
Details | Diff | Splinter Review
3.48 KB, patch
Details | Diff | Splinter Review
7.93 KB, patch
Details | Diff | Splinter Review
Something like navigator.reboot(), navigator.shutDown().  We want to use this to power off and restart gonk devices.

This is obviously a privileged API.
Version: unspecified → Trunk
Writing the API should be only some DOM/HAL boilerplate. Do we already have the Gonk code to do that?
Yes, this should be pretty straightforward.  To my knowledge, we don't have that code yet.
Kan-Ru's API in bug 708964 includes shutdown/reboot.
Blocks: 708964
Posted patch Part 1, Makefile boilerplate (obsolete) — Splinter Review
Posted patch Part 6, Connect DOM and hal (obsolete) — Splinter Review
Posted patch Part 6, Connect DOM and hal (obsolete) — Splinter Review
--
Attachment #581185 [details] [diff] - Attachment is obsolete: true
Attachment #581185 - Attachment is obsolete: true
Attachment #581180 - Flags: review?(jones.chris.g)
Attachment #581181 - Flags: review?(jones.chris.g)
Attachment #581182 - Flags: review?(jones.chris.g)
Attachment #581183 - Flags: review?(jones.chris.g)
Attachment #581184 - Flags: review?(jones.chris.g)
Attachment #581197 - Flags: review?(jones.chris.g)
Kan-Ru, I think you should ask a DOM peer [1] to review the changes inside dom/. :sicking or :smaug might be two good choices (Justin or I could help to give feedbacks but we are not peers).
Some people who know DOM APIs should probably have a look at the API. You should ask a super-review for that. Again, :sicking or :smaug might be appropriate but you should not ask the review and the super-review to the same person.

[1] https://wiki.mozilla.org/Modules/All#Document_Object_Model
Assignee: nobody → kchen
Status: NEW → ASSIGNED
Attachment #581180 - Flags: superreview?
Attachment #581180 - Flags: review?(jones.chris.g)
Attachment #581180 - Flags: review?(jonas)
Attachment #581181 - Flags: review?(jones.chris.g) → review?(jonas)
Attachment #581181 - Flags: superreview?(bugs)
Attachment #581182 - Flags: review?(jones.chris.g) → review?(jonas)
Attachment #581183 - Flags: review?(jones.chris.g) → review?(jonas)
Attachment #581197 - Flags: review?(jones.chris.g) → review?(jonas)
Comment on attachment 581181 [details] [diff] [review]
Part 2, PowerManager interface


>+[scriptable, uuid(6ec16abc-2fe8-4ab3-99b0-0f08405be81b)]
>+interface nsIDOMMozPowerManager : nsISupports {
{ should be in the next line
Attachment #581181 - Flags: superreview?(bugs) → superreview+
Comment on attachment 581184 [details] [diff] [review]
Part 5, hal code for the Power API

>diff --git a/widget/src/gonk/nsWindow.h b/widget/src/gonk/nsWindow.h

>+extern nsIntRect gScreenBounds;
>+

This is so ugly.  Please file a followup to get rid of it.

>diff --git a/widget/src/xpwidgets/nsBaseWidget.h b/widget/src/xpwidgets/nsBaseWidget.h

>+  NS_IMETHOD_(void) SetInputContext(const InputContext& aContext,
>+                                    const InputContextAction& aAction) { return; }
>+  NS_IMETHOD_(InputContext) GetInputContext()
>+  {
>+    InputContext ctx;
>+    return ctx;
>+  }

As far as I can tell, this change is unnecessary.  Please remove it.

r=me with that fix.
Attachment #581184 - Flags: review?(jones.chris.g) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> Comment on attachment 581184 [details] [diff] [review]
> Part 5, hal code for the Power API
> 
> >diff --git a/widget/src/gonk/nsWindow.h b/widget/src/gonk/nsWindow.h
> 
> >+extern nsIntRect gScreenBounds;
> >+
> 
> This is so ugly.  Please file a followup to get rid of it.
> 
> >diff --git a/widget/src/xpwidgets/nsBaseWidget.h b/widget/src/xpwidgets/nsBaseWidget.h
> 
> >+  NS_IMETHOD_(void) SetInputContext(const InputContext& aContext,
> >+                                    const InputContextAction& aAction) { return; }
> >+  NS_IMETHOD_(InputContext) GetInputContext()
> >+  {
> >+    InputContext ctx;
> >+    return ctx;
> >+  }
> 
> As far as I can tell, this change is unnecessary.  Please remove it.
> 
> r=me with that fix.

Uh, wrong patch?
Comment on attachment 581183 [details] [diff] [review]
Part 4, Dummy PowerManager DOM code

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

::: dom/base/Navigator.cpp
@@ +144,5 @@
>      mBatteryManager = nsnull;
>    }
>  
> +  if (mPowerManager) {
> +    mPowerManager->Shutdown();

Given that this function is empty, I'd say just nuke it and remove all this code.
Attachment #581183 - Flags: review?(jonas) → review-
Attachment #581197 - Flags: review?(jonas) → review+
Comment on attachment 581197 [details] [diff] [review]
Part 6, Connect DOM and hal

Actually, you need to add security checks here. We don't want just any webpage to be able to shut off your device :)
Attachment #581197 - Flags: review+ → review-
(In reply to Jonas Sicking (:sicking) from comment #16)
> Comment on attachment 581197 [details] [diff] [review]
> Part 6, Connect DOM and hal
> 
> Actually, you need to add security checks here. We don't want just any
> webpage to be able to shut off your device :)

May I ask how to add security checks? I thought the mozPower object is already only accessible by privileged pages.
Looking at part 4, it always returns a non-null object from GetMozPower, so there doesn't seem to be any security checks there either.
You can use nsContentUtils::IsCallerChrome()
Kan-Ru, can you please add a PowerManager service that calls HAL, and the DOM calls the service? This way extensions can override this, and more importantly we can call this from chrome JS code. Thanks!
--
Attachment #581181 [details] [diff] - Attachment is obsolete: true
--
Attachment #581182 [details] [diff] - Attachment is obsolete: true
--
Attachment #581183 [details] [diff] - Attachment is obsolete: true
--
Attachment #581197 [details] [diff] - Attachment is obsolete: true
Attachment #581181 - Attachment is obsolete: true
Attachment #581182 - Attachment is obsolete: true
Attachment #581183 - Attachment is obsolete: true
Attachment #581197 - Attachment is obsolete: true
Attachment #581184 - Flags: review+ → review?(jones.chris.g)
Attachment #584363 - Flags: review?(jonas)
Attachment #584364 - Flags: review?(jonas)
Attachment #584365 - Flags: review?(jonas)
Attachment #584366 - Flags: review?(jonas)
What happened to the security checks?
--
Attachment #581184 [details] [diff] - Attachment is obsolete: true
Attachment #584374 - Flags: review?(jones.chris.g)
--
Attachment #584363 [details] [diff] - Attachment is obsolete: true
Attachment #584375 - Flags: review?(jonas)
--
Attachment #584364 [details] [diff] - Attachment is obsolete: true
Attachment #584376 - Flags: review?(jonas)
--
Attachment #584365 [details] [diff] - Attachment is obsolete: true
Attachment #584377 - Flags: review?(jonas)
--
Attachment #584366 [details] [diff] - Attachment is obsolete: true
Attachment #584378 - Flags: review?(jonas)
Attachment #581184 - Attachment is obsolete: true
Attachment #581184 - Flags: review?(jones.chris.g)
Attachment #584363 - Attachment is obsolete: true
Attachment #584363 - Flags: review?(jonas)
Attachment #584364 - Attachment is obsolete: true
Attachment #584364 - Flags: review?(jonas)
Attachment #584365 - Attachment is obsolete: true
Attachment #584365 - Flags: review?(jonas)
Attachment #584366 - Attachment is obsolete: true
Attachment #584366 - Flags: review?(jonas)
Comment on attachment 584374 [details] [diff] [review]
Part 5, hal code for the Power API, v2

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+#pragma GCC visibility push(default)
>+#include <unistd.h>
>+#include <sys/reboot.h>
>+#pragma GCC visibility pop
>+

You need to add sys/reboot.h to config/system-headers.  Not one of the
prettier parts of our build system, sorry :(.

>+void
>+Reboot()
>+{
>+  reboot(RB_AUTOBOOT);
>+}
>+
>+void
>+PowerOff()
>+{
>+  reboot(RB_POWER_OFF);
>+}
>+

This implementation is the same for gonk and linux.  Let's move it
into a linux/Power.cpp file and then build it for both gonk and
other-non-android-linux.

Looks good otherwise!  I would like to see a v2 patch with the changes
I requested.
Attachment #584374 - Flags: review?(jones.chris.g)
--
Attachment #584374 [details] [diff] - Attachment is obsolete: true
Attachment #584682 - Flags: review?(jones.chris.g)
Attachment #584374 - Attachment is obsolete: true
Comment on attachment 584682 [details] [diff] [review]
Part 5, hal code for the Power API, v2

Thanks!
Attachment #584682 - Flags: review?(jones.chris.g) → review+
Blocks: 697132
Jonas, any ETA on review here?  Pulling the battery out of devices shut them down is getting tiresome :).
Comment on attachment 584376 [details] [diff] [review]
Part 3, Plug mozPower into navigator object, v3

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

Just add the mozPower property to nsIDOMNavigator directly. We've been way too interface-happy in the past which is something we're trying to stop.

r=me with that changed.
Attachment #584376 - Flags: review?(jonas) → review+
Comment on attachment 584377 [details] [diff] [review]
Part 4, Dummy PowerManager DOM code, v3

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

I'd just nuke the service thing here. It seems only useful for C++ callers, and they might as well call into hal directly.

r=me with PowerManagerService removed.

::: dom/power/PowerManager.cpp
@@ +55,5 @@
> +NS_IMPL_RELEASE(PowerManager)
> +
> +PowerManager::PowerManager()
> +{
> +}

Remove the ctor and dtor since they're empty anyway. That way they'll be inlined away.
Attachment #584377 - Flags: review?(jonas) → review+
Comment on attachment 584378 [details] [diff] [review]
Part 6, Connect DOM and hal, v3

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

I'd just nuke the service thing here. It seems only useful for C++ callers, and they might as well call into hal directly.

r=me with PowerManagerService removed.
Attachment #584378 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #37)
> Comment on attachment 584378 [details] [diff] [review]
> Part 6, Connect DOM and hal, v3
> 
> Review of attachment 584378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd just nuke the service thing here. It seems only useful for C++ callers,
> and they might as well call into hal directly.
> 
> r=me with PowerManagerService removed.

PowerManagerService was added to address the issue said in comment 20. It can also be used to hold global wakelock information.
Andreas: Is there a reason chrome code can't call navigator.mozPower.* ? Though that doesn't work from JS-components (since there is no navigator object) which might be an issue.

I'm ok with keeping the service if that's JS-components is something we care about.
Attachment #581180 - Flags: superreview? → superreview+
comment #39: there might be no content window around
Posted patch Part 1, Makefile boilerplate. (obsolete) — Splinter Review
Attachment #581180 - Attachment is obsolete: true
Attachment #584375 - Attachment is obsolete: true
Attachment #584376 - Attachment is obsolete: true
Attachment #584377 - Attachment is obsolete: true
Attachment #584682 - Attachment is obsolete: true
Attachment #584378 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #588340 - Attachment is obsolete: true
Do we want to also add an "Airplane mode" to this API? It's more mobile-specific than just powerOff/reboot but we'll need it for b2g.
I think "airplane mode" should be under control of the content power manager.  "Airplane mode" isn't clearly defined, and might include leaving wifi on (or reserving the ability to enable it later).  Reboot/shutdown are very clearly defined.
applying https://bug709585.bugzilla.mozilla.org/attachment.cgi?id=588338
patching file dom/Makefile.in
Hunk #1 FAILED at 66
1 out of 1 hunks FAILED -- saving rejects to file dom/Makefile.in.rej
patching file dom/dom-config.mk
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file dom/dom-config.mk.rej
abort: patch failed to apply
Keywords: checkin-needed
Attachment #588338 - Attachment is obsolete: true
Attachment #588339 - Attachment is obsolete: true
Attachment #588341 - Attachment is obsolete: true
Attachment #588342 - Attachment is obsolete: true
Attachment #588343 - Attachment is obsolete: true
Keywords: checkin-needed
Try run for 679052bd4045 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=679052bd4045
Results (out of 212 total builds):
    exception: 1
    success: 185
    warnings: 22
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-679052bd4045
Target Milestone: --- → mozilla12
Try run for a673abe29aba is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a673abe29aba
Results (out of 62 total builds):
    exception: 1
    success: 56
    warnings: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-a673abe29aba
Added windows stub
Attachment #590112 - Attachment is obsolete: true
Keywords: checkin-needed
Target Milestone: --- → mozilla13
Depends on: 819795
Depends on: 963366
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.