Closed Bug 975667 Opened 7 years ago Closed 6 years ago

Access battery status using xpcominterface in native code

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
feature-b2g 2.0

People

(Reporter: sbhavna, Assigned: sbhavna)

References

Details

(Whiteboard: [cr 620651])

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

battery = window.navigator.battery


Actual results:

Got error saying window is undefined.
Thats right because this is a java script module which does not have a window object.
But does that mean we cannot access a battery object without a window object ?


Expected results:

Need a way to access the battery object without a window object.
Closing this as it seems we do need to have a window object to get access to a battery object.
We could create a window object using nsIWindowWatcher interface, but that does not seem to be a good idea.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Creating a new window would be bad, yes. But you can get the current chrome window with something like:

Services.wm.getMostRecentWindow('navigator:browser');
Well, first i just need this to be able to get chargingchange event. 
Do not have any preference of doing this in jsm as compared to native code.

But when i try to use BatteryObserver i.e. for example
#include "mozilla/Hal.h"
class A : BatteryObserver

I get this error: obj/objdir-gecko/dist/include/mozilla/Hal.h:10:38: fatal error: mozilla/hal_sandbox/PHal.h: No such file or directory
compilation terminated.

Any clue ?
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---
Changing the title of the bug to better explain the issue.

How can I access the BatteryObserver class outside of Gecko ?

When i try to use BatteryObserver i.e. for example
#include "mozilla/Hal.h"
class A : BatteryObserver

I get this error: obj/objdir-gecko/dist/include/mozilla/Hal.h:10:38: fatal error: mozilla/hal_sandbox/PHal.h: No such file or directory
compilation terminated.
Summary: Cannot access battery object without a window.navigator object → Access BatteryObserver outside of gecko
blocking-b2g: --- → 1.4?
Hi Fabrice,

Can we access battery status from c++ code using an XPCOM interface?
I tried looking everywhere but couldn't find any construct to access Battery information from outside gecko?

Do you have any suggestion/pointers?

Nivi.
blocking-b2g: 1.4? → ---
Flags: needinfo?(fabrice)
Summary: Access BatteryObserver outside of gecko → Cannot access battery object without a window.navigator object
Hi Fabrice,

Can we access battery status from c++ code using an XPCOM interface?
I tried looking everywhere but couldn't find any construct to access Battery information from outside gecko?

Do you have any suggestion/pointers?

Nivi.
blocking-b2g: --- → 1.4?
Summary: Cannot access battery object without a window.navigator object → Access battery status using xpcominterface in native code
No, the battery api is not implemented as an xpcom component.

As for you include error, it's hard to say what's wrong with so little context. Have you looked at other uses of BatteryObserver in gecko? https://mxr.mozilla.org/mozilla-central/search?string=BatteryObserver
Flags: needinfo?(fabrice)
Hi Fabrice,

I referred to the use of BatteryObsever in https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.h, and it seems we only need to include mozilla/Hal.h.

But if i do that i run into this error:
I get this error: obj/objdir-gecko/dist/include/mozilla/Hal.h:10:38: fatal error: mozilla/hal_sandbox/PHal.h: No such file or directory
compilation terminated.

The location of PHal.h is in obj\objdir-gecko\ipc\ipdl\_ipdlheaders\mozilla\hal_sandbox, which cannot be found.

The module where i am trying to include the mozilla/Hal.h is outside of gecko though.

- Bhavna
Look at LOCAL_INCLUDES in https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/moz.build 
You should have something similar in your build system to get right includes.
Not a blocker - this has no end user impact & isn't a regression.
blocking-b2g: 1.4? → backlog
Hi Jason / Fabrice,

We require the battery charge status for power optimization reasons.

As I understand since there is no XPCOM interface available, we need to be directly using hal::RegisterBatteryObserver and hal::UnregisterBatteryObserver in our code. We are seeing both compilation and linker issues with using these API's

The compilation dependency is because when we try to include PHal.h there is a chain of includes required all of which are not exported, for example base/process_utils.h. 

