Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1093483002:
Changed when the UMA metric Ash.NumberOfVisibleWindowsInPrimaryDisplay is recorded. (Closed)
DescriptionChanged when the UMA metric Ash.NumberOfVisibleWindowsInPrimaryDisplay is recorded.
Ash.NumberOfVisibleWindowsInPrimaryDisplay was incorrectly being recorded at the
login and lock screen.
TEST=UserMetricsRecorderTest.VerifyIsUserInActiveDesktopEnvironmentValues
TEST=UserMetricsRecorderTest.VerifyStatsRecordedWhenUserNotInActiveDesktopEnvironment
TEST=UserMetricsRecorderTest.VerifyStatsRecordedWhenUserInActiveDesktopEnvironment
TEST=UserMetricsRecorderTest.VerifyStatsRecordedByRecordPeriodicMetrics
BUG=477538
Committed: https://crrev.com/d89b76b49964e9b764fc93b2e8473e5120b74bf2
Cr-Commit-Position: refs/heads/master@{#327088}
Patch Set 1 #Patch Set 2 : Added unittests. #
Total comments: 21
Patch Set 3 : Addressed comments from patch set 2. #
Total comments: 24
Patch Set 4 : Addressed pkotwicz@ comments from patch set 3. #
Total comments: 16
Patch Set 5 : Addressed comments from patch set 4. #
Messages
Total messages: 22 (5 generated)
bruthig@chromium.org changed reviewers: + sadrul@chromium.org, tdanderson@chromium.org
sadrul@, tdanderson@, I thought you might be interested in this review for discussions sake on some pain points from a testability perspective. I set out trying to test the values recorded in the Ash.NumberOfVisibleWindowsInPrimaryDisplay histogram but had to limit myself due to a lot of missing 'test framework'. I've added some TODO's in the code to track some missing pieces but my biggest frustration is that, AFAIK, it's very difficult to create and manipulate Window's in the different containers. Any examples I found were one-off custom methods in other *_unittest.cc files. Ideally these helper methods would be promoted to a common spot like AshTestBase or AshTestHelper but I don't currently have the expertise to push that task forward. Anyway, can you please have a look before I send it off to the owners for review.
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
Some drive by comments https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:536: if (UserIsActive()) { Should we also ignore kiosk mode? In kiosk mode, a user cannot change GetNumVisibleWindowsInPrimaryDisplay(). (Unless it is possible to open popup windows in kiosk mode?) https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:29: class TestUserMetricsRecorder : public UserMetricsRecorder { Extending the real class is a common pattern in Chrome for tests. However, I don't think that it is necessary in this case. If you make UserMetricsRecorder a friend of UserMetricsRecorderTest, I think you can access all the privates that you need. I think that it is preferable to get rid of the constructor with the boolean parameter and call UserMetricsRecorder.timer_.Stop() after the UserMetricsRecorder has been constructed. (I suspect that you will get different opinions about this if you ask different devs). I think that we like to avoid modifying the classes that we are testing for the sake of testing if at all possible. An argument can be made that you want TestUserMetricsRecorder to be generic and used by other test classes. Chrome has a philosophy "Do not make something any more generic than currently needed" and I agree with this philosophy. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:151: // ShelfLayoutManager does not exist. When does the ShelfLayoutManager not exist?
Thanks for the input pkotwicz@. You've started some of the discussions I was hoping to generate from this review :) Let's perhaps give the other reviewers a chance to digest and comment on the threads before we turn them in to a 1:1 discussion. We can certainly chat offline in the meantime though. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:536: if (UserIsActive()) { On 2015/04/17 13:45:38, pkotwicz wrote: > Should we also ignore kiosk mode? In kiosk mode, a user cannot change > GetNumVisibleWindowsInPrimaryDisplay(). (Unless it is possible to open popup > windows in kiosk mode?) Good point, I agree it should probably not be logged during kiosk mode either. tdanderson@ do you have any input here? https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:29: class TestUserMetricsRecorder : public UserMetricsRecorder { On 2015/04/17 13:45:38, pkotwicz wrote: > Extending the real class is a common pattern in Chrome for tests. However, I > don't think that it is necessary in this case. Out of curiosity when would it be necessary? > > If you make UserMetricsRecorder a friend of UserMetricsRecorderTest, I think you > can access all the privates that you need. > To be honest I'm not a big fan of either approach. The reason is that they are not scalable. In either case, if you have another test fixture that want's to use the UserMetricsRecorder it will require another friend to be declared or another custom test specific subclass. The only reason I chose this approach is because I didn't want to challenge the whole world all at once ;) I am happy for the time being to use whichever the owner's prefer. I really don't want to have to use the FRIEND_TEST_ALL_PREFIXES macro for each and every test though. From a testability perspective the better approach would be to split the UserMetricsRecorder class in to two classes. The first class would be responsible for "how the metrics are recorded". More concretely it would contain the RecordPeriodicMetrics and the RecordUserMetricsAction methods which would have to both be public by necessity. The second class would handle "when the periodic metrics are recorded". It would be responsible for owning and managing the timer, connecting the timer to the RecordPeriodicMetrics method as well as checking the "should I log conditions" like UserIsActive. > I think that it is preferable to get rid of the constructor with the boolean > parameter and call UserMetricsRecorder.timer_.Stop() after the > UserMetricsRecorder has been constructed. (I suspect that you will get different > opinions about this if you ask different devs). I think that we like to avoid > modifying the classes that we are testing for the sake of testing if at all > possible. > An argument can be made that you want TestUserMetricsRecorder to be generic and > used by other test classes. Chrome has a philosophy "Do not make something any > more generic than currently needed" and I agree with this philosophy. > It is exactly this philosophy that I am challenging and I am trying to show the benefits of the alternative :) Testing is considered a second class citizen in the world of chrome and currently is not 'important' enough to consider it as a use case or client of the system under test and this is the same push back I have received on multiple occasions. I am still searching for the underlying reason of this approach so that we can evaluate the pros and cons of it. By designing software for testability you inevitably give the test target two (if not more) use cases and you are right, this will force it to be slightly more generic. This has the beneficial "side" effects of making that test target better defined and more portable for reuse, not to mention it is well tested. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:151: // ShelfLayoutManager does not exist. On 2015/04/17 13:45:38, pkotwicz wrote: > When does the ShelfLayoutManager not exist? I'm not sure, I'm not familiar with this and only added this test because of the conditional logic in the test target.
https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:29: class TestUserMetricsRecorder : public UserMetricsRecorder { Extending the real class is necessary when the test wants to change the behavior of one of the functions of the real class. For instance, this is the case of TestFaviconHandler w.r.t to the real class FaviconHandler. I am going to completely rewrite favicon_handler_unittest.cc so lets not discuss whether favicon_handler_unittest.cc requires this. I think we want to avoid using the FRIEND_TEST_ALL_PREFIXES macro in new tests and just make the test harness a friend of the class being tested. For instance, ResourceBundleTest is a friend of ResourceBundle. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:151: // ShelfLayoutManager does not exist. I think that the test would 10x more useful if it had a comment as to when the ShelfLayouManager does not exist. We may find out that we can just nuke the ShelfLayoutManager check
https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:536: if (UserIsActive()) { On 2015/04/17 14:41:23, bruthig wrote: > On 2015/04/17 13:45:38, pkotwicz wrote: > > Should we also ignore kiosk mode? In kiosk mode, a user cannot change > > GetNumVisibleWindowsInPrimaryDisplay(). (Unless it is possible to open popup > > windows in kiosk mode?) > > Good point, I agree it should probably not be logged during kiosk mode either. > > tdanderson@ do you have any input here? Agreed, let's ignore kiosk mode. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:543: UMA_HISTOGRAM_ENUMERATION("Ash.ActiveWindowShowTypeOverTime", IMO, we should only be collecting periodic metrics related to the UI if the user is active (so that applies to all of the histograms here). Double-check with the histogram owners, though, before making any such change. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:553: user::LOGGED_IN_LOCKED; Is the first clause necessary here?
https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:546: } This function is ... interesting. What are the values of these histograms? I think it would be more useful to know how long the user spent with left/bottom etc. shelf alignment before switching to a different orientation, how long the user has X number of windows open etc. before opening/closing a window, how long a particular type of window is active at a time etc., rather than collecting this info every 30 minutes. The owner for these seem to be kuscher@ Can you guys talk to him to see if that would make more sense? https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:29: class TestUserMetricsRecorder : public UserMetricsRecorder { > > If you make UserMetricsRecorder a friend of UserMetricsRecorderTest, I think > you > > can access all the privates that you need. > > > > To be honest I'm not a big fan of either approach. The reason is that they are > not scalable. In either case, if you have another test fixture that want's to > use the UserMetricsRecorder it will require another friend to be declared or > another custom test specific subclass. The only reason I chose this approach is > because I didn't want to challenge the whole world all at once ;) I am happy for > the time being to use whichever the owner's prefer. I really don't want to have > to use the FRIEND_TEST_ALL_PREFIXES macro for each and every test though. In a number of places, we friend a TestApi class to expose the internal bits for tests. The TestApi classes are reusable, so you don't need to either keep adding new friends, or use FRIEND_TEST_ALL_PREFIXES, for new tests. (look for TestApi for various examples) > > From a testability perspective the better approach would be to split the > UserMetricsRecorder class in to two classes. The first class would be > responsible for "how the metrics are recorded". More concretely it would > contain the RecordPeriodicMetrics and the RecordUserMetricsAction methods which > would have to both be public by necessity. The second class would handle "when > the periodic metrics are recorded". It would be responsible for owning and > managing the timer, connecting the timer to the RecordPeriodicMetrics method as > well as checking the "should I log conditions" like UserIsActive. > > > I think that it is preferable to get rid of the constructor with the boolean > > parameter and call UserMetricsRecorder.timer_.Stop() after the > > UserMetricsRecorder has been constructed. (I suspect that you will get > different > > opinions about this if you ask different devs). I think that we like to avoid > > modifying the classes that we are testing for the sake of testing if at all > > possible. I do not think this is right. We do want to make sure the production code is easily testable, and make necessary changes to allow that, as long as it doesn't hurt the code (maintainability/quality/performance etc. -wise). I suppose we haven't necessarily always done a good job of it, or there isn't a universal approach that everyone agrees with, so this is something that could be improved. > > An argument can be made that you want TestUserMetricsRecorder to be generic > and > > used by other test classes. Chrome has a philosophy "Do not make something any > > more generic than currently needed" and I agree with this philosophy. > > > > It is exactly this philosophy that I am challenging and I am trying to show the > benefits of the alternative :) Testing is considered a second class citizen in > the world of chrome and currently is not 'important' enough to consider it as aI thi > use case or client of the system under test and this is the same push back I > have received on multiple occasions. I am still searching for the underlying > reason of this approach so that we can evaluate the pros and cons of it. > > By designing software for testability you inevitably give the test target two > (if not more) use cases and you are right, this will force it to be slightly > more generic. This has the beneficial "side" effects of making that test target > better defined and more portable for reuse, not to mention it is well tested.
bruthig@chromium.org changed reviewers: + oshima@chromium.org
I've re-worked the CL as per comments from sadrul@, tdanderson@, and pkotwicz@. Adding owners for review. oshima@chromium.org: Can you please have a look? https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:536: if (UserIsActive()) { On 2015/04/17 17:59:57, tdanderson wrote: > On 2015/04/17 14:41:23, bruthig wrote: > > On 2015/04/17 13:45:38, pkotwicz wrote: > > > Should we also ignore kiosk mode? In kiosk mode, a user cannot change > > > GetNumVisibleWindowsInPrimaryDisplay(). (Unless it is possible to open popup > > > windows in kiosk mode?) > > > > Good point, I agree it should probably not be logged during kiosk mode either. > > > > > tdanderson@ do you have any input here? > > Agreed, let's ignore kiosk mode. Done. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:543: UMA_HISTOGRAM_ENUMERATION("Ash.ActiveWindowShowTypeOverTime", On 2015/04/17 17:59:57, tdanderson wrote: > IMO, we should only be collecting periodic metrics related to the UI if the user > is active (so that applies to all of the histograms here). Double-check with the > histogram owners, though, before making any such change. I will check with the owner's on these and will make the change in a follow-up CL if necessary. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:546: } On 2015/04/17 22:55:39, sadrul wrote: > This function is ... interesting. What are the values of these histograms? I > think it would be more useful to know how long the user spent with left/bottom > etc. shelf alignment before switching to a different orientation, how long the > user has X number of windows open etc. before opening/closing a window, how long > a particular type of window is active at a time etc., rather than collecting > this info every 30 minutes. The owner for these seem to be kuscher@ Can you guys > talk to him to see if that would make more sense? I've added some TODO's to consider these suggestions for the two histograms that already exist. Unfortunately we don't currently have the time to push these forward. As for the histogram that is being added, tdanderson@ and I discussed and agree that tracking 'the amount of time X number of windows is visible' may be a better thing to do, we felt that the additional work to implement this was not worth the effort. The immediate questions we are trying to answer revolve around whether we need to invest time implementing window management in TouchView. sadrul@, did you have any major concerns as to why tracking it this way would be bad? https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:553: user::LOGGED_IN_LOCKED; On 2015/04/17 17:59:57, tdanderson wrote: > Is the first clause necessary here? Removed. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:29: class TestUserMetricsRecorder : public UserMetricsRecorder { On 2015/04/17 22:55:39, sadrul wrote: > > > If you make UserMetricsRecorder a friend of UserMetricsRecorderTest, I think > > you > > > can access all the privates that you need. > > > > > > > To be honest I'm not a big fan of either approach. The reason is that they > are > > not scalable. In either case, if you have another test fixture that want's to > > use the UserMetricsRecorder it will require another friend to be declared or > > another custom test specific subclass. The only reason I chose this approach > is > > because I didn't want to challenge the whole world all at once ;) I am happy > for > > the time being to use whichever the owner's prefer. I really don't want to > have > > to use the FRIEND_TEST_ALL_PREFIXES macro for each and every test though. > > In a number of places, we friend a TestApi class to expose the internal bits for > tests. The TestApi classes are reusable, so you don't need to either keep adding > new friends, or use FRIEND_TEST_ALL_PREFIXES, for new tests. (look for TestApi > for various examples) > > > > > From a testability perspective the better approach would be to split the > > UserMetricsRecorder class in to two classes. The first class would be > > responsible for "how the metrics are recorded". More concretely it would > > contain the RecordPeriodicMetrics and the RecordUserMetricsAction methods > which > > would have to both be public by necessity. The second class would handle > "when > > the periodic metrics are recorded". It would be responsible for owning and > > managing the timer, connecting the timer to the RecordPeriodicMetrics method > as > > well as checking the "should I log conditions" like UserIsActive. > > > > > I think that it is preferable to get rid of the constructor with the boolean > > > parameter and call UserMetricsRecorder.timer_.Stop() after the > > > UserMetricsRecorder has been constructed. (I suspect that you will get > > different > > > opinions about this if you ask different devs). I think that we like to > avoid > > > modifying the classes that we are testing for the sake of testing if at all > > > possible. > > I do not think this is right. We do want to make sure the production code is > easily testable, and make necessary changes to allow that, as long as it doesn't > hurt the code (maintainability/quality/performance etc. -wise). I suppose we > haven't necessarily always done a good job of it, or there isn't a universal > approach that everyone agrees with, so this is something that could be improved. > > > > An argument can be made that you want TestUserMetricsRecorder to be generic > > and > > > used by other test classes. Chrome has a philosophy "Do not make something > any > > > more generic than currently needed" and I agree with this philosophy. > > > > > > > It is exactly this philosophy that I am challenging and I am trying to show > the > > benefits of the alternative :) Testing is considered a second class citizen > in > > the world of chrome and currently is not 'important' enough to consider it as > aI thi > > use case or client of the system under test and this is the same push back I > > have received on multiple occasions. I am still searching for the underlying > > reason of this approach so that we can evaluate the pros and cons of it. > > > > By designing software for testability you inevitably give the test target two > > (if not more) use cases and you are right, this will force it to be slightly > > more generic. This has the beneficial "side" effects of making that test > target > > better defined and more portable for reuse, not to mention it is well tested. > I've reworked it to use the TestAPI approach and I'm quite happy with it. Thanks for pointing me in that direction sadrul@. https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:151: // ShelfLayoutManager does not exist. On 2015/04/17 17:45:16, pkotwicz wrote: > I think that the test would 10x more useful if it had a comment as to when the > ShelfLayouManager does not exist. We may find out that we can just nuke the > ShelfLayoutManager check To the best of my knowledge a ShelfLayoutManager may not exist in during Shell initialization or destruction. I've update the TODO to consider investigating this. I don't want to include that as a part of this change because I feel it is a higher risk and not actually tied to work encompassed by this review.
LGTM with nits https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:69: // Returns true if Kiosk mode is active. Nit: "Kiosk mode" -> "kiosk mode" https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.h (right): https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:135: // timer. Equivalent to calling UserMetricsRecorder(true). How about: "Creates a UserMetricsRecorder that records metrics periodically. Equivalent to calling ..." https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:154: // Records UMA metrics. Is invoked periodically by the |timer_|. Nit: Is invoked -> Invoked https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:157: // Returns true if the user sessions is active in a multi window environment. How about: "Returns true if the user's session is active and the user is in a multi window environment" https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:108: void UserMetricsRecorderTest::SetUserActiveInMultiWindowEnvironment() { Nit: I would make the method take a boolean. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:159: // conditional is necessary. Remove the test given that we are unsure whether the current behavior is correct. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:170: // user is not active. Nit: Please update the test comment https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:179: // is active. Nit: Please update the test comment https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:188: // logic for when they are recorded. How about: "Verifies recording of stats which are always recorded." https://codereview.chromium.org/1093483002/diff/40001/ash/test/user_metrics_r... File ash/test/user_metrics_recorder_test_api.h (right): https://codereview.chromium.org/1093483002/diff/40001/ash/test/user_metrics_r... ash/test/user_metrics_recorder_test_api.h:28: // Accessor to UserMetricsRecorder::RecordPeriodicMetrics. Nit: Add "()" add the end to make it obvious that this is an accessor to a private function https://codereview.chromium.org/1093483002/diff/40001/ash/test/user_metrics_r... ash/test/user_metrics_recorder_test_api.h:29: void RecordPeriodicMetrics(); Nit: Add "()" add the end to make it obvious that this is an accessor to a private function https://codereview.chromium.org/1093483002/diff/40001/ash/test/user_metrics_r... ash/test/user_metrics_recorder_test_api.h:31: // Accessor to UserMetricsRecorder::IsUserActiveInMultiWindowEnvironment. Nit: Add "()" add the end to make it obvious that this is an accessor to a private function
https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:546: } On 2015/04/21 14:34:05, bruthig wrote: > On 2015/04/17 22:55:39, sadrul wrote: > > This function is ... interesting. What are the values of these histograms? I > > think it would be more useful to know how long the user spent with left/bottom > > etc. shelf alignment before switching to a different orientation, how long the > > user has X number of windows open etc. before opening/closing a window, how > long > > a particular type of window is active at a time etc., rather than collecting > > this info every 30 minutes. The owner for these seem to be kuscher@ Can you > guys > > talk to him to see if that would make more sense? > > I've added some TODO's to consider these suggestions for the two histograms that > already exist. Unfortunately we don't currently have the time to push these > forward. > > As for the histogram that is being added, tdanderson@ and I discussed and agree > that tracking 'the amount of time X number of windows is visible' may be a > better thing to do, we felt that the additional work to implement this was not > worth the effort. The immediate questions we are trying to answer revolve > around whether we need to invest time implementing window management in > TouchView. > > sadrul@, did you have any major concerns as to why tracking it this way would be > bad? The only concern I have is that this data is not useful enough, and may even be misleading, depending on how we interpret the data. For example, if some user always has two open windows, but frequently opens a short-lived (in order of minutes) third/fourth window, then we would potentially completely miss that. It would be better to collect more useful data, or not collect this at all. I will leave it to you to consult with the owner. In theory, it shouldn't need too much effort to change the code to collect the more useful data.
I have addressed pkotwicz@'s comments, pkotwicz@ can you please review them. oshima@ can you please have a look at everything as well? https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:69: // Returns true if Kiosk mode is active. On 2015/04/21 16:15:17, pkotwicz wrote: > Nit: "Kiosk mode" -> "kiosk mode" Done. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.h (right): https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:135: // timer. Equivalent to calling UserMetricsRecorder(true). On 2015/04/21 16:15:17, pkotwicz wrote: > How about: "Creates a UserMetricsRecorder that records metrics periodically. > Equivalent to calling ..." Done. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:154: // Records UMA metrics. Is invoked periodically by the |timer_|. On 2015/04/21 16:15:17, pkotwicz wrote: > Nit: Is invoked -> Invoked Done. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:157: // Returns true if the user sessions is active in a multi window environment. On 2015/04/21 16:15:17, pkotwicz wrote: > How about: "Returns true if the user's session is active and the user is in a > multi window environment" Done. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:108: void UserMetricsRecorderTest::SetUserActiveInMultiWindowEnvironment() { On 2015/04/21 16:15:17, pkotwicz wrote: > Nit: I would make the method take a boolean. Done. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:159: // conditional is necessary. On 2015/04/21 16:15:17, pkotwicz wrote: > Remove the test given that we are unsure whether the current behavior is > correct. Done. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:170: // user is not active. On 2015/04/21 16:15:17, pkotwicz wrote: > Nit: Please update the test comment Done. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:179: // is active. On 2015/04/21 16:15:17, pkotwicz wrote: > Nit: Please update the test comment Done. https://codereview.chromium.org/1093483002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:188: // logic for when they are recorded. On 2015/04/21 16:15:17, pkotwicz wrote: > How about: "Verifies recording of stats which are always recorded." Done. https://codereview.chromium.org/1093483002/diff/40001/ash/test/user_metrics_r... File ash/test/user_metrics_recorder_test_api.h (right): https://codereview.chromium.org/1093483002/diff/40001/ash/test/user_metrics_r... ash/test/user_metrics_recorder_test_api.h:28: // Accessor to UserMetricsRecorder::RecordPeriodicMetrics. On 2015/04/21 16:15:17, pkotwicz wrote: > Nit: Add "()" add the end to make it obvious that this is an accessor to a > private function Done. https://codereview.chromium.org/1093483002/diff/40001/ash/test/user_metrics_r... ash/test/user_metrics_recorder_test_api.h:29: void RecordPeriodicMetrics(); On 2015/04/21 16:15:17, pkotwicz wrote: > Nit: Add "()" add the end to make it obvious that this is an accessor to a > private function Assuming this comment was added by mistake... https://codereview.chromium.org/1093483002/diff/40001/ash/test/user_metrics_r... ash/test/user_metrics_recorder_test_api.h:31: // Accessor to UserMetricsRecorder::IsUserActiveInMultiWindowEnvironment. On 2015/04/21 16:15:17, pkotwicz wrote: > Nit: Add "()" add the end to make it obvious that this is an accessor to a > private function Done.
SLGTM https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:7: #include "ash/shelf/shelf_layout_manager.h" Nit: You can remove this include? https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:40: // Sets the current user session to be (in)active in a multi window Nit: I think that "active or inactive" is clearer than "(in)active" https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:43: I don't think that you use this method
https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/1093483002/diff/20001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:546: } On 2015/04/21 17:24:56, sadrul wrote: > On 2015/04/21 14:34:05, bruthig wrote: > > On 2015/04/17 22:55:39, sadrul wrote: > > > This function is ... interesting. What are the values of these histograms? I > > > think it would be more useful to know how long the user spent with > left/bottom > > > etc. shelf alignment before switching to a different orientation, how long > the > > > user has X number of windows open etc. before opening/closing a window, how > > long > > > a particular type of window is active at a time etc., rather than collecting > > > this info every 30 minutes. The owner for these seem to be kuscher@ Can you > > guys > > > talk to him to see if that would make more sense? > > > > I've added some TODO's to consider these suggestions for the two histograms > that > > already exist. Unfortunately we don't currently have the time to push these > > forward. > > > > As for the histogram that is being added, tdanderson@ and I discussed and > agree > > that tracking 'the amount of time X number of windows is visible' may be a > > better thing to do, we felt that the additional work to implement this was not > > worth the effort. The immediate questions we are trying to answer revolve > > around whether we need to invest time implementing window management in > > TouchView. > > > > sadrul@, did you have any major concerns as to why tracking it this way would > be > > bad? > > The only concern I have is that this data is not useful enough, and may even be > misleading, depending on how we interpret the data. For example, if some user > always has two open windows, but frequently opens a short-lived (in order of > minutes) third/fourth window, then we would potentially completely miss that. > > It would be better to collect more useful data, or not collect this at all. I > will leave it to you to consult with the owner. > > In theory, it shouldn't need too much effort to change the code to collect the > more useful data. You can listen to activation change plus login state change, and record accumulative time instead of doing this periodic. https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.h (right): https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:148: // want the timer to be started. can you be more explicit that this is for test? like "This is used for test .." https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:158: // window environment. "multi window environment" sounds a bit odd. how about IsUserInActiveDesktopEnvironment() ? https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:33: nit: // test::AshTestBase: https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:46: } This seems to be unused? https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:84: new test::UserMetricsRecorderTestAPI(user_metrics_recorder_.get())); what's the benefit of creating recorder, then create test api, if the recorder itself isn't used? If you want to access the recorder, it'd be better to let test api create the recorder internally, then get reference from it when necessary. That can minimize exposure of the test only constructor.
I have addressed comments from patch set 4. oshima@, can you have another look? https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.h (right): https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:148: // want the timer to be started. On 2015/04/25 01:06:58, oshima wrote: > can you be more explicit that this is for test? like "This is used for test .." Done. https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.h:158: // window environment. On 2015/04/25 01:06:58, oshima wrote: > "multi window environment" sounds a bit odd. how about > > IsUserInActiveDesktopEnvironment() ? Done. https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:7: #include "ash/shelf/shelf_layout_manager.h" On 2015/04/25 00:16:53, pkotwicz wrote: > Nit: You can remove this include? Done. https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:33: On 2015/04/25 01:06:58, oshima wrote: > nit: // test::AshTestBase: Done. https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:40: // Sets the current user session to be (in)active in a multi window On 2015/04/25 00:16:53, pkotwicz wrote: > Nit: I think that "active or inactive" is clearer than "(in)active" Done. https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:43: On 2015/04/25 00:16:53, pkotwicz wrote: > I don't think that you use this method Done. https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:46: } On 2015/04/25 01:06:58, oshima wrote: > This seems to be unused? Done. https://codereview.chromium.org/1093483002/diff/60001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder_unittest.cc:84: new test::UserMetricsRecorderTestAPI(user_metrics_recorder_.get())); On 2015/04/25 01:06:58, oshima wrote: > what's the benefit of creating recorder, then create test api, if the recorder > itself isn't used? > > If you want to access the recorder, it'd be better to let test api create the > recorder internally, then get reference from it when necessary. That can > minimize > exposure of the test only constructor. > Done.
lgtm
The CQ bit was checked by bruthig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1093483002/#ps80001 (title: "Addressed comments from patch set 4.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1093483002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d89b76b49964e9b764fc93b2e8473e5120b74bf2 Cr-Commit-Position: refs/heads/master@{#327088} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
