Navigator.getBattery should return constant result #567

jumde · 2018-10-04T03:31:48Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

Open Brave and navigate to the Developer Console
Enter: navigator.getBattery()

>navigator.getBattery()
Promise {
<resolved>: BatteryManager}
__proto__: Promise[[PromiseStatus]]: "resolved"
[[PromiseValue]]: BatteryManager
  charging: true
  chargingTime: 0
  dischargingTime: Infinity
  level: 1
  onchargingchange: null
  onchargingtimechange: null
  ondischargingtimechange: null
  onlevelchange: null
  __proto__: BatteryManager

Disconnect the charger from your device and let the battery drain a bit. Invoke getBattery again and verify the values remain the same.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

bridiver · 2018-10-04T05:24:00Z

chromium_src/third_party/blink/renderer/modules/battery/navigator_battery.cc

 #include "net/proxy_resolution/proxy_resolver_v8.h"
+#include "third_party/blink/renderer/core/execution_context/execution_context.h"


why are we still overriding this at all?

bridiver · 2018-10-04T05:24:50Z

chromium_src/third_party/blink/renderer/modules/battery/battery_manager.cc

+  }
+
+  bool BatteryManager::charging() {
+    return true;


shouldn't this be false?

We are not using this function at all, I picked true as the default value for charging. Shouldn't be hard to change this to false.

https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/battery/battery_status.h?q=BatteryStatus&sq=package:chromium&g=0&l=19

can you check the values on a fully charged and plugged in laptop?

Chrome:

Fully charged:

charging: true chargingTime: 0 dischargingTime: Infinity level: 1 onchargingchange: null onchargingtimechange: null ondischargingtimechange: null onlevelchange: null

89%

charging: false chargingTime: Infinity dischargingTime: 5820 level: 0.89 onchargingchange: null onchargingtimechange: null ondischargingtimechange: null onlevelchange: null

Brave (with fix):

Fully charged:

charging: true chargingTime: 0 dischargingTime: Infinity level: 1 onchargingchange: null onchargingtimechange: null ondischargingtimechange: null onlevelchange: null

89%:

[[PromiseValue]]: BatteryManager charging: true chargingTime: 0 dischargingTime: Infinity level: 1 onchargingchange: null onchargingtimechange: null ondischargingtimechange: null onlevelchange: null

bridiver · 2018-10-04T05:25:25Z

chromium_src/third_party/blink/renderer/modules/battery/battery_manager.cc

+
+  BatteryManager* BatteryManager::Create(ExecutionContext* context) {
+    BatteryManager* battery_manager = new BatteryManager(context);
+    battery_manager->PauseIfNeeded();


I don't think we need to call this here because Pause/Unpause are noops anyway

Without this a DCHECK is invoked when the BatteryManager destructor is called [30122:775:1005/101814.262486:FATAL:pausable_object.cc(49)] Check failed: pause_if_needed_called_.

bridiver · 2018-10-05T15:54:10Z

chromium_src/third_party/blink/renderer/modules/battery/battery_manager.cc

+
+  ScriptPromise BatteryManager::StartRequest(ScriptState* script_state) {
+    if (!battery_property_) {
+      BatteryStatus* status = new BatteryStatus();


battery status is never used and this will leak memory for every page

bridiver · 2018-10-05T18:08:26Z

chromium_src/third_party/blink/renderer/modules/battery/battery_manager.cc

+  }
+
+  void BatteryManager::UnregisterWithDispatcher() {
+    return;


upstream sets battery_property_ to null here and I think that might be necessary for GC of the Member property

https://github.com/brave/brave-core/pull/567/files#diff-70de47d152a190708103966693ce09e9R80

sorry, wrong method. Happens in ContextDestroyed below which is now correct

@bridiver

Auditors: @bridiver

Navigator.getBattery should return constant result

bsclifton · 2018-10-13T06:19:01Z

master 441ea83
0.56.x 6158801
0.55.x 9a458ee

jumde requested a review from bridiver October 4, 2018 03:31

jumde force-pushed the battery_status branch 2 times, most recently from 40af933 to dc036ef Compare October 4, 2018 04:48

jumde changed the title ~~WIP: Navigator.getBattery should return constant result~~ Navigator.getBattery should return constant result Oct 4, 2018

bridiver reviewed Oct 4, 2018

View reviewed changes

jumde force-pushed the battery_status branch from dc036ef to ea97c9e Compare October 4, 2018 05:38

bridiver reviewed Oct 5, 2018

View reviewed changes

jumde force-pushed the battery_status branch from ea97c9e to e99d007 Compare October 5, 2018 17:27

bridiver reviewed Oct 5, 2018

View reviewed changes

Issue 1398: Navigator.getBattery should return constant result

a71abb6

Auditors: @bridiver

jumde force-pushed the battery_status branch from e99d007 to a71abb6 Compare October 5, 2018 18:53

bridiver approved these changes Oct 5, 2018

View reviewed changes

bridiver mentioned this pull request Oct 13, 2018

Battery api should return constant result brave/brave-browser#1398

Closed

bsclifton assigned jumde Oct 13, 2018

bsclifton merged commit 441ea83 into master Oct 13, 2018

bsclifton deleted the battery_status branch October 13, 2018 06:17

bsclifton added a commit that referenced this pull request Oct 13, 2018

Merge pull request #567 from brave/battery_status

6158801

Navigator.getBattery should return constant result

bsclifton added a commit that referenced this pull request Oct 13, 2018

Merge pull request #567 from brave/battery_status

9a458ee

Navigator.getBattery should return constant result

bbondy added the merged/0.55.x label Oct 15, 2018

bbondy added this to the 0.55.x - Release milestone Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigator.getBattery should return constant result #567

Navigator.getBattery should return constant result #567

jumde commented Oct 4, 2018 •

edited

Loading

bridiver Oct 4, 2018

bridiver Oct 4, 2018

jumde Oct 4, 2018

jumde Oct 4, 2018

bridiver Oct 4, 2018

jumde Oct 4, 2018

bridiver Oct 4, 2018

jumde Oct 5, 2018 •

edited

Loading

bridiver Oct 5, 2018

bridiver Oct 5, 2018

bridiver Oct 5, 2018 •

edited

Loading

jumde Oct 5, 2018

bridiver Oct 5, 2018

bsclifton commented Oct 13, 2018

		#include "net/proxy_resolution/proxy_resolver_v8.h"
		#include "third_party/blink/renderer/core/execution_context/execution_context.h"

Navigator.getBattery should return constant result #567

Navigator.getBattery should return constant result #567

Conversation

jumde commented Oct 4, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumde Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Oct 13, 2018

jumde commented Oct 4, 2018 •

edited

Loading

jumde Oct 5, 2018 •

edited

Loading

bridiver Oct 5, 2018 •

edited

Loading