Bhavna Sharma Assignee
	Description • 11 years ago

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.

	Bhavna Sharma Assignee
	Comment 1 • 11 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 2 • 11 years ago

Creating a new window would be bad, yes. But you can get the current chrome window with something like:

Services.wm.getMostRecentWindow('navigator:browser');

	Bhavna Sharma Assignee
	Comment 3 • 11 years ago

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 ?

	Bhavna Sharma Assignee
	Comment 4 • 11 years ago

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.

	Nivi
	Comment 5 • 11 years ago

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.

	Nivi
	Comment 6 • 11 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 7 • 11 years ago

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

	Bhavna Sharma Assignee
	Comment 8 • 11 years ago

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

	[:fabrice] Fabrice Desré
	Comment 9 • 11 years ago

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.

	Jason Smith [:jsmith]
	Comment 10 • 11 years ago

Not a blocker - this has no end user impact & isn't a regression.

	Bhavna Sharma Assignee
	Comment 11 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 12 • 11 years ago

I'll wait Fabrice to weigh in here on what we should do here.

	[:fabrice] Fabrice Desré
	Comment 13 • 11 years ago

Yes, I'm fine with exporting that. But why don't you land you code in gecko instead of doing your own xpcom component?

	Bhavna Sharma Assignee
	Comment 14 • 11 years ago

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.

	(no longer active)
	Comment 15 • 11 years ago

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

	Bhavna Sharma Assignee
	Comment 16 • 11 years ago

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 ?

	[:fabrice] Fabrice Desré
	Comment 17 • 11 years ago

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.

	Preeti Raghunath(:Preeti)
	Comment 18 • 11 years ago

Inder,

Can you please help move this ahead? Not sure who needs to own this here.

	[:fabrice] Fabrice Desré
	Comment 19 • 11 years ago

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

	Bhavna Sharma Assignee
	Comment 20 • 11 years ago

I will take care of this, i am waiting on some code scans to be done internally.

	Inder
	Comment 21 • 11 years ago

Bhavna, can you please update the latest status.

	Sri Kasetti
	Comment 22 • 11 years ago

Can we add an assignee to this bug if this is a blocker?

	Bhavna Sharma Assignee
	Comment 23 • 11 years ago

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.

	Jason Smith [:jsmith]
	Comment 24 • 11 years ago

Is this is a WONTFIX then?

	Bhavna Sharma Assignee
	Comment 25 • 11 years ago

Yes it can be a WONTFIX as we well add our own implementation.

	Michael Vines [:m1] [:evilmachines]
	Comment 26 • 11 years ago

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.

	Bhavna Sharma Assignee
	Comment 27 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 28 • 11 years ago

I'm not really the right person to ask about this. Are you looking for product input? Technical input?

	Bhavna Sharma Assignee
	Comment 29 • 11 years ago

Looking for a technical input. Can you add someone who I could talk to about the design ?

	Dave Hylands [:dhylands]
	Comment 30 • 11 years ago

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)

	Michael Vines [:m1] [:evilmachines]
	Comment 31 • 11 years ago

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!

	[:fabrice] Fabrice Desré
	Comment 32 • 11 years ago

(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 ;)

	Bhavna Sharma Assignee
	Comment 33 • 11 years ago

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 ?

	Michael Vines [:m1] [:evilmachines]
	Comment 34 • 11 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 35 • 11 years ago

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.

	Bhavna Sharma Assignee
	Comment 36 • 10 years ago

Fabrice, any update on this ?

	(no longer active)
	Comment 37 • 10 years ago

I think requests for relicensing our source code need to go to Gerv.  Gerv, please see comment 34.

	Gervase Markham [:gerv]
	Comment 38 • 10 years ago

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

	Bhavna Sharma Assignee
	Comment 39 • 10 years ago

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 ?

	[:fabrice] Fabrice Desré
	Comment 40 • 10 years ago

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

	Gervase Markham [:gerv]
	Comment 41 • 10 years ago

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

	Bhavna Sharma Assignee
	Comment 42 • 10 years ago

This patch gives a proposed solution to be able to emit battery level/charging information to components that are not statically linked to Gecko.

	Michael Vines [:m1] [:evilmachines]
	Comment 43 • 10 years ago

Yes? :)

	[:fabrice] Fabrice Desré
	Comment 44 • 10 years ago

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

	Bhavna Sharma Assignee
	Comment 45 • 10 years ago

Will try out the propertybag suggestion.

	Bhavna Sharma Assignee
	Comment 46 • 10 years ago

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.

	Dave Hylands [:dhylands]
	Comment 47 • 10 years ago

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.

	Dave Hylands [:dhylands]
	Comment 48 • 10 years ago

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.

	Michael Vines [:m1] [:evilmachines]
	Comment 49 • 10 years ago

(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

	Bhavna Sharma Assignee
	Comment 50 • 10 years ago

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

	Dave Hylands [:dhylands]
	Comment 51 • 10 years ago

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

	Bhavna Sharma Assignee
	Comment 52 • 10 years ago

Revised patch as per comments#51

	Michael Vines [:m1] [:evilmachines]
	Comment 53 • 10 years ago

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)

	Dave Hylands [:dhylands]
	Comment 54 • 10 years ago

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.

	Michael Vines [:m1] [:evilmachines]
	Comment 55 • 10 years ago

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

	Bhavna Sharma Assignee
	Comment 56 • 10 years ago

Corrected user-name

	Bhavna Sharma Assignee
	Comment 57 • 10 years ago

Add the Bug number to the commit message

	Michael Vines [:m1] [:evilmachines]
	Comment 58 • 10 years ago

Try run looks ok

	Carsten Book [:Tomcat]
	Comment 59 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec43f21b0c1

	Carsten Book [:Tomcat]
	Comment 60 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/6ec43f21b0c1

	Bhavna Sharma Assignee
	Comment 61 • 10 years ago

Great thanks everyone !