Also these API's are not exported in libxul.so which causes linker errors.

This is what we suggest in hal/Hal.h to resolve the linker dependency:

-void RegisterBatteryObserver(BatteryObserver* aBatteryObserver);
+MOZ_EXPORT void RegisterBatteryObserver(BatteryObserver* aBatteryObserver);

-void UnregisterBatteryObserver(BatteryObserver* aBatteryObserver);
+MOZ_EXPORT void UnregisterBatteryObserver(BatteryObserver* aBatteryObserver);

- Bhavna
blocking-b2g: backlog → 1.4?
Flags: needinfo?(jsmith)
Flags: needinfo?(fabrice)
I'll wait Fabrice to weigh in here on what we should do here.
Flags: needinfo?(jsmith)
Yes, I'm fine with exporting that. But why don't you land you code in gecko instead of doing your own xpcom component?
Flags: needinfo?(fabrice)
Hi Fabrice,

Our code is in proprietary space, cannot be part of Gecko.

I would really prefer getting us an XPCOM Interface, unless your strongly against it.
Flags: needinfo?(fabrice)
(In reply to Bhavna Sharma from comment #11)
> Hi Jason / Fabrice,
> 
> We require the battery charge status for power optimization reasons.
> 
> As I understand since there is no XPCOM interface available, we need to be
> directly using hal::RegisterBatteryObserver and
> hal::UnregisterBatteryObserver in our code. We are seeing both compilation
> and linker issues with using these API's
> 
> The compilation dependency is because when we try to include PHal.h there is
> a chain of includes required all of which are not exported, for example
> base/process_utils.h. 

In order to get the base/process_utils.h header, you need this line in your moz.build file in addition to the LOCAL_INCLUDES Fabrice mentioned: https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/moz.build#111

> Also these API's are not exported in libxul.so which causes linker errors.
> 
> This is what we suggest in hal/Hal.h to resolve the linker dependency:
> 
> -void RegisterBatteryObserver(BatteryObserver* aBatteryObserver);
> +MOZ_EXPORT void RegisterBatteryObserver(BatteryObserver* aBatteryObserver);
> 
> -void UnregisterBatteryObserver(BatteryObserver* aBatteryObserver);
> +MOZ_EXPORT void UnregisterBatteryObserver(BatteryObserver*
> aBatteryObserver);

I'll leave this part to Fabrice.  :-)
Thanks Ehsan, I think the declaration is not a very big issue as we can always declare it in our own code to all the compiler to go ahead.

Fabrice, will you be uploading the MOZ_EXPORT changes ?
I won't upload any patch here myself. Happy to review though.

Since we have no hope to get your code into the upstream tree, I tend to prefer going with the MOZ_EXPORT and not add the overhead of an xpcom to maintain.
Flags: needinfo?(fabrice)
Inder,

Can you please help move this ahead? Not sure who needs to own this here.
Flags: needinfo?(ikumar)
(In reply to Preeti Raghunath(:Preeti) from comment #18)
> Inder,
> 
> Can you please help move this ahead? Not sure who needs to own this here.

QC is the owner here.
I will take care of this, i am waiting on some code scans to be done internally.
Flags: needinfo?(ikumar) → needinfo?(sbhavna)
blocking-b2g: 1.4? → 1.4+
Component: General → DOM: Device Interfaces
Product: Firefox OS → Core
Version: unspecified → Trunk
Bhavna, can you please update the latest status.
Can we add an assignee to this bug if this is a blocker?
Flags: needinfo?(mvines)
This feature had 2 parts, one was exporting the register/unregister battery observer functions from HAL, and the other was too make the BatteryInformation class available in proprietary space.

Making this BatteryInformation class which comes under MPL 2.0, available in proprietary code is not an option.

Removing the FC blocker.
blocking-b2g: 1.4+ → ---
Is this is a WONTFIX then?
Yes it can be a WONTFIX as we well add our own implementation.
Flags: needinfo?(sbhavna)
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago7 years ago
Flags: needinfo?(mvines)
Resolution: --- → WONTFIX
Let's keep this bug open because the workaround is unsatisfactory -  two battery observers in Gecko will now actively polling for battery state.  The way to fix this bug is to enable a clean way for XPCOM components that are not statically linked to libxul.so access to the in-Gecko battery observer.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Reopening the discussion for v2.0

Battery charging status is very useful for our location algorithm. We need some help with writing the xpcom component.

The battery manager under gecko/dmo/battery serves the need for DOM applications via the navigator object. Seems to be used by gaia for battery charging status. Our proprietary module GonkGPSGeolocationProvider cannot make use of this approach and neither can it directly make use of hal battery observer because of compilation and linker dependencies with BatteryInformation type.

I was thinking of writing an XPCOM component that does pretty much what the battery manager under gecko/dom/battery is doing. The component will have its own .idl and manifest. Have some design questions:

1. The component will be written in native code ?
2. Where should the code of the component live , /gecko/dom/battery or someplace else ?
3. The idl of the component would look something like this
interface nsIBatteryObserver : nsISupports
{
  readonly attribute double level;
  readonly attribute bool charging;
  readonly attribute double remainingTime;
}
4. On receiving the Notify call from HAL, it will use observer service to call into clients registered for a topic "battery-status-change".
5. Will the component be instantiated at boot-up time ?

- Bhavna
blocking-b2g: --- → 2.0?
Flags: needinfo?(jsmith)
I'm not really the right person to ask about this. Are you looking for product input? Technical input?
Flags: needinfo?(jsmith)
Looking for a technical input. Can you add someone who I could talk to about the design ?
Flags: needinfo?(jsmith)
Flags: needinfo?(jsmith) → needinfo?(fabrice)
Since you're in proprietary code, you could just listen to the OS events the same way that the BatteryObserver does:

http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkHal.cpp#297

(i.e. it uses uevents and a handful of /sys files to obtain its information)
That's definitely possible but we're trying to also avoid having multiple timers/code blocks that do the same thing, especially when we're all in the same process.  How else will we get down to 64MB!
(In reply to Bhavna Sharma from comment #27)

> I was thinking of writing an XPCOM component that does pretty much what the
> battery manager under gecko/dom/battery is doing. The component will have
> its own .idl and manifest. Have some design questions:
> 
> 1. The component will be written in native code ?

Yes.

> 2. Where should the code of the component live , /gecko/dom/battery or
> someplace else ?

Yes, dom/battery/BatteryService.XX

> 3. The idl of the component would look something like this
> interface nsIBatteryObserver : nsISupports
> {
>   readonly attribute double level;
>   readonly attribute bool charging;
>   readonly attribute double remainingTime;
> }

Well, you'll need a batterymanager that will provide a way to register and unregister observers. Here you only have the parameters passed to the observers.

> 4. On receiving the Notify call from HAL, it will use observer service to
> call into clients registered for a topic "battery-status-change".

Oh, if you do that you don't need to write any custom xpcom at all. But it will very likely only work in the parent process (disclaimer: I didn't check the hal battery code)

> 5. Will the component be instantiated at boot-up time ?

No. If you do a service, it will be instantiated at first use.


(In reply to Michael Vines [:m1] [:evilmachines] from comment #31)
> That's definitely possible but we're trying to also avoid having multiple
> timers/code blocks that do the same thing, especially when we're all in the
> same process.  How else will we get down to 64MB!

I'm really happy to learn that you're not only going the easy way by adding more cores and memory ;)
Flags: needinfo?(fabrice)
Oh, if you do that you don't need to write any custom xpcom at all. But it will very likely only work in the parent process (disclaimer: I didn't check the hal battery code)
>> The hal BatteryObserver notifies the /gecko/dom/batterymanager via Observer::Notify(Observer<T>) call i.e. http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/Observer.h#67. We do not have access to the BatteryInformation class in this case.


Well, you'll need a batterymanager that will provide a way to register and unregister observers. Here you only have the parameters passed to the observers.
>> If we use the observer service the component will not have to maintain a list of observers.

What is your thought ?
Flags: needinfo?(fabrice)
Instead of a new xpcom component could we do something with the existing 
RegisterBatteryObserver()/UnregisterBatteryObserver() Hal functions instead?

We are unable to use solution discussed in comment 11 because the observer needs to statically link with the PIDL-generated hal::BatteryInformation class.  hal::BatteryInformation is defined in a MPL2-licensed file, PHal.ipdl.  Is there any possibility of simply moving the hal::BatteryInformation struct into an independent file that is Apache2 licensed?  That combined with annotating these functions with MOZ_EXPORT would do the trick.  The battery observer implementation is Apache2 already.
I agreed with m1 on irc to check on my side the license issue, and if we're in the clear to go with its proposal in comment 34.
Flags: needinfo?(fabrice)
Fabrice, any update on this ?
Flags: needinfo?(fabrice)
I think requests for relicensing our source code need to go to Gerv.  Gerv, please see comment 34.
Flags: needinfo?(gerv)
Flags: needinfo?(fabrice)
Someone needs to do the archaeology to work out who has contributed to that file. If all the contributors were Mozilla employees or contractors contributing on Mozilla time, then it's fine. If not, we have to get permission.

Let me know what you find :-)

