Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1432093002:
Improve readability of power monitoring code and add additional (Closed)
|
Created:
5 years, 1 month ago by alexandermont Modified:
5 years, 1 month ago CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove readability of power monitoring code and add additional
logging and validation.
Committed: https://crrev.com/adce66abd74fb2d6e22da36eee85b7e324f962ba
Cr-Commit-Position: refs/heads/master@{#359640}
Patch Set 1 #
Total comments: 18
Patch Set 2 : update based on comments in code review #
Total comments: 12
Patch Set 3 : improve readability of power monitoring code and add logging, validation, and more unit tests #
Total comments: 24
Patch Set 4 : code review updates #
Total comments: 26
Patch Set 5 : Fix more issues from code review. #
Total comments: 2
Patch Set 6 : fix final nit #Patch Set 7 : rebase #Patch Set 8 : fix issue 556653 #Messages
Total messages: 29 (5 generated)
alexandermont@chromium.org changed reviewers: + nednguyen@google.com, rnephew@google.com
Cleanup of power monitoring code
https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:33: assert not self._monitoring, "Already monitoring power." Their design pattern for this is that it should raise NotImplementedError if these classes are not overridden. These should be changed back to remain consistent with the rest of telemetry. Unless Ned feels otherwise and is ok with this change; then ignore this opinion. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py (left): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:31: DUMP_VERSION_INDEX = 0 This is here so people reading the code know the reason for using 0, and we aren't using magic numbers. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:44: self._battery.TieredSetCharging(False) I am confused at how this is here, I got rid of TieredSetCharging and when I look locally it isn't there. You might need to sync and rebase. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:9: from telemetry.internal.platform.power_monitor import power_monitor_utils as pmu We typically do not do this with imports. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:32: # Dumpsys power data is present in dumpsys versions 8 and 9 Move these comments to before return statement. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor.py:6: from telemetry.internal.platform.power_monitor import power_monitor_utils as pmu Same as dumpsys. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:45: logging.warning("""String returned from device.ReadFile(_TEMPERATURE_FILE) Same formatting issue as the other logging message. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py:42: logging.warning(""""Device not on battery power during power monitoring. Same formatting nit. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py:56: logging.warning("""Device not on battery power during power monitoring. Same formatting nit. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py (left): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:31: assert not self._active_monitors, 'Must call StopMonitoringPower().' Do not get rid of this. Its there to make sure things are getting called properly. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:66: assert self._active_monitors, 'StartMonitoringPower() not called.' Same. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_utils.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_utils.py:1: import logging Instead of making this its own thing, maybe make a class that the android power monitors inherit from that have these functions. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_utils.py:32: Results may be inaccurate.""") Should be in the form: logging.warning('Charging re-enabled during test. 'Results may be inaccurate.') or logging.warning( 'Charging re-enabled during test. Results may be inaccurate.') https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py:55: def StartMonitoringPower(self, browser): put the _ back in front of browser. Its to signify its not used.
rnephew@google.com changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:33: assert not self._monitoring, "Already monitoring power." On 2015/11/10 19:17:52, rnephew wrote: > Their design pattern for this is that it should raise NotImplementedError if > these classes are not overridden. These should be changed back to remain > consistent with the rest of telemetry. > > Unless Ned feels otherwise and is ok with this change; then ignore this opinion. > Yeah, I prefer these methods to be "abstract" so that if people make mistake when overriding these, we know right away.
On 2015/11/10 at 19:25:42, nednguyen wrote: > https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... > File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): > > https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... > tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:33: assert not self._monitoring, "Already monitoring power." > On 2015/11/10 19:17:52, rnephew wrote: > > Their design pattern for this is that it should raise NotImplementedError if > > these classes are not overridden. These should be changed back to remain > > consistent with the rest of telemetry. > > > > Unless Ned feels otherwise and is ok with this change; then ignore this opinion. > > > > Yeah, I prefer these methods to be "abstract" so that if people make mistake when overriding these, we know right away. Hello, I wanted to clarify why I made the change to power_monitor/__init__.py. The idea here is that all of the different power monitor classes have the same restriction where calls to StartMonitoringPower() must be paired with calls to StopMonitoringPower(). In the existing implementation, most of the classes check this, but they do it in different ways, and often ways which are not clear (e.g. android_dumpsys_power_monitor effectively checks this condition by whether self._browser is None). The purpose of the change was to ensure that this check is done by all classes, and in a consistent way. Since this functionality is the same in all classes, this is why I put it in the superclass. This is also why, for instance, I made the change you had mentioned above: assert not self._active_monitors, 'Must call StopMonitoringPower().' Do not get rid of this. Its there to make sure things are getting called properly. The idea is that I thought that this line was not necessary because now the functionality that is being tested (whether you already called StartMonitoringPower()) is now being done in the consistent way, in the superclass. I am interested in whether you think the general idea of this change is good, and if so what is the best alternative way to do this change. Perhaps, in the superclass, we could keep StartMonitoringPower() and StopMonitoringPower() as "raise NotImplementedError()" like you expected, but add the check functionality in new functions that are called at this level (maybe name them something like "StartCheck" and "StopCheck") and have these be called by the StartMonitoringPower() and StopMonitoringPower() functions in the subclass (so no need to call super(...))? Also, you had mentioned another change, on power_monitor_utils.py: Instead of making this its own thing, maybe make a class that the android power monitors inherit from that have these functions. Assuming this comment is referring to the entire power_monitor_utils file, it's unclear to me what the advantage of this would be. The android power monitors already inherit from the base PowerMonitor class, so this would require multiple inheritance (which I don't think we use), putting this functionality in the base PowerMonitor class, or adding another level to the inheritance hierarchy. I would think of these as more "utility functions" that lots of classes could use - I don't see this as functionality that is tied to the particular class. What do you see as the advantage of putting this functionality in the clas
Response inline. > Hello, > > I wanted to clarify why I made the change to power_monitor/__init__.py. > > The idea here is that all of the different power monitor classes have the same > restriction where calls to StartMonitoringPower() must be paired with calls to > StopMonitoringPower(). In the existing implementation, most of the classes check > this, but they do it in different ways, and often ways which are not clear (e.g. > android_dumpsys_power_monitor effectively checks this condition by whether > self._browser is None). The purpose of the change was to ensure that this check > is done by all classes, and in a consistent way. Since this functionality is the > same in all classes, this is why I put it in the superclass. > > This is also why, for instance, I made the change you had mentioned above: > > assert not self._active_monitors, 'Must call StopMonitoringPower().' > Do not get rid of this. Its there to make sure things are getting called > properly. > > The idea is that I thought that this line was not necessary because now the > functionality that is being tested (whether you already called > StartMonitoringPower()) is now being done in the consistent way, in the > superclass. > > I am interested in whether you think the general idea of this change is good, > and if so what is the best alternative way to do this change. Perhaps, in the > superclass, we could keep StartMonitoringPower() and StopMonitoringPower() as > "raise NotImplementedError()" like you expected, but add the check functionality > in new functions that are called at this level (maybe name them something like > "StartCheck" and "StopCheck") and have these be called by the > StartMonitoringPower() and StopMonitoringPower() functions in the subclass (so > no need to call super(...))? I am ok with this change, if Ned is ok with it. > Also, you had mentioned another change, on power_monitor_utils.py: > > Instead of making this its own thing, maybe make a class that the > android power monitors inherit from that have these functions. > > Assuming this comment is referring to the entire power_monitor_utils file, it's > unclear to me what the advantage of this would be. The android power monitors > already inherit from the base PowerMonitor class, so this would require multiple > inheritance (which I don't think we use), putting this functionality in the base > PowerMonitor class, or adding another level to the inheritance hierarchy. I > would think of these as more "utility functions" that lots of classes could use > - I don't see this as functionality that is tied to the particular class. What > do you see as the advantage of putting this functionality in the clas In the days before mutiple power monitor support , all android power monitors inherited from sysfs_power_monitor which inherited from __init__ so there is history of the power monitors having multiple inheritances. As for them being general 'utility functions' there are small subtle things about them that are android specific. Such as the default voltage being 4. This is not necessarily true for other platforms. There are also some parts that are obviously android only, such as the set charging stuff. By making it part of the class we are clearly stating that these are for android only; while having it in a util function leaves it up to the user to know what platform it is ok to use it on.
https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py (left): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:44: self._battery.TieredSetCharging(False) On 2015/11/10 at 19:17:52, rnephew wrote: > I am confused at how this is here, I got rid of TieredSetCharging and when I look locally it isn't there. You might need to sync and rebase. I had forgotten to do the rebase before submitting this CL. I will do the rebase before updating the CL. https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:9: from telemetry.internal.platform.power_monitor import power_monitor_utils as pmu On 2015/11/10 at 19:17:52, rnephew wrote: > We typically do not do this with imports. What do you mean by "do not do this with imports?" Is the issue the "import ... as"? Or is the issue that we are importing the module power_monitor and then importing something else inside power_monitor separately?
rnephew@chromium.org changed reviewers: + rnephew@chromium.org
https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:9: from telemetry.internal.platform.power_monitor import power_monitor_utils as pmu On 2015/11/10 23:09:19, alexandermont wrote: > On 2015/11/10 at 19:17:52, rnephew wrote: > > We typically do not do this with imports. > > What do you mean by "do not do this with imports?" Is the issue the "import ... > as"? Or is the issue that we are importing the module power_monitor and then > importing something else inside power_monitor separately? importing using 'as'.
> In the days before mutiple power monitor support , all android power monitors inherited from sysfs_power_monitor which inherited from __init__ so there is history of the power monitors having multiple inheritances. As for them being general 'utility functions' there are small subtle things about them that are android specific. Such as the default voltage being 4. This is not necessarily true for other platforms. There are also some parts that are obviously android only, such as the set charging stuff. By making it part of the class we are clearly stating that these are for android only; while having it in a util function leaves it up to the user to know what platform it is ok to use it on. I think we might be talking about different things when we are talking about "multiple inheritance". I was thinking about "multiple inheritance" as meaning that the same class inherits from multiple superclasses (i.e. A inherits from B, and also A inherits from C). Here it looks like you are talking about just a multiple level inheritance hierarchy (i.e. A inherits from B, and also B inherits from C), i.e. android_dumpsys_power_monitor inherits from sysfs_power_monitor which inherits from the PowerMonitor class in __init__. So I see what you mean now, that what we want in the new version is something along the lines of android_dumpsys_power_monitor inherits from some new class, android_power_monitor, that itself inherits from the version in __init__.
On 2015/11/10 23:23:56, alexandermont wrote: > > In the days before mutiple power monitor support , all android power monitors > inherited from sysfs_power_monitor which inherited from __init__ so there is > history of the power monitors having multiple inheritances. As for them being > general 'utility functions' there are small subtle things about them that are > android specific. Such as the default voltage being 4. This is not necessarily > true for other platforms. There are also some parts that are obviously android > only, such as the set charging stuff. By making it part of the class we are > clearly stating that these are for android only; while having it in a util > function leaves it up to the user to know what platform it is ok to use it on. > > I think we might be talking about different things when we are talking about > "multiple inheritance". > > I was thinking about "multiple inheritance" as meaning that the same class > inherits from multiple superclasses (i.e. A inherits from B, and also A inherits > from C). Here it looks like you are talking about just a multiple level > inheritance hierarchy (i.e. A inherits from B, and also B inherits from C), i.e. > android_dumpsys_power_monitor inherits from sysfs_power_monitor which inherits > from the PowerMonitor class in __init__. So I see what you mean now, that what > we want in the new version is something along the lines of > android_dumpsys_power_monitor inherits from some new class, > android_power_monitor, that itself inherits from the version in __init__. Yep, thats what I meant. Maybe name it something like android_base_power_monitor so that its obvious from the name that it isn't a power monitor itself.
https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:29: def CheckStart(self): This should probably be an internal method since nothing outside the class will call it. We do this by naming the function with an _: def _CheckStart(self): I'm not sure on the name though, since it does more than just checking. Maybe it should be _InitializeMonitoring? But that name might be too similar to StartMonitoringPower. Naming, the hardest part of programming. I'm ok with leaving it the same if a better name can't be thought of, because I cant come up with one myself. https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:51: class AndroidPowerMonitor(PowerMonitor): Should probably be in its own file, and name it something like AndroidPowerMonitorBase so its obvious that it itself is not a complete power monitor, but a base to build on top of. https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:42: self.ChargingOff(self._battery) This could also be made into a private method: self._ChargingOff, same for ChargingOn, LogPowerAnomalies, and ParseVoltage https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:48: logging.warning('String returned from device.ReadFile(_TEMPERATURE_FILE)' nit: add a space, this will read as: String returned from device.ReadFile(_TEMPERATURE_FILE)in invalid format. https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py:42: logging.warning('Device not on battery power during power monitoring.' nit: add space https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py:57: logging.warning('Device not on battery power during power monitoring.' nit: add space https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py:46: class MonsoonPowerMonitor(power_monitor.PowerMonitor): This is also an android power monitor, so it should also inherit from the AndroidPowerMonitorBase class so that it is more obvious that this is an android power monitor, even if it doesn't use the functions added to that class. https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py:138: self._backend.CloseMsrServer() Nice catch. I think this should be moved up to _CheckMSRs and only close it if it is going to return false, otherwise you ares stopping and starting the MSR server multiple times. https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:31: super(PowerMonitorController, self).StartMonitoringPower(browser) This will raise a NotImplementedException as-is. Also, just a warning I will have a CL shortly that will be editing this file to solve crbug.com/553601 so one of us will have to rebase, depending on who's lands first. https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:66: super(PowerMonitorController, self).StopMonitoringPower() Same exception problem as StartMonitoringPower
Looking good. Almost there, but I'm not an owner in telemetry so the final decision is up to Ned. Any other telemetry people you want to look at the code Ned? https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:11: Class level constants up here. _PACKAGE = 'com.google.android.chrome' _TYPICAL_POWER_DATA = { 'system_total': 2000.0, 'per_package': { package: {'data': [23.9], 'uid': '12345'} } } https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:39: package = 'com.google.android.apps.chrome' Since multiple tests use this same exact data, it can be moved to a class level constant so there is less repeated code. package = self._PACKAGE power_data = self._TYPICAL_POWER_DATA Or just replace where they are used with the constants. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:58: pm.StopMonitoringPower() This should be in a separate test, since its not the normal cycle thus not really part of testMonitorCycle, make a separate test called 'testDoubleStop'. For that test, you do not need to do the asserts on the results dict. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py:31: pm.StopMonitoringPower() Same as android_dumpsys. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:41: def StartMonitoringPower(self, browser): Not needed, it'll catch it from the parent class. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:48: def StopMonitoringPower(self): Same. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller_unittest.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller_unittest.py:42: super(P3, self).StartMonitoringPower(browser) These should be chagned back, since P1/2/3 inherit from power_monitor.PowerMonitor they will raise NotImplementedError
lg2me overall with some style nits. Did you ignore the warning on "git cl upload?". FYI: the patch won't commit with PRESBUMIT warning, so you may want to fix them sooner than later. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:5: import logging nits: unused? Though telemetry PRESUBMIT should warn you with unused import when you try to upload the patch. Lemme know if that's not the case. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:7: nits: add a extra blank line. Top level blocks in a file a separated with 2 blank lines https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (left): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:45: for path_element in temperature_path[:-1]: Thanks for simplifying this mess! Though do we have unittest coverage on this method to make sure that the return values are the same? https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:36: ave_temp = self._GetBoardTemperatureCelsius() s/ave_temp/avg_temp https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:41: 'platform_info': {'average_temperature_c':ave_temp}} nit: add an extra space, i.e: "'average_temperature_c': ave_temp". Telemetry presubmit should also warn you on this. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/powermetrics_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/powermetrics_power_monitor.py:19: class PowerMetricsPowerMonitor(power_monitor.PowerMonitor): "PowerMetricsPowerMonitor"? Seems like this is one is used by mac. Look like a poor naming choice for me. Can you add a TODO to rename this?
On 2015/11/12 at 17:22:13, nednguyen wrote: > lg2me overall with some style nits. Did you ignore the warning on "git cl upload?". FYI: the patch won't commit with PRESBUMIT warning, so you may want to fix them sooner than later. > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:5: import logging > nits: unused? Though telemetry PRESUBMIT should warn you with unused import when you try to upload the patch. Lemme know if that's not the case. > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > File tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py (right): > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:7: > nits: add a extra blank line. Top level blocks in a file a separated with 2 blank lines > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (left): > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:45: for path_element in temperature_path[:-1]: > Thanks for simplifying this mess! > > Though do we have unittest coverage on this method to make sure that the return values are the same? > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (right): > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:36: ave_temp = self._GetBoardTemperatureCelsius() > s/ave_temp/avg_temp > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:41: 'platform_info': {'average_temperature_c':ave_temp}} > nit: add an extra space, i.e: "'average_temperature_c': ave_temp". Telemetry presubmit should also warn you on this. > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > File tools/telemetry/telemetry/internal/platform/power_monitor/powermetrics_power_monitor.py (right): > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/powermetrics_power_monitor.py:19: class PowerMetricsPowerMonitor(power_monitor.PowerMonitor): > "PowerMetricsPowerMonitor"? Seems like this is one is used by mac. Look like a poor naming choice for me. Can you add a TODO to rename this? I have uploaded new version that addresses these comments. In all of the versions I have uploaded so far, the presubmit checks have passed.
Look good with some minor nits; but Ned has the final say https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:5: import logging nit:Space between these two imports. Below is a link to a random python file for an example of how we format imports: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:24: Nit: 2 spaces before class declaration. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:76: nit: 2 spaces before any top level declaration. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py:10: nit: 2 spaces https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py:31: pm.StopMonitoringPower() Since the other test is now testing this case, the double stop should be removed from this test case. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py:49: nit: 2 spaces https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:6: from telemetry.internal.platform import power_monitor nit: space between different styles of imports https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:42: def StartMonitoringPower(self, browser): Nit: The StartMonitoringPower and StopMonitoringPower definitions still need to be removed from these, the parent class already throws NotImplementedError exceptions. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:35: def StopMonitoringPower(self): nednguyen 2015/11/12 17:22:13 Thanks for simplifying this mess! Though do we have unittest coverage on this method to make sure that the return values are the same? ^ Was Neds question about unit test coverage covered? I didn't see anywhere where it was. Looking at android_temperature_monitor_unittests.py I would say the answer is yes though. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py:7: import logging nit: alphabetical ordering https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py:9: import logging nit: alphabetical ordering. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py:12: from telemetry.internal.platform.profiler import monsoon nit: alphabetical ordering. telemetry.internal.platform.power_monitor telemetry.internal.platform.profiler https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py:45: nit: 2 spaces between top level items.
Comments on remaining code items https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py:138: self._backend.CloseMsrServer() On 2015/11/11 at 22:46:27, rnephew1 wrote: > Nice catch. > > I think this should be moved up to _CheckMSRs and only close it if it is going to return false, otherwise you ares stopping and starting the MSR server multiple times. This wasn't mine. I didn't change anything in this function. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:5: import logging On 2015/11/12 at 17:22:12, nednguyen wrote: > nits: unused? Though telemetry PRESUBMIT should warn you with unused import when you try to upload the patch. Lemme know if that's not the case. The presubmit checks did pass. I wonder if the unused import check is disabled in __init__ files? My understanding is that the way __init__ files work, sometimes you want to have imports in them in order to run code that is "run on import," even if you don't plan to use that import in the __init__ file itself. But I will remove this; I don't think it is necessary. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:39: package = 'com.google.android.apps.chrome' On 2015/11/12 at 01:36:45, rnephew1 wrote: > Since multiple tests use this same exact data, it can be moved to a class level constant so there is less repeated code. > > package = self._PACKAGE > power_data = self._TYPICAL_POWER_DATA > > Or just replace where they are used with the constants. done https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:58: pm.StopMonitoringPower() On 2015/11/12 at 01:36:45, rnephew1 wrote: > This should be in a separate test, since its not the normal cycle thus not really part of testMonitorCycle, make a separate test called 'testDoubleStop'. For that test, you do not need to do the asserts on the results dict. done https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:7: On 2015/11/12 at 17:22:12, nednguyen wrote: > nits: add a extra blank line. Top level blocks in a file a separated with 2 blank lines done https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:41: def StartMonitoringPower(self, browser): On 2015/11/12 at 01:36:46, rnephew1 wrote: > Not needed, it'll catch it from the parent class. If I don't put these in, presubmit blocks it, saying that I have an abstract method in the superclass that isn't overridden in the subclass. Do you have to somehow tell it that AndroidPowerMonitorBase is also an abstract class? Maybe it has something to do with the fact that AndroidPowerMonitorBase is in its own file now, not in __init__.py. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:48: def StopMonitoringPower(self): On 2015/11/12 at 01:36:46, rnephew1 wrote: > Same. See above. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (left): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:45: for path_element in temperature_path[:-1]: On 2015/11/12 at 17:22:13, nednguyen wrote: > Thanks for simplifying this mess! > > Though do we have unittest coverage on this method to make sure that the return values are the same? We do, in android_temperature_monitor_unittest.py. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:36: ave_temp = self._GetBoardTemperatureCelsius() On 2015/11/12 at 17:22:13, nednguyen wrote: > s/ave_temp/avg_temp done https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:41: 'platform_info': {'average_temperature_c':ave_temp}} On 2015/11/12 at 17:22:13, nednguyen wrote: > nit: add an extra space, i.e: "'average_temperature_c': ave_temp". Telemetry presubmit should also warn you on this. Done. The presubmit didn't warn about this. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller_unittest.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller_unittest.py:42: super(P3, self).StartMonitoringPower(browser) On 2015/11/12 at 01:36:46, rnephew1 wrote: > These should be chagned back, since P1/2/3 inherit from power_monitor.PowerMonitor they will raise NotImplementedError Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:5: import logging On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit:Space between these two imports. > > Below is a link to a random python file for an example of how we format imports: > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:24: On 2015/11/12 at 21:20:02, rnephew1 wrote: > Nit: 2 spaces before class declaration. Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:76: On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit: 2 spaces before any top level declaration. Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py:10: On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit: 2 spaces Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py:31: pm.StopMonitoringPower() On 2015/11/12 at 21:20:02, rnephew1 wrote: > Since the other test is now testing this case, the double stop should be removed from this test case. Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor_unittest.py:49: On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit: 2 spaces Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:6: from telemetry.internal.platform import power_monitor On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit: space between different styles of imports Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:42: def StartMonitoringPower(self, browser): On 2015/11/12 at 21:20:02, rnephew1 wrote: > Nit: The StartMonitoringPower and StopMonitoringPower definitions still need to be removed from these, the parent class already throws NotImplementedError exceptions. Done https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_temperature_monitor.py:35: def StopMonitoringPower(self): On 2015/11/12 at 21:20:02, rnephew1 wrote: > nednguyen 2015/11/12 17:22:13 > Thanks for simplifying this mess! > > Though do we have unittest coverage on this method to make sure that the return > values are the same? > > > ^ Was Neds question about unit test coverage covered? I didn't see anywhere where it was. Looking at android_temperature_monitor_unittests.py I would say the answer is yes though. It was. (I just forgot to commit that answer when I uploaded the new version.) https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/cros_power_monitor.py:7: import logging On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit: alphabetical ordering Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py:9: import logging On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit: alphabetical ordering. Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py:12: from telemetry.internal.platform.profiler import monsoon On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit: alphabetical ordering. > telemetry.internal.platform.power_monitor > telemetry.internal.platform.profiler Done. https://codereview.chromium.org/1432093002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/monsoon_power_monitor.py:45: On 2015/11/12 at 21:20:02, rnephew1 wrote: > nit: 2 spaces between top level items. Done.
lgtm but pease also wait for Randy
One more nit, then LGTM. https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py:138: self._backend.CloseMsrServer() On 2015/11/12 23:49:46, alexandermont wrote: > On 2015/11/11 at 22:46:27, rnephew1 wrote: > > Nice catch. > > > > I think this should be moved up to _CheckMSRs and only close it if it is going > to return false, otherwise you ares stopping and starting the MSR server > multiple times. > > This wasn't mine. I didn't change anything in this function. Ah, must have been from the rebasing. https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py (right): https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:5: import logging On 2015/11/12 23:49:46, alexandermont wrote: > On 2015/11/12 at 17:22:12, nednguyen wrote: > > nits: unused? Though telemetry PRESUBMIT should warn you with unused import > when you try to upload the patch. Lemme know if that's not the case. > > The presubmit checks did pass. I wonder if the unused import check is disabled > in __init__ files? My understanding is that the way __init__ files work, > sometimes you want to have imports in them in order to run code that is "run on > import," even if you don't plan to use that import in the __init__ file itself. > > But I will remove this; I don't think it is necessary. The presubmit wasn't catching out of order ones either, must be something wrong. https://codereview.chromium.org/1432093002/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor.py:6: nit: 2 spaces
On 2015/11/13 00:02:58, rnephew1 wrote: > One more nit, then LGTM. > > https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... > File > tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py > (right): > > https://codereview.chromium.org/1432093002/diff/20001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py:138: > self._backend.CloseMsrServer() > On 2015/11/12 23:49:46, alexandermont wrote: > > On 2015/11/11 at 22:46:27, rnephew1 wrote: > > > Nice catch. > > > > > > I think this should be moved up to _CheckMSRs and only close it if it is > going > > to return false, otherwise you ares stopping and starting the MSR server > > multiple times. > > > > This wasn't mine. I didn't change anything in this function. > > Ah, must have been from the rebasing. > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > File tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py > (right): > > https://codereview.chromium.org/1432093002/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/__init__.py:5: import > logging > On 2015/11/12 23:49:46, alexandermont wrote: > > On 2015/11/12 at 17:22:12, nednguyen wrote: > > > nits: unused? Though telemetry PRESUBMIT should warn you with unused import > > when you try to upload the patch. Lemme know if that's not the case. > > > > The presubmit checks did pass. I wonder if the unused import check is disabled > > in __init__ files? My understanding is that the way __init__ files work, > > sometimes you want to have imports in them in order to run code that is "run > on > > import," even if you don't plan to use that import in the __init__ file > itself. > > > > But I will remove this; I don't think it is necessary. > > The presubmit wasn't catching out of order ones either, must be something wrong. > > https://codereview.chromium.org/1432093002/diff/80001/tools/telemetry/telemet... > File > tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor.py > (right): > > https://codereview.chromium.org/1432093002/diff/80001/tools/telemetry/telemet... > tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor.py:6: > > nit: 2 spaces Just a warning, you will have to rebase because of my changes to power_monitor_controller in https://codereview.chromium.org/1440013002/ It contains a temporary work around for a problem in https://code.google.com/p/chromium/issues/detail?id=553601. Just put the call to self._CheckStart to after my new code.
https://codereview.chromium.org/1432093002/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor.py (right): https://codereview.chromium.org/1432093002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/platform/power_monitor/android_fuelgauge_power_monitor.py:6: On 2015/11/13 at 00:02:58, rnephew1 wrote: > nit: 2 spaces Done.
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, rnephew@chromium.org Link to the patchset: https://codereview.chromium.org/1432093002/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432093002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432093002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/adce66abd74fb2d6e22da36eee85b7e324f962ba Cr-Commit-Position: refs/heads/master@{#359640}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1452753002/ by nednguyen@google.com. The reason for reverting is: Speculative revert since this may break page_cycler.typical_25 BUG=556653. |
