Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1040473002:
[android] Create Battery Utils to seperate power functionality (Closed)
|
Created:
5 years, 9 months ago by rnephew (Reviews Here) Modified:
5 years, 8 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[android] Create Battery Utils to seperate power functionality
Move charging commands from device_utils to batter_utils.
Add GetPowerData() and GetPackagePowerData(package)
BUG=
Committed: https://crrev.com/9c524f0c7fdc20bec3fec8a2e3a779423e091782
Cr-Commit-Position: refs/heads/master@{#323099}
Patch Set 1 #
Total comments: 26
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Messages
Total messages: 28 (3 generated)
rnephew@google.com changed reviewers: + jbudorick@chromium.org, klundberg@chromium.org, perezju@chromium.org, rnephew@google.com
On 2015/03/26 22:04:59, rnephew wrote: Is this duplicating the implementation code from device_utils in battery_utils? Could you instead: - move clients, as needed, to call methods from battery_utils rather than device_utils; - or, if that may mean too many changes for this one CL, turn the corresponding methods in device_utils simply into wrappers around the actual implementations from batter_utils. In either case, you also probably want to remove the tests for battery methods inside device_utils_test after having moved them to battery_utils_test.
On 2015/03/27 10:13:04, perezju wrote: > On 2015/03/26 22:04:59, rnephew wrote: > > Is this duplicating the implementation code from device_utils in battery_utils? > > Could you instead: > - move clients, as needed, to call methods from battery_utils rather than > device_utils; > - or, if that may mean too many changes for this one CL, turn the corresponding > methods in device_utils simply into wrappers around the actual implementations > from batter_utils. > > In either case, you also probably want to remove the tests for battery methods > inside device_utils_test after having moved them to battery_utils_test. My intention was to do this CL, a second where I change all the uses of these functions to battery utils, and a third where I would remove the functions and tests from devie utils/tests. I can do it all in one CL if that is preferred though.
On 2015/03/27 12:57:54, rnephew wrote: > On 2015/03/27 10:13:04, perezju wrote: > > On 2015/03/26 22:04:59, rnephew wrote: > > > > Is this duplicating the implementation code from device_utils in > battery_utils? > > > > Could you instead: > > - move clients, as needed, to call methods from battery_utils rather than > > device_utils; > > - or, if that may mean too many changes for this one CL, turn the > corresponding > > methods in device_utils simply into wrappers around the actual implementations > > from batter_utils. > > > > In either case, you also probably want to remove the tests for battery methods > > inside device_utils_test after having moved them to battery_utils_test. > > My intention was to do this CL, a second where I change all the uses of these > functions to battery utils, and a third where I would remove the functions and > tests from devie utils/tests. > How many clients are there? > I can do it all in one CL if that is preferred though.
https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:85: raise TypeError('Must be initialized with device utils object.') s/device utils/DeviceUtils/ https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:98: { package_name: { nit: formatting. Drop the key onto its own line. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:111: # Wrong dumpsys version. This should _at least_ be logged. Perhaps it should be an exception; I'm not sure. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:117: assert current_package not in uid_entries Don't assert on data we're getting from the dumpsys output. If this is truly exceptional, raise an exception -- probably a CommandFailedError. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:119: elif (_PWI_POWER_CONSUMPTION_INDEX < len(entry) and 'and' should start the next line rather than end this line. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:125: out_dict = {} Why are we building this separately? Can we just build this in the CSV reading loop above? https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:140: { 'uid': uid, Same. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:144: return self.GetPowerData()[package] This should gracefully handle |package| not being in the power data. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:244: ['dumpsys', 'battery', 'set', 'usb', '1']) check_return=True https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:257: check_return=True) Nit: formatting. Here, I'd drop the command onto its own line., as you've done above. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:273: check_return=True) Same formatting nit. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:290: get_power_data() # report usage within this block What will this typically look like? Should BatteryMeasurement yield something that gets used to obtain this power data? https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1656: def GetCacheEntry(self, key): We talked about protecting cache users from overwriting each other's entries (and from overwriting DeviceUtils's entries). This does not do that. Perhaps that cache could be implemented in a separate class and DeviceUtils owns an instance and manages its lifetime?
On 2015/03/27 12:57:54, rnephew wrote: > On 2015/03/27 10:13:04, perezju wrote: > > On 2015/03/26 22:04:59, rnephew wrote: > > > > Is this duplicating the implementation code from device_utils in > battery_utils? > > > > Could you instead: > > - move clients, as needed, to call methods from battery_utils rather than > > device_utils; > > - or, if that may mean too many changes for this one CL, turn the > corresponding > > methods in device_utils simply into wrappers around the actual implementations > > from batter_utils. > > > > In either case, you also probably want to remove the tests for battery methods > > inside device_utils_test after having moved them to battery_utils_test. > > My intention was to do this CL, a second where I change all the uses of these > functions to battery utils, and a third where I would remove the functions and > tests from devie utils/tests. > > I can do it all in one CL if that is preferred though. I'm fine with 3 CL's, but do not duplicate code in the first one. Instead make the interface of device_utils point to the implementation in battery_utils, this will make much more explicit which pieces of code are being moved where, and reduces the possibility of the duplicated code diverging before we get to remove the extra copy.
On 2015/03/27 13:33:40, perezju wrote: > On 2015/03/27 12:57:54, rnephew wrote: > > On 2015/03/27 10:13:04, perezju wrote: > > > On 2015/03/26 22:04:59, rnephew wrote: > > > > > > Is this duplicating the implementation code from device_utils in > > battery_utils? > > > > > > Could you instead: > > > - move clients, as needed, to call methods from battery_utils rather than > > > device_utils; > > > - or, if that may mean too many changes for this one CL, turn the > > corresponding > > > methods in device_utils simply into wrappers around the actual > implementations > > > from batter_utils. > > > > > > In either case, you also probably want to remove the tests for battery > methods > > > inside device_utils_test after having moved them to battery_utils_test. > > > > My intention was to do this CL, a second where I change all the uses of these > > functions to battery utils, and a third where I would remove the functions and > > tests from devie utils/tests. > > > > I can do it all in one CL if that is preferred though. > > I'm fine with 3 CL's, but do not duplicate code in the first one. Instead make > the interface of device_utils point to the implementation in battery_utils, this > will make much more explicit which pieces of code are being moved where, and > reduces the possibility of the duplicated code diverging before we get to remove > the extra copy. That's a little awkward to do because BatteryUtils is implemented on top of DeviceUtils.
On 2015/03/27 13:35:57, jbudorick wrote: > On 2015/03/27 13:33:40, perezju wrote: > > On 2015/03/27 12:57:54, rnephew wrote: > > > On 2015/03/27 10:13:04, perezju wrote: > > > > On 2015/03/26 22:04:59, rnephew wrote: > > > > > > > > Is this duplicating the implementation code from device_utils in > > > battery_utils? > > > > > > > > Could you instead: > > > > - move clients, as needed, to call methods from battery_utils rather than > > > > device_utils; > > > > - or, if that may mean too many changes for this one CL, turn the > > > corresponding > > > > methods in device_utils simply into wrappers around the actual > > implementations > > > > from batter_utils. > > > > > > > > In either case, you also probably want to remove the tests for battery > > methods > > > > inside device_utils_test after having moved them to battery_utils_test. > > > > > > My intention was to do this CL, a second where I change all the uses of > these > > > functions to battery utils, and a third where I would remove the functions > and > > > tests from devie utils/tests. > > > > > > I can do it all in one CL if that is preferred though. > > > > I'm fine with 3 CL's, but do not duplicate code in the first one. Instead make > > the interface of device_utils point to the implementation in battery_utils, > this > > will make much more explicit which pieces of code are being moved where, and > > reduces the possibility of the duplicated code diverging before we get to > remove > > the extra copy. > > That's a little awkward to do because BatteryUtils is implemented on top of > DeviceUtils. I think you'd actually want to do the reverse: - add the battery utils interface, which: - fully implements any new functions - wraps any existing DeviceUtils function - switch client to the BatteryUtils interface - Move the implementations up to BatteryUtils
On 2015/03/27 13:37:31, jbudorick wrote: > On 2015/03/27 13:35:57, jbudorick wrote: > > On 2015/03/27 13:33:40, perezju wrote: > > > On 2015/03/27 12:57:54, rnephew wrote: > > > > On 2015/03/27 10:13:04, perezju wrote: > > > > > On 2015/03/26 22:04:59, rnephew wrote: > > > > > > > > > > Is this duplicating the implementation code from device_utils in > > > > battery_utils? > > > > > > > > > > Could you instead: > > > > > - move clients, as needed, to call methods from battery_utils rather > than > > > > > device_utils; > > > > > - or, if that may mean too many changes for this one CL, turn the > > > > corresponding > > > > > methods in device_utils simply into wrappers around the actual > > > implementations > > > > > from batter_utils. > > > > > > > > > > In either case, you also probably want to remove the tests for battery > > > methods > > > > > inside device_utils_test after having moved them to battery_utils_test. > > > > > > > > My intention was to do this CL, a second where I change all the uses of > > these > > > > functions to battery utils, and a third where I would remove the functions > > and > > > > tests from devie utils/tests. > > > > > > > > I can do it all in one CL if that is preferred though. > > > > > > I'm fine with 3 CL's, but do not duplicate code in the first one. Instead > make > > > the interface of device_utils point to the implementation in battery_utils, > > this > > > will make much more explicit which pieces of code are being moved where, and > > > reduces the possibility of the duplicated code diverging before we get to > > remove > > > the extra copy. > > > > That's a little awkward to do because BatteryUtils is implemented on top of > > DeviceUtils. > > I think you'd actually want to do the reverse: > - add the battery utils interface, which: > - fully implements any new functions > - wraps any existing DeviceUtils function > - switch client to the BatteryUtils interface > - Move the implementations up to BatteryUtils Yep, that would sound much better.
https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:85: raise TypeError('Must be initialized with device utils object.') On 2015/03/27 13:26:35, jbudorick wrote: > s/device utils/DeviceUtils/ Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:98: { package_name: { On 2015/03/27 13:26:35, jbudorick wrote: > nit: formatting. Drop the key onto its own line. Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:111: # Wrong dumpsys version. On 2015/03/27 13:26:35, jbudorick wrote: > This should _at least_ be logged. Perhaps it should be an exception; I'm not > sure. Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:117: assert current_package not in uid_entries On 2015/03/27 13:26:35, jbudorick wrote: > Don't assert on data we're getting from the dumpsys output. If this is truly > exceptional, raise an exception -- probably a CommandFailedError. Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:119: elif (_PWI_POWER_CONSUMPTION_INDEX < len(entry) and On 2015/03/27 13:26:36, jbudorick wrote: > 'and' should start the next line rather than end this line. Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:125: out_dict = {} On 2015/03/27 13:26:36, jbudorick wrote: > Why are we building this separately? Can we just build this in the CSV reading > loop above? Talked about this offline, cliff notes of the reasoning for record: I do not know if there is a guarantee that the package name -> uid mapping will appear before the power entries. That would make the logic to put it in the csv loop messier and harder to read. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:140: { 'uid': uid, On 2015/03/27 13:26:36, jbudorick wrote: > Same. Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:144: return self.GetPowerData()[package] On 2015/03/27 13:26:35, jbudorick wrote: > This should gracefully handle |package| not being in the power data. Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:244: ['dumpsys', 'battery', 'set', 'usb', '1']) On 2015/03/27 13:26:35, jbudorick wrote: > check_return=True Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:257: check_return=True) On 2015/03/27 13:26:35, jbudorick wrote: > Nit: formatting. > > Here, I'd drop the command onto its own line., as you've done above. Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:273: check_return=True) On 2015/03/27 13:26:35, jbudorick wrote: > Same formatting nit. Done. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:290: get_power_data() # report usage within this block On 2015/03/27 13:26:35, jbudorick wrote: > What will this typically look like? > > Should BatteryMeasurement yield something that gets used to obtain this power > data? My thoughts against that are, you could run GetPowerData() after the with block and get the power data for the entire with block. Thats why I wanted that function to be called, so that there aren't two ways to get the power data. https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1040473002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1656: def GetCacheEntry(self, key): On 2015/03/27 13:26:36, jbudorick wrote: > We talked about protecting cache users from overwriting each other's entries > (and from overwriting DeviceUtils's entries). This does not do that. > > Perhaps that cache could be implemented in a separate class and DeviceUtils owns > an instance and manages its lifetime? I've made it so it wont override what others put in, unless explicitly told to. I'll work on making it its own class as well.
https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:130: out_dict = {} maybe something like this would be more concise and a bit easier to read? return {p: {'uid': uid, 'data': pwi_entries[uid]} for p, uid in uid_entries.iteritems()} https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1657: def GetCacheEntry(self, key): I'm not sure I like this idea. Why would a class expose its private cache to the public? Seems like an invitation to mess with its internal state, and break assumptions that methods using the cache may be making about it. We may want a Cache class (if we really feel like we need some features not already provided by a simple dictionary), but even then I also think that the _cache object itself should be a private member. If BatteryUtils needs a cache, it should use its own private one, either some instance of a fancy Cache class or just a simple dictionary. This would also lessen the worry of "others" overriding what we put in the cache. Thoughts? (Also, BatteryUtils does not seem to be using this yet. So maybe material for another CL?)
https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1657: def GetCacheEntry(self, key): On 2015/03/30 11:05:23, perezju wrote: > I'm not sure I like this idea. Why would a class expose its private cache to the > public? Seems like an invitation to mess with its internal state, and break > assumptions that methods using the cache may be making about it. > > We may want a Cache class (if we really feel like we need some features not > already provided by a simple dictionary), but even then I also think that the > _cache object itself should be a private member. > > If BatteryUtils needs a cache, it should use its own private one, either some > instance of a fancy Cache class or just a simple dictionary. This would also > lessen the worry of "others" overriding what we put in the cache. Thoughts? > > (Also, BatteryUtils does not seem to be using this yet. So maybe material for > another CL?) BatteryUtils doesn't know how to control the lifetime of cache variables; DeviceUtils does. The cache functionality is the only reason GetCharging/SetCharging are in DeviceUtils. They'd otherwise be implemented separately (which is the driving reason behind this change, iirc). That said, the current implementation is still problematic. If DeviceUtils is providing the cache, the implementation needs to keep the client caches separate from its own cache.
On 2015/03/30 13:35:04, jbudorick wrote: > https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... > build/android/pylib/device/device_utils.py:1657: def GetCacheEntry(self, key): > On 2015/03/30 11:05:23, perezju wrote: > > I'm not sure I like this idea. Why would a class expose its private cache to > the > > public? Seems like an invitation to mess with its internal state, and break > > assumptions that methods using the cache may be making about it. > > > > We may want a Cache class (if we really feel like we need some features not > > already provided by a simple dictionary), but even then I also think that the > > _cache object itself should be a private member. > > > > If BatteryUtils needs a cache, it should use its own private one, either some > > instance of a fancy Cache class or just a simple dictionary. This would also > > lessen the worry of "others" overriding what we put in the cache. Thoughts? > > > > (Also, BatteryUtils does not seem to be using this yet. So maybe material for > > another CL?) > > BatteryUtils doesn't know how to control the lifetime of cache variables; > DeviceUtils does. The cache functionality is the only reason > GetCharging/SetCharging are in DeviceUtils. They'd otherwise be implemented > separately (which is the driving reason behind this change, iirc). > > That said, the current implementation is still problematic. If DeviceUtils is > providing the cache, the implementation needs to keep the client caches separate > from its own cache. Got it. Here is a suggestion. Make the constructor of BatteryUtils get a device and a cache object. BatteryUtils should keep this in, e.g., _cache, and add/remove values to it at will. Add a DeviceUtils.GetBatteryUtils() that: - creates a new cache object - stores the cache object on its own, e.g., _client_caches['battery_utils'] (many BatteryUtils instances from the same device should be able to share the same cache, right?) - passes itself and the corresponding cache object to the BatteryUtils constructor. Add a DeviceUtils.ClearCahces() that: - resets its own _cache, and loops over _client_caches calling .clear() on all of them. I suggest we use a simple dictionary as the cache object. If we later we need anything fancier, we can have a Cache class sub-classing collections.MutableMapping.
On 2015/03/30 14:12:10, perezju wrote: > On 2015/03/30 13:35:04, jbudorick wrote: > > > https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... > > File build/android/pylib/device/device_utils.py (right): > > > > > https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... > > build/android/pylib/device/device_utils.py:1657: def GetCacheEntry(self, key): > > On 2015/03/30 11:05:23, perezju wrote: > > > I'm not sure I like this idea. Why would a class expose its private cache to > > the > > > public? Seems like an invitation to mess with its internal state, and break > > > assumptions that methods using the cache may be making about it. > > > > > > We may want a Cache class (if we really feel like we need some features not > > > already provided by a simple dictionary), but even then I also think that > the > > > _cache object itself should be a private member. > > > > > > If BatteryUtils needs a cache, it should use its own private one, either > some > > > instance of a fancy Cache class or just a simple dictionary. This would also > > > lessen the worry of "others" overriding what we put in the cache. Thoughts? > > > > > > (Also, BatteryUtils does not seem to be using this yet. So maybe material > for > > > another CL?) > > > > BatteryUtils doesn't know how to control the lifetime of cache variables; > > DeviceUtils does. The cache functionality is the only reason > > GetCharging/SetCharging are in DeviceUtils. They'd otherwise be implemented > > separately (which is the driving reason behind this change, iirc). > > > > That said, the current implementation is still problematic. If DeviceUtils is > > providing the cache, the implementation needs to keep the client caches > separate > > from its own cache. > > Got it. > > Here is a suggestion. Make the constructor of BatteryUtils get a device and a > cache object. BatteryUtils should keep this in, e.g., _cache, and add/remove > values to it at will. > > Add a DeviceUtils.GetBatteryUtils() that: > - creates a new cache object > - stores the cache object on its own, e.g., _client_caches['battery_utils'] > (many BatteryUtils instances from the same device should be able to share > the same cache, right?) > - passes itself and the corresponding cache object to the BatteryUtils > constructor. > > Add a DeviceUtils.ClearCahces() that: > - resets its own _cache, and loops over _client_caches calling .clear() on all > of them. > > I suggest we use a simple dictionary as the cache object. If we later we need > anything fancier, we can have a Cache class sub-classing > collections.MutableMapping. Alternatively, we might have a DeviceUtils.GetClientCache(client_name) method with similar functionality. So that DeviceUtils is not forced to know in advance about all possible helper objects that might need such a client cache.
On 2015/03/30 14:14:47, perezju wrote: > On 2015/03/30 14:12:10, perezju wrote: > > On 2015/03/30 13:35:04, jbudorick wrote: > > > > > > https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... > > > File build/android/pylib/device/device_utils.py (right): > > > > > > > > > https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... > > > build/android/pylib/device/device_utils.py:1657: def GetCacheEntry(self, > key): > > > On 2015/03/30 11:05:23, perezju wrote: > > > > I'm not sure I like this idea. Why would a class expose its private cache > to > > > the > > > > public? Seems like an invitation to mess with its internal state, and > break > > > > assumptions that methods using the cache may be making about it. > > > > > > > > We may want a Cache class (if we really feel like we need some features > not > > > > already provided by a simple dictionary), but even then I also think that > > the > > > > _cache object itself should be a private member. > > > > > > > > If BatteryUtils needs a cache, it should use its own private one, either > > some > > > > instance of a fancy Cache class or just a simple dictionary. This would > also > > > > lessen the worry of "others" overriding what we put in the cache. > Thoughts? > > > > > > > > (Also, BatteryUtils does not seem to be using this yet. So maybe material > > for > > > > another CL?) > > > > > > BatteryUtils doesn't know how to control the lifetime of cache variables; > > > DeviceUtils does. The cache functionality is the only reason > > > GetCharging/SetCharging are in DeviceUtils. They'd otherwise be implemented > > > separately (which is the driving reason behind this change, iirc). > > > > > > That said, the current implementation is still problematic. If DeviceUtils > is > > > providing the cache, the implementation needs to keep the client caches > > separate > > > from its own cache. > > > > Got it. > > > > Here is a suggestion. Make the constructor of BatteryUtils get a device and a > > cache object. BatteryUtils should keep this in, e.g., _cache, and add/remove > > values to it at will. > > > > Add a DeviceUtils.GetBatteryUtils() that: > > - creates a new cache object > > - stores the cache object on its own, e.g., _client_caches['battery_utils'] > > (many BatteryUtils instances from the same device should be able to share > > the same cache, right?) > > - passes itself and the corresponding cache object to the BatteryUtils > > constructor. I don't like this part of this idea. BatteryUtils is implemented on top of DeviceUtils; giving DeviceUtils a BatteryUtils factory function leaves us with some circular dependencies. > > > > Add a DeviceUtils.ClearCahces() that: > > - resets its own _cache, and loops over _client_caches calling .clear() on all > > of them. > > > > I suggest we use a simple dictionary as the cache object. If we later we need > > anything fancier, we can have a Cache class sub-classing > > collections.MutableMapping. > > Alternatively, we might have a DeviceUtils.GetClientCache(client_name) method > with similar functionality. So that DeviceUtils is not forced to know in advance > about all possible helper objects that might need such a client cache. Yeah, I don't think DeviceUtils should know who needs a cache.
On 2015/03/30 14:18:19, jbudorick wrote: > Yeah, I don't think DeviceUtils should know who needs a cache. Agreed, so DeviceUtils.GetClientCache(client_name) and DeviceUtils.ClearCaches() should be all we need.
On 2015/03/30 14:28:26, perezju wrote: > On 2015/03/30 14:18:19, jbudorick wrote: > > Yeah, I don't think DeviceUtils should know who needs a cache. > > Agreed, so DeviceUtils.GetClientCache(client_name) and > DeviceUtils.ClearCaches() should be all we need. Does ClearCaches need to be public? I think the only callers would be in the DeviceUtils instance.
On 2015/03/30 14:30:26, jbudorick wrote: > On 2015/03/30 14:28:26, perezju wrote: > > On 2015/03/30 14:18:19, jbudorick wrote: > > > Yeah, I don't think DeviceUtils should know who needs a cache. > > > > Agreed, so DeviceUtils.GetClientCache(client_name) and > > DeviceUtils.ClearCaches() should be all we need. > > Does ClearCaches need to be public? I think the only callers would be in the > DeviceUtils instance. +1 for _ClearCaches then :)
https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:130: out_dict = {} On 2015/03/30 11:05:23, perezju wrote: > maybe something like this would be more concise and a bit easier to read? > > return {p: {'uid': uid, 'data': pwi_entries[uid]} for p, uid in > uid_entries.iteritems()} Done. https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1040473002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1657: def GetCacheEntry(self, key): On 2015/03/30 13:35:03, jbudorick wrote: > On 2015/03/30 11:05:23, perezju wrote: > > I'm not sure I like this idea. Why would a class expose its private cache to > the > > public? Seems like an invitation to mess with its internal state, and break > > assumptions that methods using the cache may be making about it. > > > > We may want a Cache class (if we really feel like we need some features not > > already provided by a simple dictionary), but even then I also think that the > > _cache object itself should be a private member. > > > > If BatteryUtils needs a cache, it should use its own private one, either some > > instance of a fancy Cache class or just a simple dictionary. This would also > > lessen the worry of "others" overriding what we put in the cache. Thoughts? > > > > (Also, BatteryUtils does not seem to be using this yet. So maybe material for > > another CL?) It will use this when it stops passing the calls through to device_utils and does everything in its own implementation. > > BatteryUtils doesn't know how to control the lifetime of cache variables; > DeviceUtils does. The cache functionality is the only reason > GetCharging/SetCharging are in DeviceUtils. They'd otherwise be implemented > separately (which is the driving reason behind this change, iirc). > > That said, the current implementation is still problematic. If DeviceUtils is > providing the cache, the implementation needs to keep the client caches separate > from its own cache. Done.
thanks! lgtm with an optional nit https://codereview.chromium.org/1040473002/diff/60001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1040473002/diff/60001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:132: 'data': pwi_entries[uid]} for p, uid in uid_entries.iteritems()} optional nit: if you have to break it in two lines, I would go for return {p: {'uid': uid, 'data': pwi_entries[uid]} for p, uid in uid_entries.iteritems()} but either way is fine.
lgtm w/ nits https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:243: raise device_errors.CommandFailedError('Device must be L or higher.') nit: Should this be a DeviceVersionError? https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_errors.py (right): https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_errors.py:79: class DeviceVersionError(CommandFailedError): nit: move this up, either before AdbCommandFailedError or before CommandTimeoutError. https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1657: # TODO(rnephew): Implement cache class. nit: Do we need to?
https://codereview.chromium.org/1040473002/diff/60001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1040473002/diff/60001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:132: 'data': pwi_entries[uid]} for p, uid in uid_entries.iteritems()} On 2015/03/31 08:33:25, perezju wrote: > optional nit: if you have to break it in two lines, I would go for > > return {p: {'uid': uid, 'data': pwi_entries[uid]} > for p, uid in uid_entries.iteritems()} > > but either way is fine. Done. https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:243: raise device_errors.CommandFailedError('Device must be L or higher.') On 2015/03/31 18:30:19, jbudorick wrote: > nit: Should this be a DeviceVersionError? Done. https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_errors.py (right): https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_errors.py:79: class DeviceVersionError(CommandFailedError): On 2015/03/31 18:30:19, jbudorick wrote: > nit: move this up, either before AdbCommandFailedError or before > CommandTimeoutError. Done. https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1040473002/diff/80001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1657: # TODO(rnephew): Implement cache class. On 2015/03/31 18:30:19, jbudorick wrote: > nit: Do we need to? Done.
The CQ bit was checked by rnephew@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1040473002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1040473002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9c524f0c7fdc20bec3fec8a2e3a779423e091782 Cr-Commit-Position: refs/heads/master@{#323099} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