Gerv
Flags: needinfo?(gerv)
Who can help find the information in the Comment 38 above ?

Also from m1's comment 34, we were talking about moving the BatteryInformation struct to a different .ipdl which could be Apache2 license, but not touch the original license in PHAL.ipdl ? Is it still necessary to find who contributed to phal.pidl in that case ?
Flags: needinfo?(mvines)
Flags: needinfo?(fabrice)
(In reply to Bhavna Sharma from comment #39)
> Who can help find the information in the Comment 38 above ?
> 
> Also from m1's comment 34, we were talking about moving the
> BatteryInformation struct to a different .ipdl which could be Apache2
> license, but not touch the original license in PHAL.ipdl ? Is it still
> necessary to find who contributed to phal.pidl in that case ?

I would rather ask contributors, yes (look at the file blame and history to get the list). You're not really creating a new file here so please don't hack around.

Gerv, what do you think?
Flags: needinfo?(fabrice)
Copying it somewhere with a new license would still require the same permissions to be sought.

Yes, it requires someone who wants this to happen to look at the file's blame and history and work out who all the contributors are, work out their employment status at the time of contribution, and see if there's anyone we need to ask. Then tell me.

Gerv
This patch gives a proposed solution to be able to emit battery level/charging information to components that are not statically linked to Gecko.
Attachment #8425627 - Flags: review?(fabrice)
Flags: needinfo?(mvines)
Flags: needinfo?(mvines)
Flags: needinfo?(fabrice)
Yes? :)
Flags: needinfo?(mvines)
Whiteboard: cr 620651
Whiteboard: cr 620651 → [cr 620651]
Flags: needinfo?(fabrice)
Comment on attachment 8425627 [details] [diff] [review]
0001-Bug-975667-Emit-battery-level-charging-notification.patch

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

