Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(8)

Issue 2805433002: Adding ArcBridge interface for synchronizing brightness settings (Closed)

Created:
3 years, 8 months ago by yueli
Modified:
3 years, 8 months ago
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hashimoto+watch_chromium.org, derat+watch_chromium.org, viettrungluu+watch_chromium.org, qsr+mojo_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, oshima+watch_chromium.org, darin (slow to review), ejcaruso+watch_chromium.org, yusukes+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Adding ArcBridge interface for synchronizing brightness settings In order to enable brightness adjustment from Android side, this cl provide ArcBridge interfaces to ArcPowerManagerService on Android side: - After PowerHost was initialized, UpdateScreenBrightnessSettings(.) will be called to notify Android side about current screen brightness. - When changes occur for brightness setting on Android side, OnScreenBrightnessUpdateRequest(.) will be called to adjust the OS brightness. - When OS brightness was changed, UpdateScreenBrightnessSettings(.) will be called to notify Android side about the change. BUG=b/36129958 Review-Url: https://codereview.chromium.org/2805433002 Cr-Commit-Position: refs/heads/master@{#462629} Committed: https://chromium.googlesource.com/chromium/src/+/c063fd910b9a39dbcf27d63ad5eaa02bcd3f6a71

Patch Set 1 #

Total comments: 25

Patch Set 2 : Formatting Revision #

Patch Set 3 : Clean things up #

Total comments: 7

Patch Set 4 : Refactoring #

Total comments: 17

Patch Set 5 : Renaming #

Total comments: 4

Patch Set 6 : Refactoring #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -4 lines) Patch
M components/arc/common/power.mojom View 1 2 3 4 4 chunks +11 lines, -3 lines 0 comments Download
M components/arc/power/arc_power_bridge.h View 1 2 3 4 5 3 chunks +5 lines, -0 lines 0 comments Download
M components/arc/power/arc_power_bridge.cc View 1 2 3 4 5 5 chunks +24 lines, -1 line 0 comments Download

Messages

