1320275 - QuotaManager uses hal:GetCurrentBatteryInformation incorrectly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: qm.patch

In order to return the correct value, GetCurrentBatteryInformation() must be called with a BatteryObserver registered.

This code will be make async soon.

Comment 1

[Jan Varga [:janv]]

Hm, are you sure it doesn't work right now ?
I didn't do any real testing, but if I'm reading code correctly, BatteryObservers().GetCurrentInformation() should work w/o registered observers.

Comment 2

[Jan Varga [:janv]]

Comment on attachment 8814327 [details] [diff] [review]
qm.patch

Request a review again, if you believe this is really needed.

Comment 3

[Andrea Marchesini [:baku]]

Let's take the linux implementation:

https://dxr.mozilla.org/mozilla-central/source/hal/linux/UPowerClient.cpp#151
Here we populate the battery info data struct with the 'current' info, but only when:

https://dxr.mozilla.org/mozilla-central/source/hal/linux/UPowerClient.cpp#195
we have listeners, we activate the reading of the battery level from dbus. Otherwise, we return the default info.

Same for mac where the changes, are received only if we have listeners:
https://dxr.mozilla.org/mozilla-central/source/hal/cocoa/CocoaBattery.cpp#195

Of course, if in the meantime, we have use of BatteryAPI, QM works fine.

Comment 4

[Jan Varga [:janv]]

Comment on attachment 8814327 [details] [diff] [review]
qm.patch

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

Ok, thanks for pointers.

Wow, that part of hal API is quite odd. I'm not surprised that bent didn't add an observer in his original database maintenance implementation.
Hal.h should probably mention the need to register an observer before calling GetCurrentBatteryInformation().

In future, QM could just use the battery manager to get the charging info.

::: dom/quota/QuotaManagerService.cpp
@@ +439,5 @@
>    // Android XPCShell doesn't load the AndroidBridge that is needed to make
>    // GetCurrentBatteryInformation work...
>    if (!QuotaManager::kRunningXPCShellTests)
>  #endif
>    {

This deserves a comment at least.

@@ +709,5 @@
>    return NS_OK;
>  }
>  
> +void
> +QuotaManagerService::Notify(const hal::BatteryInformation& aBatteryInfo)

This should go after QuotaManagerService::Observe()

@@ +711,5 @@
>  
> +void
> +QuotaManagerService::Notify(const hal::BatteryInformation& aBatteryInfo)
> +{
> +  // Nothing here.

The comment could be better.

::: dom/quota/QuotaManagerService.h
@@ +7,5 @@
>  #ifndef mozilla_dom_quota_QuotaManagerService_h
>  #define mozilla_dom_quota_QuotaManagerService_h
>  
>  #include "mozilla/dom/ipc/IdType.h"
> +#include "mozilla/Observer.h"

This can be "mozilla/dom/battery/Types.h"

@@ +26,5 @@
> +namespace hal {
> +
> +class BatteryInformation;
> +
> +} // namespace hal

no need for this then

@@ +37,5 @@
>  
>  class QuotaManagerService final
>    : public nsIQuotaManagerService
>    , public nsIObserver
> +  , public Observer<hal::BatteryInformation>

and here BatteryObserver

@@ +85,5 @@
>    AbortOperationsForProcess(ContentParentId aContentParentId);
>  
> +  // For IObserver.
> +  void
> +  Notify(const hal::BatteryInformation& aBatteryInfo) override;

this should go after NS_DECL_NSIOBSERVER and the comment should be |// BatteryObserver override|

Comment 5

[Andrea Marchesini [:baku]]

> In future, QM could just use the battery manager to get the charging info.

In the future, this code will change. I already have patches to have battery info async requests.
Also the QM part is ready. It will be in your review queue asap.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa64db7e2ff
QuotaManager must be registered as battery observer before calling hal:GetCurrentBatteryInformation, r=janv

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fa64db7e2ff