You also need to get r+ from a hal peer, which I'm not. Ask dhylands for instance.

::: hal/gonk/GonkHal.cpp
@@ +458,5 @@
> +        aBatteryInfo->level(),
> +        aBatteryInfo->charging() ? "true" : "false");
> +
> +      NS_ConvertUTF8toUTF16 data16(data);
> +      os->NotifyObservers(nullptr, "gonkhal-battery-notifier", data16.get());

that's kind of ugly, and will make the life of c++ users miserable. You should rather stick the values in the subject of the notification. Either create a new xpcom interface for that, or use an object implementing nsIWritablePropertyBag2 (see https://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#3493 for instance).
Attachment #8425627 - Flags: review?(fabrice) → review-
Will try out the propertybag suggestion.
Comment on attachment 8425627 [details] [diff] [review]
0001-Bug-975667-Emit-battery-level-charging-notification.patch

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

Dave, could you also give an initial review ?
I will check-out the propertybag suggestion by Fabrice.
Attachment #8425627 - Flags: review?(dhylands)
Comment on attachment 8425627 [details] [diff] [review]
0001-Bug-975667-Emit-battery-level-charging-notification.patch

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

::: hal/gonk/GonkHal.cpp
@@ +460,5 @@
> +
> +      NS_ConvertUTF8toUTF16 data16(data);
> +      os->NotifyObservers(nullptr, "gonkhal-battery-notifier", data16.get());
> +    }
> +  }

