Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(57)

Issue 2643713002: Port BatteryMonitor into Device Service (Closed)

Created:
2 years, 10 months ago by blundell
Modified:
2 years, 8 months ago
CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, blink-reviews, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, haraken, jam, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Port BatteryMonitor into Device Service This CL ports the BatteryMonitor Mojo interface from being hosted in //content/browser to being hosted in the Device Service. Of particular interest: - Adds the ability for the Device Service to register interfaces in Java, taking the lead of //content/browser but being more simple as //content/browser is designed to allow an embedder to register such interfaces as well. - Moves BatteryStatusService shutdown from BrowserMainLoop to the Device Service destructor. This has the concrete impact of moving this code to being called on Mojo shutdown, which is slightly earlier in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was previously being called. - Eliminates the BatteryMonitorIntegration browsertests. These tests inject a dummy BatteryMonitor implementation and test that this dummy implementation is correctly exposed to the renderer. Injecting a dummy implementation into the Device Service from //content/browser is non-trivial, and the functionality being tested is already covered by the BatteryMonitorImpl browsertests, which remain. BUG=678920, 680936 TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that the information being displayed is correct. Review-Url: https://codereview.chromium.org/2643713002 Cr-Commit-Position: refs/heads/master@{#459089} Committed: https://chromium.googlesource.com/chromium/src/+/663fd6d75da6a49f89485181bc7ce91463b39f91

Patch Set 1 #

Patch Set 2 : nits #

Patch Set 3 : Rebase #

Patch Set 4 : Rebase #

Patch Set 5 : Rebase #

Patch Set 6 : Fix GN check #

Patch Set 7 : Self-review #

Total comments: 16

Patch Set 8 : Rebase #

Patch Set 9 : Response to review #

Patch Set 10 : Rebase #

Total comments: 6

Patch Set 11 : Response to review #

Patch Set 12 : Rebase #

Patch Set 13 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+151 lines, -236 lines) Patch
M content/app/DEPS View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -3 lines 0 comments Download
M content/browser/DEPS View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download
A content/browser/battery_status/DEPS View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
D content/browser/battery_status/battery_monitor_integration_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -190 lines 0 comments Download
M content/browser/browser_main_loop.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -7 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +0 lines, -6 lines 0 comments Download
M content/public/android/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -2 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -4 lines 0 comments Download
M content/public/app/mojo/content_browser_manifest.json View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download
M content/public/app/mojo/content_renderer_manifest.json View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download
M content/renderer/DEPS View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download
M device/battery/battery_monitor.mojom View 1 chunk +2 lines, -0 lines 0 comments Download
M services/device/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +26 lines, -0 lines 0 comments Download
M services/device/DEPS View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +27 lines, -0 lines 0 comments Download
M services/device/device_service.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +23 lines, -0 lines 0 comments Download
M services/device/device_service.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +50 lines, -3 lines 0 comments Download
M services/device/manifest.json View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/battery-status/resources/mock-battery-monitor.js View 1 2 3 4 2 chunks +11 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/modules/battery/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/battery/DEPS View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 65 (46 generated)
blundell
rockot@: whole CL timvolodine@: for //content/browser/battery_status OWNERS, the question that I left for you re: ...
2 years, 10 months ago (2017-01-24 11:35:43 UTC) #26
Ken Rockot(use gerrit already)
This is really great to see. LGTM all around.
2 years, 10 months ago (2017-01-24 21:15:34 UTC) #27
Sam McNally
https://codereview.chromium.org/2643713002/diff/120001/services/device/BUILD.gn File services/device/BUILD.gn (right): https://codereview.chromium.org/2643713002/diff/120001/services/device/BUILD.gn#newcode40 services/device/BUILD.gn:40: deps -= [ "//device/battery" ] if (is_android) { deps ...
2 years, 10 months ago (2017-01-25 05:53:06 UTC) #28
blundell
Thanks! sammc@, PTAL. https://codereview.chromium.org/2643713002/diff/120001/services/device/BUILD.gn File services/device/BUILD.gn (right): https://codereview.chromium.org/2643713002/diff/120001/services/device/BUILD.gn#newcode40 services/device/BUILD.gn:40: deps -= [ "//device/battery" ] On ...
2 years, 10 months ago (2017-01-25 15:00:35 UTC) #37
timvolodine
maybe I am missing something, is the description wrt battery monitor mojo correct? it says ...
2 years, 10 months ago (2017-01-25 18:54:40 UTC) #38
blundell
Thanks for the review, Tim! With respect to the CL description: The BatteryMonitor *code* lives ...
2 years, 10 months ago (2017-01-26 08:42:09 UTC) #39
blundell
https://codereview.chromium.org/2643713002/diff/180001/services/device/manifest.json File services/device/manifest.json (right): https://codereview.chromium.org/2643713002/diff/180001/services/device/manifest.json#newcode7 services/device/manifest.json:7: "device:battery_monitor": ["device::BatteryMonitor" ], On 2017/01/25 18:54:40, timvolodine wrote: > ...
2 years, 10 months ago (2017-01-26 08:44:59 UTC) #40
Sam McNally
lgtm
2 years, 10 months ago (2017-01-26 23:43:07 UTC) #41
blundell
Tim: friendly ping.
2 years, 10 months ago (2017-01-27 12:52:19 UTC) #42
timvolodine
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browser_main_loop.cc File content/browser/browser_main_loop.cc (left): https://codereview.chromium.org/2643713002/diff/120001/content/browser/browser_main_loop.cc#oldcode1372 content/browser/browser_main_loop.cc:1372: device::BatteryStatusService::GetInstance()->Shutdown(); On 2017/01/24 11:35:43, blundell wrote: > As I ...
2 years, 10 months ago (2017-01-27 18:16:27 UTC) #43
blundell
On 2017/01/27 18:16:27, timvolodine wrote: > https://codereview.chromium.org/2643713002/diff/120001/content/browser/browser_main_loop.cc > File content/browser/browser_main_loop.cc (left): > > https://codereview.chromium.org/2643713002/diff/120001/content/browser/browser_main_loop.cc#oldcode1372 > ...
2 years, 10 months ago (2017-01-30 14:28:24 UTC) #44
timvolodine
On 2017/01/30 14:28:24, blundell wrote: > On 2017/01/27 18:16:27, timvolodine wrote: > > > https://codereview.chromium.org/2643713002/diff/120001/content/browser/browser_main_loop.cc ...
2 years, 10 months ago (2017-01-31 07:21:48 UTC) #45
blundell
On 2017/01/31 07:21:48, timvolodine wrote: > On 2017/01/30 14:28:24, blundell wrote: > > On 2017/01/27 ...
2 years, 10 months ago (2017-01-31 08:32:17 UTC) #46
timvolodine
On 2017/01/31 08:32:17, blundell wrote: > On 2017/01/31 07:21:48, timvolodine wrote: > > On 2017/01/30 ...
2 years, 10 months ago (2017-02-02 02:33:51 UTC) #47
blundell
jam@: Can you review //content? tsepez@: Can you review the changes to manifest files? Thanks!
2 years, 8 months ago (2017-03-22 17:23:02 UTC) #49
Tom Sepez
lgtm
2 years, 8 months ago (2017-03-22 19:48:38 UTC) #50
jam
lgtm
2 years, 8 months ago (2017-03-23 00:28:47 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2643713002/240001
2 years, 8 months ago (2017-03-23 15:15:17 UTC) #62
commit-bot: I haz the power
2 years, 8 months ago (2017-03-23 15:23:17 UTC) #65
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/663fd6d75da6a49f89485181bc7c...

Powered by Google App Engine
This is Rietveld 408576698