|
|
Created:
3 years, 8 months ago by yueli Modified:
3 years, 8 months ago Reviewers:
Yusuke Sato, Daniel Erat, xiyuan, Luis Héctor Chávez, Eric Caruso, dcheng, xc, Muyuan, Vladislav Kaznacheev, victorhsieh, victorhsieh0 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. |
DescriptionAdding 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 #
Messages
Total messages: 44 (18 generated)
Description was changed from ========== 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 on android side, SyncBrightness() will be called to get brightness in OS and notify android side. - When changes occur for brightness setting on android side, SetBrightness(.) will be called to adjust the OS brightness. - When OS brightness was changed, UpdateBrightness(.) will be called to notify android side about the change. BUG=b/36129958 ========== to ========== 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 on android side, SyncBrightness() will be called to get brightness in OS and notify android side. - When changes occur for brightness setting on android side, SetBrightness(.) will be called to adjust the OS brightness. - When OS brightness was changed, UpdateBrightness(.) will be called to notify android side about the change. BUG=b/36129958 ==========
updowndota@chromium.org changed reviewers: + ejcaruso@chromium.org, muyuanli@chromium.org, victorhsieh@chromium.org, xiaohuic@chromium.org
The general comment is that you should add owners of each folder otherwise the CL will not pass CQ. Also could you please run: git cl try for presubmit checks? https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... chromeos/dbus/power_manager_client.cc:192: weak_ptr_factory_.GetWeakPtr()); You can inline callback into next line. Also there is no need for ref const GetScreenBrightnessPercentCallback& https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... File chromeos/dbus/power_manager_client.h (right): https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... chromeos/dbus/power_manager_client.h:64: // Noitify android setting about brightness change dbus/power_manager_client is pretty general and it exists in many platforms that you shouldn't say "notify android".. https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... chromeos/dbus/power_manager_client.h:65: virtual void NotifyBrightnessChange(double brightness) {} This function is probably not needed. You can override BrightnessChanged in ArcPowerBridge https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:22: // Next method ID: 3 Next Method ID = max methodid in interface + 1 https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:31: // Set screen brightness. Same as L22 https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:32: SetBrightness@3(int32 brightness); add MinVersion https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:35: SyncBrightness@4(); Same https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:54: UpdateBrightness@4(int32 brightness); Same as L32 https://codereview.chromium.org/2805433002/diff/1/components/arc/power/arc_po... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/power/arc_po... components/arc/power/arc_power_bridge.cc:83: int brightness = round(percent * 2.55); Document why multiply this by 2.55 https://codereview.chromium.org/2805433002/diff/1/components/arc/power/arc_po... File components/arc/power/arc_power_bridge.h (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/power/arc_po... components/arc/power/arc_power_bridge.h:45: void NotifyBrightnessChange(double percent) override; Just override BrightnessChanged.
derat@chromium.org changed reviewers: + derat@chromium.org
argh, why did https://codereview.chromium.org/2797063003/ get deleted? i left a bunch of comments on it. please let me know if you need me to forward them to you via email. i think they all apply to this version of the change too.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:32: SetBrightness@3(int32 brightness); On 2017/04/05 18:41:08, Muyuan wrote: > add MinVersion Also document the range of |brightness|. I'd rather have you send the brightness as a percentage to avoid conversions here and do them on Android, so change this to |double percentage|. https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:35: SyncBrightness@4(); Why is this needed? Can't you always send the initial brightness when the Android side connects? https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:54: UpdateBrightness@4(int32 brightness); Same as L32: please send the brightness as a double percentage.
victorhsieh@google.com changed reviewers: + victorhsieh@google.com
https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:35: SyncBrightness@4(); nit: This name is a bit ambiguous to me. Would RequestBrightness be more clear? Also, would be nice to mention the instance's UpdateBrightness will be called in the comment.
updowndota@chromium.org changed reviewers: + xiyuan@chromium.org, yusukes@chromium.org
https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... chromeos/dbus/power_manager_client.cc:192: weak_ptr_factory_.GetWeakPtr()); On 2017/04/05 18:41:08, Muyuan wrote: > You can inline callback into next line. Also there is no need for ref const > GetScreenBrightnessPercentCallback& Done. https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... File chromeos/dbus/power_manager_client.h (right): https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... chromeos/dbus/power_manager_client.h:64: // Noitify android setting about brightness change On 2017/04/05 18:41:08, Muyuan wrote: > dbus/power_manager_client is pretty general and it exists in many platforms that > you shouldn't say "notify android".. Done. https://codereview.chromium.org/2805433002/diff/1/chromeos/dbus/power_manager... chromeos/dbus/power_manager_client.h:65: virtual void NotifyBrightnessChange(double brightness) {} On 2017/04/05 18:41:08, Muyuan wrote: > This function is probably not needed. You can override BrightnessChanged in > ArcPowerBridge Done. https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:22: // Next method ID: 3 On 2017/04/05 18:41:08, Muyuan wrote: > Next Method ID = max methodid in interface + 1 Done. https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:31: // Set screen brightness. On 2017/04/05 18:41:08, Muyuan wrote: > Same as L22 Done. https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:32: SetBrightness@3(int32 brightness); On 2017/04/05 18:41:08, Muyuan wrote: > add MinVersion Done. https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:35: SyncBrightness@4(); On 2017/04/05 18:41:10, Muyuan wrote: > Same Done. https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:54: UpdateBrightness@4(int32 brightness); On 2017/04/05 18:41:08, Muyuan wrote: > Same as L32 Done. https://codereview.chromium.org/2805433002/diff/1/components/arc/power/arc_po... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/power/arc_po... components/arc/power/arc_power_bridge.cc:83: int brightness = round(percent * 2.55); On 2017/04/05 18:41:10, Muyuan wrote: > Document why multiply this by 2.55 Done. https://codereview.chromium.org/2805433002/diff/1/components/arc/power/arc_po... File components/arc/power/arc_power_bridge.h (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/power/arc_po... components/arc/power/arc_power_bridge.h:45: void NotifyBrightnessChange(double percent) override; On 2017/04/05 18:41:11, Muyuan wrote: > Just override BrightnessChanged. Done.
i think you still need to upload a new revision of the patch (and send email after doing that so reviewers know to take another look).
updowndota@chromium.org changed reviewers: + dcheng@chromium.org, kaznacheev@chromium.org
The CQ bit was checked by updowndota@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/1/components/arc/common/power... components/arc/common/power.mojom:35: SyncBrightness@4(); On 2017/04/05 19:14:40, Luis Héctor Chávez wrote: > Why is this needed? Can't you always send the initial brightness when the > Android side connects? Done.
https://codereview.chromium.org/2805433002/diff/40001/components/arc/common/p... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/40001/components/arc/common/p... components/arc/common/power.mojom:33: [MinVersion=2] SetBrightness@3(int32 brightness); What happened to the request to make this |double percentage| instead of |int32 brightness|? https://codereview.chromium.org/2805433002/diff/40001/components/arc/power/ar... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/40001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:39: ->SyncScreenBrightness(); You don't need SyncScreenBrightness() either :) Instead you can do chromeos::DBusThreadManager::Get() ->GetPowerManagerClient() ->GetGetScreenBrightnessPercent( base::Bind(&ArcPowerBridge::UpdateBrightness, weak_ptr_factory_.GetWeakPtr())); then declare a private method void ArcPowerBridge::UpdateBrightness(double percent) { mojom::PowerInstance* power_instance = ARC_GET_INSTANCE_FOR_METHOD( arc_bridge_service()->power(), UpdateBrightness); if (!power_instance) return; power_instance->UpdateBrightness(percent); } And call it from the callback: void ArcPowerBridge::BrightnessChanged(int level, double percent) { UpdateBrightness(percent); } See other places where a base::WeakPtrFactory is used to see how you need to declare it.
https://codereview.chromium.org/2805433002/diff/40001/chromeos/dbus/power_man... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/2805433002/diff/40001/chromeos/dbus/power_man... chromeos/dbus/power_manager_client.cc:193: } adding a new method isn't the right way to do this. if your code needs to get the current brightness from powerd, it should call the existing GetScreenBrightnessPercent method. it doesn't make sense to add a new SyncScreenBrightness method that just calls the existing public method. (i see that luis left a comment on another file suggesting the same approach.) https://codereview.chromium.org/2805433002/diff/40001/components/arc/common/p... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/40001/components/arc/common/p... components/arc/common/power.mojom:33: [MinVersion=2] SetBrightness@3(int32 brightness); On 2017/04/05 21:28:02, Luis Héctor Chávez wrote: > What happened to the request to make this |double percentage| instead of |int32 > brightness|? agreed; "double percentage" is better.
https://codereview.chromium.org/2805433002/diff/40001/chromeos/dbus/power_man... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/2805433002/diff/40001/chromeos/dbus/power_man... chromeos/dbus/power_manager_client.cc:193: } On 2017/04/05 21:47:21, Daniel Erat wrote: > adding a new method isn't the right way to do this. > > if your code needs to get the current brightness from powerd, it should call the > existing GetScreenBrightnessPercent method. it doesn't make sense to add a new > SyncScreenBrightness method that just calls the existing public method. > > (i see that luis left a comment on another file suggesting the same approach.) Done. https://codereview.chromium.org/2805433002/diff/40001/components/arc/common/p... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/40001/components/arc/common/p... components/arc/common/power.mojom:33: [MinVersion=2] SetBrightness@3(int32 brightness); On 2017/04/05 21:28:02, Luis Héctor Chávez wrote: > What happened to the request to make this |double percentage| instead of |int32 > brightness|? Done. https://codereview.chromium.org/2805433002/diff/40001/components/arc/power/ar... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/40001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:39: ->SyncScreenBrightness(); On 2017/04/05 21:28:02, Luis Héctor Chávez wrote: > You don't need SyncScreenBrightness() either :) Instead you can do > > chromeos::DBusThreadManager::Get() > ->GetPowerManagerClient() > ->GetGetScreenBrightnessPercent( > base::Bind(&ArcPowerBridge::UpdateBrightness, > weak_ptr_factory_.GetWeakPtr())); > > then declare a private method > > void ArcPowerBridge::UpdateBrightness(double percent) { > mojom::PowerInstance* power_instance = ARC_GET_INSTANCE_FOR_METHOD( > arc_bridge_service()->power(), UpdateBrightness); > if (!power_instance) > return; > power_instance->UpdateBrightness(percent); > } > > And call it from the callback: > > void ArcPowerBridge::BrightnessChanged(int level, double percent) { > UpdateBrightness(percent); > } > > See other places where a base::WeakPtrFactory is used to see how you need to > declare it. Thanks Luis! Done.
https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... components/arc/common/power.mojom:51: // Update android brightness settings. nit: s/android/Android/ https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... components/arc/common/power.mojom:53: [MinVersion=3] UpdateBrightness@4(double percent); would UpdateBrightnessSettings be clearer? it's hard to know from the method name that this just updates the settings without updating the actual display brightness. https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:86: power_instance->UpdateBrightness(percent); nit: just do: if (power_instance) power_instance->UpdateBrightness(percent); https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:89: void ArcPowerBridge::BrightnessChanged(int level, bool user_initiated) { please make order of methods in .cc files match order in .h https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:149: return; remove pointless return statement https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... File components/arc/power/arc_power_bridge.h (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.h:55: void UpdateBrightness(double percent); nit: rename to UpdateAndroidBrightness to make it clearer what this actually does
All right, this looks much better :) https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... components/arc/common/power.mojom:5: // Next min version: 3 nit: Next min version: 4 (your modification is version 3). https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... components/arc/common/power.mojom:33: [MinVersion=2] SetBrightness@3(double percent); MinVersion=3 (all the modifications in this change need to use the same MinVersion. Your version is 3). Also, how about you change this to // Requests that the screen brightness be changed to |percent|. [MinVersion=3] OnScreenBrightnessUpdateRequest@3(double percent). https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... components/arc/common/power.mojom:53: [MinVersion=3] UpdateBrightness@4(double percent); On 2017/04/05 23:43:38, Daniel Erat wrote: > would UpdateBrightnessSettings be clearer? it's hard to know from the method > name that this just updates the settings without updating the actual display > brightness. Can you also throw in "Screen" somewhere in the method name? https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... File components/arc/power/arc_power_bridge.h (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.h:51: void SetBrightness(double brightness) override; nit: s/double brightness/double percent/ https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.h:55: void UpdateBrightness(double percent); On 2017/04/05 23:43:38, Daniel Erat wrote: > nit: rename to UpdateAndroidBrightness to make it clearer what this actually > does Same thing, "Screen" somewhere.
On 2017/04/05 23:55:28, Luis Héctor Chávez wrote: > All right, this looks much better :) > > https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... > File components/arc/common/power.mojom (right): > > https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... > components/arc/common/power.mojom:5: // Next min version: 3 > nit: > > Next min version: 4 > > (your modification is version 3). > > https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... > components/arc/common/power.mojom:33: [MinVersion=2] SetBrightness@3(double > percent); > MinVersion=3 > > (all the modifications in this change need to use the same MinVersion. Your > version is 3). > > Also, how about you change this to > > // Requests that the screen brightness be changed to |percent|. > [MinVersion=3] OnScreenBrightnessUpdateRequest@3(double percent). > > https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... > components/arc/common/power.mojom:53: [MinVersion=3] UpdateBrightness@4(double > percent); > On 2017/04/05 23:43:38, Daniel Erat wrote: > > would UpdateBrightnessSettings be clearer? it's hard to know from the method > > name that this just updates the settings without updating the actual display > > brightness. > > Can you also throw in "Screen" somewhere in the method name? > > https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... > File components/arc/power/arc_power_bridge.h (right): > > https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... > components/arc/power/arc_power_bridge.h:51: void SetBrightness(double > brightness) override; > nit: s/double brightness/double percent/ > > https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... > components/arc/power/arc_power_bridge.h:55: void UpdateBrightness(double > percent); > On 2017/04/05 23:43:38, Daniel Erat wrote: > > nit: rename to UpdateAndroidBrightness to make it clearer what this actually > > does > > Same thing, "Screen" somewhere. Ah right, once you make all these changes, you'll need to click on "Edit Issue" and change the commit message. `git cl upload` won't update it if you amend it locally.
https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... File components/arc/common/power.mojom (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... components/arc/common/power.mojom:5: // Next min version: 3 On 2017/04/05 23:55:27, Luis Héctor Chávez wrote: > nit: > > Next min version: 4 > > (your modification is version 3). Done. https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... components/arc/common/power.mojom:33: [MinVersion=2] SetBrightness@3(double percent); On 2017/04/05 23:55:27, Luis Héctor Chávez wrote: > MinVersion=3 > > (all the modifications in this change need to use the same MinVersion. Your > version is 3). > > Also, how about you change this to > > // Requests that the screen brightness be changed to |percent|. > [MinVersion=3] OnScreenBrightnessUpdateRequest@3(double percent). Done. https://codereview.chromium.org/2805433002/diff/60001/components/arc/common/p... components/arc/common/power.mojom:53: [MinVersion=3] UpdateBrightness@4(double percent); On 2017/04/05 23:43:38, Daniel Erat wrote: > would UpdateBrightnessSettings be clearer? it's hard to know from the method > name that this just updates the settings without updating the actual display > brightness. Done. https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:89: void ArcPowerBridge::BrightnessChanged(int level, bool user_initiated) { On 2017/04/05 23:43:38, Daniel Erat wrote: > please make order of methods in .cc files match order in .h Done. https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:149: return; On 2017/04/05 23:43:38, Daniel Erat wrote: > remove pointless return statement Done. https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... File components/arc/power/arc_power_bridge.h (right): https://codereview.chromium.org/2805433002/diff/60001/components/arc/power/ar... components/arc/power/arc_power_bridge.h:55: void UpdateBrightness(double percent); On 2017/04/05 23:43:38, Daniel Erat wrote: > nit: rename to UpdateAndroidBrightness to make it clearer what this actually > does Done.
thanks! lgtm but please wait for arc owner(s) to sign off on it too
components/arc lgtm with two more nits. Typically when you send some code for review, you explicitly enumerate who is supposed to look at which part of the code (`git cl owners` can help you find those people). Avoid adding too many people to the reviewers list because it creates confusion (and since people see that there's other reviewers, they'll probably defer to somebody else. avoiding work is the path of least resistance!). For instance, in this case, with the pruned list of files you should have sent out a message like: """ PTAL dcheng@ *.mojom lhchavez@ components/arc """ https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:82: void ArcPowerBridge::BrightnessChanged(int level, bool user_initiated) { This should also be moved to L70. https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... File components/arc/power/arc_power_bridge.h (right): https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... components/arc/power/arc_power_bridge.h:45: void BrightnessChanged(int level, bool user_initiated) override; argh, one more nit: Move this to after L39, with the rest of the PowerManagerClient::Observer overrides.
Ah wait, what happened to updating the commit message? That's missing.
mojom lgtm
On 2017/04/06 01:36:20, Luis Héctor Chávez wrote: > components/arc lgtm with two more nits. > > Typically when you send some code for review, you explicitly enumerate who is > supposed to look at which part of the code (`git cl owners` can help you find > those people). Avoid adding too many people to the reviewers list because it > creates confusion (and since people see that there's other reviewers, they'll > probably defer to somebody else. avoiding work is the path of least > resistance!). > > For instance, in this case, with the pruned list of files you should have sent > out a message like: > > """ > PTAL > > dcheng@ *.mojom > lhchavez@ components/arc > """ > > https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... > File components/arc/power/arc_power_bridge.cc (right): > > https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... > components/arc/power/arc_power_bridge.cc:82: void > ArcPowerBridge::BrightnessChanged(int level, bool user_initiated) { > This should also be moved to L70. > > https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... > File components/arc/power/arc_power_bridge.h (right): > > https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... > components/arc/power/arc_power_bridge.h:45: void BrightnessChanged(int level, > bool user_initiated) override; > argh, one more nit: Move this to after L39, with the rest of the > PowerManagerClient::Observer overrides. Thanks Luis, that's really helpful!:D
https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:82: void ArcPowerBridge::BrightnessChanged(int level, bool user_initiated) { On 2017/04/06 01:36:20, Luis Héctor Chávez wrote: > This should also be moved to L70. Done. https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... File components/arc/power/arc_power_bridge.h (right): https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... components/arc/power/arc_power_bridge.h:45: void BrightnessChanged(int level, bool user_initiated) override; On 2017/04/06 01:36:20, Luis Héctor Chávez wrote: > argh, one more nit: Move this to after L39, with the rest of the > PowerManagerClient::Observer overrides. Done.
https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... File components/arc/power/arc_power_bridge.cc (right): https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... components/arc/power/arc_power_bridge.cc:82: void ArcPowerBridge::BrightnessChanged(int level, bool user_initiated) { On 2017/04/06 01:36:20, Luis Héctor Chávez wrote: > This should also be moved to L70. Done. https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... File components/arc/power/arc_power_bridge.h (right): https://codereview.chromium.org/2805433002/diff/80001/components/arc/power/ar... components/arc/power/arc_power_bridge.h:45: void BrightnessChanged(int level, bool user_initiated) override; On 2017/04/06 01:36:20, Luis Héctor Chávez wrote: > argh, one more nit: Move this to after L39, with the rest of the > PowerManagerClient::Observer overrides. Done.
lgtm
Description was changed from ========== 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 on android side, SyncBrightness() will be called to get brightness in OS and notify android side. - When changes occur for brightness setting on android side, SetBrightness(.) will be called to adjust the OS brightness. - When OS brightness was changed, UpdateBrightness(.) will be called to notify android side about the change. BUG=b/36129958 ========== to ========== 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 ==========
The CQ bit was checked by updowndota@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, lhchavez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2805433002/#ps100001 (title: "Refactoring")
The CQ bit was unchecked by updowndota@chromium.org
The CQ bit was checked by updowndota@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, lhchavez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2805433002/#ps100001 (title: "Refactoring")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491507344302570, "parent_rev": "ed6da57dfdcee72b4fcc5e925b6b8e6384d500f4", "commit_rev": "c063fd910b9a39dbcf27d63ad5eaa02bcd3f6a71"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/c063fd910b9a39dbcf27d63ad5ea... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c063fd910b9a39dbcf27d63ad5ea...
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted |