Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
914673003:
Correctly record TouchView metrics on suspend/shutdown (Closed)
|
Created:
5 years, 10 months ago by tdanderson Modified:
5 years, 10 months ago CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCorrectly record TouchView metrics on suspend/shutdown
Modify the logic in MaximizeModeController to correctly
record the amount of time spent in and out of TouchView
in cases where the device is suspended or shut down.
BUG=431829
TEST=none
Committed: https://crrev.com/617d480f3c9cea2a815e2e8da9d4746f76418b2c
Cr-Commit-Position: refs/heads/master@{#316090}
Patch Set 1 #
Total comments: 2
Patch Set 2 : also handle shutdown case #
Total comments: 4
Patch Set 3 : don't assume state transitions on suspend/shutdown #
Total comments: 8
Patch Set 4 : added helper function #Patch Set 5 : compilation #
Messages
Total messages: 21 (4 generated)
tdanderson@chromium.org changed reviewers: + jonross@chromium.org
Hey Jon, mind taking a first look at this?
https://codereview.chromium.org/914673003/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/914673003/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:296: RecordTouchViewStateTransition(); It appears that the same situation can occur on device shutdown. Can you update here as well?
Jon, can you PTAL? https://codereview.chromium.org/914673003/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/914673003/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:296: RecordTouchViewStateTransition(); On 2015/02/10 19:38:07, jonross wrote: > It appears that the same situation can occur on device shutdown. Can you update > here as well? Ah, good catch. Done.
On 2015/02/10 21:09:54, tdanderson wrote: > Jon, can you PTAL? > > https://codereview.chromium.org/914673003/diff/1/ash/wm/maximize_mode/maximiz... > File ash/wm/maximize_mode/maximize_mode_controller.cc (right): > > https://codereview.chromium.org/914673003/diff/1/ash/wm/maximize_mode/maximiz... > ash/wm/maximize_mode/maximize_mode_controller.cc:296: > RecordTouchViewStateTransition(); > On 2015/02/10 19:38:07, jonross wrote: > > It appears that the same situation can occur on device shutdown. Can you > update > > here as well? > > Ah, good catch. Done. LGTM
tdanderson@chromium.org changed reviewers: + flackr@chromium.org
Rob, can you PTAL?
https://codereview.chromium.org/914673003/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/914673003/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:187: RecordTouchViewStateTransition(); This looks like it will report touch view inactive durations for times when the device was in touchview and suspended. The core issue is that RecordTouchViewStateTransition assumes that we just had a state transition, which isn't true on suspend. https://codereview.chromium.org/914673003/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:284: UMA_HISTOGRAM_LONG_TIMES("Ash.TouchView.TouchViewInactive", delta); Note: if suspending in maximize mode will report all of the time up until now as having been TouchViewInactive.
New patchsets have been uploaded after l-g-t-m from jonross@chromium.org
Rob and Jon, can you PTAL at the new approach? The names I chose might be a bit awkward, but I think this is the correct logic. https://codereview.chromium.org/914673003/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/914673003/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:187: RecordTouchViewStateTransition(); On 2015/02/10 21:32:11, flackr wrote: > This looks like it will report touch view inactive durations for times when the > device was in touchview and suspended. The core issue is that > RecordTouchViewStateTransition assumes that we just had a state transition, > which isn't true on suspend. Done. https://codereview.chromium.org/914673003/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:284: UMA_HISTOGRAM_LONG_TIMES("Ash.TouchView.TouchViewInactive", delta); On 2015/02/10 21:32:11, flackr wrote: > Note: if suspending in maximize mode will report all of the time up until now as > having been TouchViewInactive. Done.
https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:192: type = TOUCH_VIEW_INTERVAL_ACTIVE; If it's a bool, can you just use IsMaximizeModeWindowManagerEnabled() rather than if statement? If still an enum, maybe make a function which returns the current state. https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:102: enum TouchViewIntervalType { Why not a bool if we only have active an inactive? I.e. bool touchview_active param on the record usage method?
https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:192: type = TOUCH_VIEW_INTERVAL_ACTIVE; On 2015/02/12 19:59:06, flackr wrote: > If it's a bool, can you just use IsMaximizeModeWindowManagerEnabled() rather > than if statement? If still an enum, maybe make a function which returns the > current state. OK, will do. https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:102: enum TouchViewIntervalType { On 2015/02/12 19:59:06, flackr wrote: > Why not a bool if we only have active an inactive? I.e. bool touchview_active > param on the record usage method? I prefer enums over bools in cases where it would improve readability at call sites. RecordTouchViewUsageInterval(TOUCH_VIEW_INTERVAL_ACTIVE) is more self-documenting than RecordTouchViewUsageInterval(true). But if you prefer that I use a bool instead, I'm ok with that too.
https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:102: enum TouchViewIntervalType { On 2015/02/12 20:30:49, tdanderson wrote: > On 2015/02/12 19:59:06, flackr wrote: > > Why not a bool if we only have active an inactive? I.e. bool touchview_active > > param on the record usage method? > > I prefer enums over bools in cases where it would improve readability at call > sites. RecordTouchViewUsageInterval(TOUCH_VIEW_INTERVAL_ACTIVE) is more > self-documenting than RecordTouchViewUsageInterval(true). But if you prefer that > I use a bool instead, I'm ok with that too. Can also use RecordTouchViewUsageInterval(/* touchview_active */ true) which is fairly prevalent in codebase: https://code.google.com/p/chromium/codesearch#search/&q=%5C/%5C*%5C%20%5B%5E*... of course not strictly enforced. I don't have a strong preference though.
https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:102: enum TouchViewIntervalType { On 2015/02/12 20:44:43, flackr wrote: > On 2015/02/12 20:30:49, tdanderson wrote: > > On 2015/02/12 19:59:06, flackr wrote: > > > Why not a bool if we only have active an inactive? I.e. bool > touchview_active > > > param on the record usage method? > > > > I prefer enums over bools in cases where it would improve readability at call > > sites. RecordTouchViewUsageInterval(TOUCH_VIEW_INTERVAL_ACTIVE) is more > > self-documenting than RecordTouchViewUsageInterval(true). But if you prefer > that > > I use a bool instead, I'm ok with that too. > > Can also use RecordTouchViewUsageInterval(/* touchview_active */ true) which is > fairly prevalent in codebase: > https://code.google.com/p/chromium/codesearch#search/&q=%5C/%5C*%5C%20%5B%5E*... > of course not strictly enforced. I don't have a strong preference though. using a bool would clean up some of the logic in the *.cc file.
Rob, PTAL. https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:192: type = TOUCH_VIEW_INTERVAL_ACTIVE; On 2015/02/12 19:59:06, flackr wrote: > If it's a bool, can you just use IsMaximizeModeWindowManagerEnabled() rather > than if statement? If still an enum, maybe make a function which returns the > current state. Done. https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:102: enum TouchViewIntervalType { On 2015/02/12 20:49:26, jonross wrote: > On 2015/02/12 20:44:43, flackr wrote: > > On 2015/02/12 20:30:49, tdanderson wrote: > > > On 2015/02/12 19:59:06, flackr wrote: > > > > Why not a bool if we only have active an inactive? I.e. bool > > touchview_active > > > > param on the record usage method? > > > > > > I prefer enums over bools in cases where it would improve readability at > call > > > sites. RecordTouchViewUsageInterval(TOUCH_VIEW_INTERVAL_ACTIVE) is more > > > self-documenting than RecordTouchViewUsageInterval(true). But if you prefer > > that > > > I use a bool instead, I'm ok with that too. > > > > Can also use RecordTouchViewUsageInterval(/* touchview_active */ true) which > is > > fairly prevalent in codebase: > > > https://code.google.com/p/chromium/codesearch#search/&q=%5C/%5C*%5C%20%5B%5E*... > > of course not strictly enforced. I don't have a strong preference though. > > using a bool would clean up some of the logic in the *.cc file. Rob and I had a chat offline and he's OK leaving this as an enum.
On 2015/02/12 21:38:22, tdanderson wrote: > Rob, PTAL. > > https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... > File ash/wm/maximize_mode/maximize_mode_controller.cc (right): > > https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... > ash/wm/maximize_mode/maximize_mode_controller.cc:192: type = > TOUCH_VIEW_INTERVAL_ACTIVE; > On 2015/02/12 19:59:06, flackr wrote: > > If it's a bool, can you just use IsMaximizeModeWindowManagerEnabled() rather > > than if statement? If still an enum, maybe make a function which returns the > > current state. > > Done. > > https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... > File ash/wm/maximize_mode/maximize_mode_controller.h (right): > > https://codereview.chromium.org/914673003/diff/40001/ash/wm/maximize_mode/max... > ash/wm/maximize_mode/maximize_mode_controller.h:102: enum TouchViewIntervalType > { > On 2015/02/12 20:49:26, jonross wrote: > > On 2015/02/12 20:44:43, flackr wrote: > > > On 2015/02/12 20:30:49, tdanderson wrote: > > > > On 2015/02/12 19:59:06, flackr wrote: > > > > > Why not a bool if we only have active an inactive? I.e. bool > > > touchview_active > > > > > param on the record usage method? > > > > > > > > I prefer enums over bools in cases where it would improve readability at > > call > > > > sites. RecordTouchViewUsageInterval(TOUCH_VIEW_INTERVAL_ACTIVE) is more > > > > self-documenting than RecordTouchViewUsageInterval(true). But if you > prefer > > > that > > > > I use a bool instead, I'm ok with that too. > > > > > > Can also use RecordTouchViewUsageInterval(/* touchview_active */ true) which > > is > > > fairly prevalent in codebase: > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%5C/%5C*%5C%20%5B%5E*... > > > of course not strictly enforced. I don't have a strong preference though. > > > > using a bool would clean up some of the logic in the *.cc file. > > Rob and I had a chat offline and he's OK leaving this as an enum. SGTM
lgtm
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914673003/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/617d480f3c9cea2a815e2e8da9d4746f76418b2c Cr-Commit-Position: refs/heads/master@{#316090} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
