Closed Bug 1320275 Opened 5 years ago Closed 5 years ago

QuotaManager uses hal:GetCurrentBatteryInformation incorrectly


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

50 Branch
Not set



Tracking Status
firefox53 --- fixed


(Reporter: baku, Assigned: baku)


(Depends on 1 open bug)



(1 file)

Attached patch qm.patchSplinter Review
In order to return the correct value, GetCurrentBatteryInformation() must be called with a BatteryObserver registered.

This code will be make async soon.
Attachment #8814327 - Flags: review?(jvarga)
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 on attachment 8814327 [details] [diff] [review]

Request a review again, if you believe this is really needed.
Attachment #8814327 - Flags: review?(jvarga)
Let's take the linux implementation:
Here we populate the battery info data struct with the 'current' info, but only when:
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:

Of course, if in the meantime, we have use of BatteryAPI, QM works fine.
Attachment #8814327 - Flags: review?(jvarga)
Comment on attachment 8814327 [details] [diff] [review]

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|
Attachment #8814327 - Flags: review?(jvarga) → review+
> 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.
Pushed by
QuotaManager must be registered as battery observer before calling hal:GetCurrentBatteryInformation, r=janv
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1274731
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.