This feels like the wrong place to put this. Calling GetBatteryInformation should not be triggering notifications. (especially since the observer of the notification may call GetBatteryInformation, and now you have an infinite loop).

There is already a BatteryObserver whose Notify method is called each time a uevent is received from the kernel.

NotifyOnservers needs to be called on the main thread, but you can use a runnable to make that happen.
Attachment #8425627 - Flags: review?(dhylands) → review-
I reread through the comments in the bug, and I think that the reason you're putting this in GetBatteryInformation is because you can't call EnableBatterNotifications?

If that's the case, then this little block of code needs a couple of things.

1 - A comment that explains why the call to NotifyOberservers even exists inside GetBatteryInformation

2 - Some protection to prevent NotifyObservers from being called again while you're still processing the first call to NotifyObservers.
(In reply to Dave Hylands [:dhylands] from comment #48)
> I reread through the comments in the bug, and I think that the reason you're
> putting this in GetBatteryInformation is because you can't call
> EnableBatterNotifications?

Thank Dave.  Yeah, we can't call RegisterBatteryObserver() from non-statically linked code and/or staticaly link with parts of PHal.h
Should we instead move the notifyobservers call to here:
https://hg.mozilla.org/mozilla-central/file/c695c2bd13ae/hal/gonk/GonkHal.cpp#l292 ,

instead of calling it from GetCurrentBatteryInformation
(In reply to Bhavna Sharma from comment #50)
> Should we instead move the notifyobservers call to here:
> https://hg.mozilla.org/mozilla-central/file/c695c2bd13ae/hal/gonk/GonkHal.
> cpp#l292 ,
> 
> instead of calling it from GetCurrentBatteryInformation

I think that would be a good location.

I still think it deserves a comment, since it's "out-of-place" (i.e. the natural thing to do would be to use the regular battery notification stuff). You should probably include a reference to this bug as well (in the comment). This will make it easier for people to find out the background behind why this "non-obvious" thing was done.
Assignee: nobody → sbhavna
Revised patch as per comments#51
Attachment #8425627 - Attachment is obsolete: true
Attachment #8430384 - Flags: review?(mvines)
Attachment #8430384 - Flags: review?(dhylands)
Comment on attachment 8430384 [details] [diff] [review]
0001-Bug-975667-hal-gonk-Emit-battery-level-charging-notification.patch

(clearing r? as I'm not a peer for this component)
Attachment #8430384 - Flags: review?(mvines)
Comment on attachment 8430384 [details] [diff] [review]
0001-Bug-975667-hal-gonk-Emit-battery-level-charging-notification.patch

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

Looks good to me. Thanks for the patch.
Attachment #8430384 - Flags: review?(dhylands) → review+
Comment on attachment 8430384 [details] [diff] [review]
0001-Bug-975667-hal-gonk-Emit-battery-level-charging-notification.patch

Oops, the commiter name is wrong.  Bhavna, please upload a new patch with this fixed and carry forward the r+.

I pushed this patch to try:   https://tbpl.mozilla.org/?tree=Try&rev=e8acc2372235
Flags: needinfo?(sbhavna)
Corrected user-name
Attachment #8430384 - Attachment is obsolete: true
Attachment #8431169 - Flags: review+
Add the Bug number to the commit message
Attachment #8431169 - Attachment is obsolete: true
Attachment #8431172 - Flags: review+
blocking-b2g: 2.0? → ---
feature-b2g: --- → 2.0
Try run looks ok
Flags: needinfo?(sbhavna)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ec43f21b0c1
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Great thanks everyone !
You need to log in before you can comment on or make changes to this bug.