Something like navigator.reboot(), navigator.shutDown(). We want to use this to power off and restart gonk devices. This is obviously a privileged API.

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.

-- Attachment #581185 [details] [diff] - Attachment is obsolete: true

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

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

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.

(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.

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 :)

(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

What happened to the security checks?

-- Attachment #581184 [details] [diff] - Attachment is obsolete: true

-- Attachment #584363 [details] [diff] - Attachment is obsolete: true

-- Attachment #584364 [details] [diff] - Attachment is obsolete: true

-- Attachment #584365 [details] [diff] - Attachment is obsolete: true

-- Attachment #584366 [details] [diff] - Attachment is obsolete: true

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 [details] [diff] - Attachment is obsolete: true

Comment on attachment 584682 [details] [diff] [review] Part 5, hal code for the Power API, v2 Thanks!

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.

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.

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.

(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.

comment #39: there might be no content window around

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

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

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/561e1013aba0 https://hg.mozilla.org/integration/mozilla-inbound/rev/828713f89601 https://hg.mozilla.org/integration/mozilla-inbound/rev/648e5e5b9dd3 https://hg.mozilla.org/integration/mozilla-inbound/rev/656500fc5e79 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a4c4cb828a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/10b455909e94

We had to back these patches out because they caused the windows builds to break. https://hg.mozilla.org/integration/mozilla-inbound/rev/15ca5d162a81 https://hg.mozilla.org/integration/mozilla-inbound/rev/de33a82cee1f https://hg.mozilla.org/integration/mozilla-inbound/rev/f64266a2463a https://hg.mozilla.org/integration/mozilla-inbound/rev/c89909dee64f https://hg.mozilla.org/integration/mozilla-inbound/rev/068243a96679 https://hg.mozilla.org/integration/mozilla-inbound/rev/543a3ea8cbc8

(In reply to Scott Johnson (:jwir3) from comment #59) > We had to back these patches out because they caused the windows builds to > break. > > https://hg.mozilla.org/integration/mozilla-inbound/rev/15ca5d162a81 > https://hg.mozilla.org/integration/mozilla-inbound/rev/de33a82cee1f > https://hg.mozilla.org/integration/mozilla-inbound/rev/f64266a2463a > https://hg.mozilla.org/integration/mozilla-inbound/rev/c89909dee64f > https://hg.mozilla.org/integration/mozilla-inbound/rev/068243a96679 > https://hg.mozilla.org/integration/mozilla-inbound/rev/543a3ea8cbc8 Yeah, I found it from last try push. I will update shortly.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c649c8e47ef https://hg.mozilla.org/integration/mozilla-inbound/rev/be91138c1a84 https://hg.mozilla.org/integration/mozilla-inbound/rev/0d097d1f741e https://hg.mozilla.org/integration/mozilla-inbound/rev/52147d521bbc https://hg.mozilla.org/integration/mozilla-inbound/rev/acc23575da69 https://hg.mozilla.org/integration/mozilla-inbound/rev/629978209c61

https://hg.mozilla.org/mozilla-central/rev/8c649c8e47ef https://hg.mozilla.org/mozilla-central/rev/be91138c1a84 https://hg.mozilla.org/mozilla-central/rev/0d097d1f741e https://hg.mozilla.org/mozilla-central/rev/52147d521bbc https://hg.mozilla.org/mozilla-central/rev/acc23575da69 https://hg.mozilla.org/mozilla-central/rev/629978209c61