Closed Bug 1320275 Opened 3 years ago Closed 3 years ago

QuotaManager uses hal:GetCurrentBatteryInformation incorrectly

Categories

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

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Depends on 1 open bug)

Details

Attachments

(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]
qm.patch

Request a review again, if you believe this is really needed.
Attachment #8814327 - Flags: review?(jvarga)
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.
Attachment #8814327 - Flags: review?(jvarga)
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|
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 amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa64db7e2ff
QuotaManager must be registered as battery observer before calling hal:GetCurrentBatteryInformation, r=janv
https://hg.mozilla.org/mozilla-central/rev/3fa64db7e2ff
Status: NEW → RESOLVED
Closed: 3 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.