Total messages: 44 (18 generated)
Muyuan
The general comment is that you should add owners of each folder otherwise the CL ...
3 years, 8 months ago (2017-04-05 18:41:13 UTC) #3
Daniel Erat
argh, why did https://codereview.chromium.org/2797063003/ get deleted? i left a bunch of comments on it. please ...
3 years, 8 months ago (2017-04-05 18:50:23 UTC) #5
Luis Héctor Chávez
drive-by https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power.mojom File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power.mojom#newcode32 components/arc/common/power.mojom:32: SetBrightness@3(int32 brightness); On 2017/04/05 18:41:08, Muyuan wrote: > ...
3 years, 8 months ago (2017-04-05 19:14:40 UTC) #7
victorhsieh0
https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power.mojom File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power.mojom#newcode35 components/arc/common/power.mojom:35: SyncBrightness@4(); nit: This name is a bit ambiguous to ...
3 years, 8 months ago (2017-04-05 19:14:51 UTC) #9
yueli
https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager_client.cc File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager_client.cc#newcode192 chromeos/dbus/power_manager_client.cc:192: weak_ptr_factory_.GetWeakPtr()); On 2017/04/05 18:41:08, Muyuan wrote: > You can ...
3 years, 8 months ago (2017-04-05 19:59:47 UTC) #11
Daniel Erat
i think you still need to upload a new revision of the patch (and send ...
3 years, 8 months ago (2017-04-05 20:05:17 UTC) #12
yueli
https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power.mojom File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power.mojom#newcode35 components/arc/common/power.mojom:35: SyncBrightness@4(); On 2017/04/05 19:14:40, Luis Héctor Chávez wrote: > ...
3 years, 8 months ago (2017-04-05 21:19:55 UTC) #16
Luis Héctor Chávez
https://codereview.chromium.org/2805433002/diff/40001/components/arc/common/power.mojom File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/40001/components/arc/common/power.mojom#newcode33 components/arc/common/power.mojom:33: [MinVersion=2] SetBrightness@3(int32 brightness); What happened to the request to ...
3 years, 8 months ago (2017-04-05 21:28:02 UTC) #17
Daniel Erat
https://codereview.chromium.org/2805433002/diff/40001/chromeos/dbus/power_manager_client.cc File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/2805433002/diff/40001/chromeos/dbus/power_manager_client.cc#newcode193 chromeos/dbus/power_manager_client.cc:193: } adding a new method isn't the right way ...
3 years, 8 months ago (2017-04-05 21:47:21 UTC) #18
yueli
https://codereview.chromium.org/2805433002/diff/40001/chromeos/dbus/power_manager_client.cc File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/2805433002/diff/40001/chromeos/dbus/power_manager_client.cc#newcode193 chromeos/dbus/power_manager_client.cc:193: } On 2017/04/05 21:47:21, Daniel Erat wrote: > adding ...
3 years, 8 months ago (2017-04-05 23:27:17 UTC) #19
Daniel Erat
https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/power.mojom File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/power.mojom#newcode51 components/arc/common/power.mojom:51: // Update android brightness settings. nit: s/android/Android/ https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/power.mojom#newcode53 components/arc/common/power.mojom:53: ...
3 years, 8 months ago (2017-04-05 23:43:38 UTC) #20
Luis Héctor Chávez
All right, this looks much better :) https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/power.mojom File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/power.mojom#newcode5 components/arc/common/power.mojom:5: // Next ...
3 years, 8 months ago (2017-04-05 23:55:28 UTC) #21
Luis Héctor Chávez
On 2017/04/05 23:55:28, Luis Héctor Chávez wrote: > All right, this looks much better :) ...
3 years, 8 months ago (2017-04-06 00:56:10 UTC) #22
yueli
https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/power.mojom File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/power.mojom#newcode5 components/arc/common/power.mojom:5: // Next min version: 3 On 2017/04/05 23:55:27, Luis ...
3 years, 8 months ago (2017-04-06 01:12:37 UTC) #23
Daniel Erat
thanks! lgtm but please wait for arc owner(s) to sign off on it too
3 years, 8 months ago (2017-04-06 01:14:57 UTC) #24
Luis Héctor Chávez
components/arc lgtm with two more nits. Typically when you send some code for review, you ...
3 years, 8 months ago (2017-04-06 01:36:20 UTC) #25
Luis Héctor Chávez
Ah wait, what happened to updating the commit message? That's missing.
3 years, 8 months ago (2017-04-06 01:48:33 UTC) #26
dcheng
mojom lgtm
3 years, 8 months ago (2017-04-06 05:39:53 UTC) #27
yueli
On 2017/04/06 01:36:20, Luis Héctor Chávez wrote: > components/arc lgtm with two more nits. > ...
3 years, 8 months ago (2017-04-06 16:41:00 UTC) #28
yueli
https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/arc_power_bridge.cc File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/arc_power_bridge.cc#newcode82 components/arc/power/arc_power_bridge.cc:82: void ArcPowerBridge::BrightnessChanged(int level, bool user_initiated) { On 2017/04/06 01:36:20, ...
3 years, 8 months ago (2017-04-06 16:43:00 UTC) #29
yueli
https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/arc_power_bridge.cc File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/arc_power_bridge.cc#newcode82 components/arc/power/arc_power_bridge.cc:82: void ArcPowerBridge::BrightnessChanged(int level, bool user_initiated) { On 2017/04/06 01:36:20, ...
3 years, 8 months ago (2017-04-06 16:43:00 UTC) #30
yueli
3 years, 8 months ago (2017-04-06 16:43:01 UTC) #31
yueli
3 years, 8 months ago (2017-04-06 16:43:04 UTC) #32
victorhsieh0
lgtm
3 years, 8 months ago (2017-04-06 16:47:40 UTC) #33
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/2805433002/100001
3 years, 8 months ago (2017-04-06 19:36:53 UTC) #40
commit-bot: I haz the power
3 years, 8 months ago (2017-04-06 21:31:21 UTC) #43
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c063fd910b9a39dbcf27d63ad5ea...

Powered by Google App Engine
This is Rietveld 408576698