Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1727823002:
[Herb] Add metrics to track button usage (Closed)
Description[Herb] Add metrics
* Adds metrics for the system back button, the return button, and the
Open in Chrome button. The SystemBack action records whether back
was hit, while a histogram tracks specifically what happened when
this happened.
* Adds a histogram that tracks why ChromeTabbedActivity was stopped.
* Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed()
so that it's possible to return false if the action really, really
isn't handled.
* Recombines the two handleBackPressed methods. They were separated
initially because the system back behavior required it for one of
the prototypes but that isn't the case anymore.
BUG=582539, 588809
Committed: https://crrev.com/480e2efb79e326b1bd940a795eda5f62711bfc5e
Cr-Commit-Position: refs/heads/master@{#377803}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changed as per offline discussion #Patch Set 3 : Rebasing #
Total comments: 18
Patch Set 4 : Comments, cleaning up the code paths #Patch Set 5 : Cleaning #
Total comments: 6
Patch Set 6 : Comments #
Total comments: 10
Patch Set 7 : Comments, deprecating #Patch Set 8 : Histogram #Patch Set 9 : Hard-coded histogram name #
Messages
Total messages: 39 (15 generated)
dfalcantara@chromium.org changed reviewers: + ianwen@chromium.org, tedchoc@chromium.org
Can you guys take a pass on this? I've got a bunch of actions that Grace et al wanted to track. It ends up adding a few to CustomTabActivity, as well.
https://codereview.chromium.org/1727823002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/1727823002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1103: return true; since we always return true, I think we don't need this https://codereview.chromium.org/1727823002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1178: if (shouldCloseTab) { per our offline discussion...we should figure out if it is ever to be in the case where !minimizeApp and !shouldCloseTab If that is the case, the back button will not do anything, which is way not good. In my opinion, I would assert shouldCloseTab and close the tab here always. Also, I think we should split out the back button a bit more granular. BACK_PRESSED_MINIMIZED_NO_TAB_CLOSED BACK_PRESSED_MINIMIZED_TAB_CLOSED BACK_PRESSED_TAB_CLOSED
Description was changed from ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. BUG=582539,588809 ========== to ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. BUG=582539,588809 ==========
https://chromiumcodereview.appspot.com/1727823002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1103: return true; On 2016/02/23 23:00:36, Ted C wrote: > since we always return true, I think we don't need this Expanded the logic a lot. https://chromiumcodereview.appspot.com/1727823002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1178: if (shouldCloseTab) { On 2016/02/23 23:00:36, Ted C wrote: > per our offline discussion...we should figure out if it is ever to be in the > case where !minimizeApp and !shouldCloseTab > > If that is the case, the back button will not do anything, which is way not > good. > > In my opinion, I would assert shouldCloseTab and close the tab here always. > > Also, I think we should split out the back button a bit more granular. > > BACK_PRESSED_MINIMIZED_NO_TAB_CLOSED > BACK_PRESSED_MINIMIZED_TAB_CLOSED > BACK_PRESSED_TAB_CLOSED Pivoted and just made this return false when necessary.
Description was changed from ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. BUG=582539,588809 ========== to ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. These are recorded as counts to allow future checking of what users do with tabs that are kept opened through Herb. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. BUG=582539,588809 ==========
dfalcantara@chromium.org changed reviewers: + isherman@chromium.org
isherman: One final CL for the current round, hopefully. Thanks for bearing with me this week! These are supposed to go hand in hand with the trial launch; adding you to give the histograms/actions a once over.
I'm pretty confused by what these metrics are trying to achieve. Maybe it would be useful to have a brief meeting/chat to discuss the intent? https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java:51: public static final int STOP_REASON_MAX = 6; nit: s/MAX/COUNT https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/ac... File tools/metrics/actions/actions.xml (right): https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/ac... tools/metrics/actions/actions.xml:3638: <action name="Herb.OpenInChromeActionButtonClicked"> Are you sure that you want to use "Herb" as the top-level name for this metric? It's a pretty opaque name. https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/ac... tools/metrics/actions/actions.xml:3647: <description>User hit the "X" button to close the tab.</description> How does the "return" button differ from a "close" button? https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/ac... tools/metrics/actions/actions.xml:13015: Foreground tab was closed because the user hit the system back button. Is this recorded in the case that MinimizedAppTabClosed is recorded? Or are they mutually exclusive? These don't really seem like distinct user actions -- the user performs the same action regardless. I'd recommend using an enumerated histogram to track these. https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:57888: + <int value="2" label="User hit the return button"/> What's a return button? https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:57891: + <int value="5" label="Another Chrome Activity is in the foreground"/> It's not entirely clear to me what this case means, in terms of user interaction. How did the other activity become foregrounded?
Yeah, do you have any particular time you can carve out? My calendar's pretty blank. I can try to fill you in with what I think they wanted the metrics for.
CCT lgtm. I have a couple of questions about the change in ChromeTabbedActivity though. :) https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (left): https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1047: RecordUserAction.record("SystemBackForNavigation"); Q: Is this change intentional? Do we decide not to record these two metrics when the current tab is null? https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1078: RecordUserAction.record("SystemBack"); After this change SystemBack will be only recorded in FullscreenActivity. If so, should we either mark it as deprecated in actions.xml or rename it to SystemBackFullscreenActivity? https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1140: boolean alwaysAllowTabClosure) { Can we move #1140 to the previous line?
Description was changed from ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. These are recorded as counts to allow future checking of what users do with tabs that are kept opened through Herb. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. BUG=582539,588809 ========== to ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. The SystemBack action records whether back was hit, while a histogram tracks specifically what happened when this happened. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. * Recombines the two handleBackPressed methods. They were separated initially because the system back behavior required it for one of the prototypes but that isn't the case anymore. BUG=582539,588809 ==========
Took another pass, based on discussions with isherman@ about histograms. I've re-merged the handleBackPressed pathways now that they're not waffling on what the back button is supposed to do. https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (left): https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1047: RecordUserAction.record("SystemBackForNavigation"); On 2016/02/24 23:25:09, Ian Wen wrote: > Q: Is this change intentional? Do we decide not to record these two metrics when > the current tab is null? This actually shouldn't ever happen -- if the tab is null, there's no way the ToolbarManager should allow a tab to go backwards. https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1078: RecordUserAction.record("SystemBack"); On 2016/02/24 23:25:09, Ian Wen wrote: > After this change SystemBack will be only recorded in FullscreenActivity. If so, > should we either mark it as deprecated in actions.xml or rename it to > SystemBackFullscreenActivity? Redoing this bit. https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1140: boolean alwaysAllowTabClosure) { On 2016/02/24 23:25:09, Ian Wen wrote: > Can we move #1140 to the previous line? This artificial split isn't necessary anymore https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java:51: public static final int STOP_REASON_MAX = 6; On 2016/02/24 23:01:02, Ilya Sherman wrote: > nit: s/MAX/COUNT Done. https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/ac... File tools/metrics/actions/actions.xml (right): https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/ac... tools/metrics/actions/actions.xml:3638: <action name="Herb.OpenInChromeActionButtonClicked"> On 2016/02/24 23:01:02, Ilya Sherman wrote: > Are you sure that you want to use "Herb" as the top-level name for this metric? > It's a pretty opaque name. Swapped over to "TaskManagement"... it's about as accurate as I can get for the nebulous proposal that I'm working with. https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/ac... tools/metrics/actions/actions.xml:3647: <description>User hit the "X" button to close the tab.</description> On 2016/02/24 23:01:02, Ilya Sherman wrote: > How does the "return" button differ from a "close" button? The return button is an explicit "close this tab and return to the previous app" button that appears to the left of the Omnibox, and only in two of the prototypes on phones. The regular close button would be something like the the X in the tab switcher or on the right of the tab in tablet mode, neither of which sends the user back to the previous app AIUI. https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/ac... tools/metrics/actions/actions.xml:13015: Foreground tab was closed because the user hit the system back button. On 2016/02/24 23:01:02, Ilya Sherman wrote: > Is this recorded in the case that MinimizedAppTabClosed is recorded? Or are > they mutually exclusive? These don't really seem like distinct user actions -- > the user performs the same action regardless. I'd recommend using an enumerated > histogram to track these. Yeah, swapped over. https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:57888: + <int value="2" label="User hit the return button"/> On 2016/02/24 23:01:02, Ilya Sherman wrote: > What's a return button? Clarified, hopefully https://chromiumcodereview.appspot.com/1727823002/diff/40001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:57891: + <int value="5" label="Another Chrome Activity is in the foreground"/> On 2016/02/24 23:01:02, Ilya Sherman wrote: > It's not entirely clear to me what this case means, in terms of user > interaction. How did the other activity become foregrounded? Is this clearer?
https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (left): https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1075: RecordUserAction.record("SystemBackForNavigation"); we dropped this user action...is that intentional? Do we track the action elsewhere? I find it to be clearer than MobileTabClobbered. https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1174: can we add an assert false here to catch this in local builds?
https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (left): https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1075: RecordUserAction.record("SystemBackForNavigation"); On 2016/02/25 18:07:41, Ted C wrote: > we dropped this user action...is that intentional? Do we track the action > elsewhere? > > I find it to be clearer than MobileTabClobbered. Yeah, figured it'd be handled by the Histogram. I can add it back if we're tracking that along a sequence, though I'm not sure what the case would be. https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1174: On 2016/02/25 18:07:41, Ted C wrote: > can we add an assert false here to catch this in local builds? Done, but I'm worried it'll result in a bunch of unparsable monkey failures.
lgtm ... discussions continue but I don't really care about the outcome :-) https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (left): https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1075: RecordUserAction.record("SystemBackForNavigation"); On 2016/02/25 18:50:14, dfalcantara wrote: > On 2016/02/25 18:07:41, Ted C wrote: > > we dropped this user action...is that intentional? Do we track the action > > elsewhere? > > > > I find it to be clearer than MobileTabClobbered. > > Yeah, figured it'd be handled by the Histogram. I can add it back if we're > tracking that along a sequence, though I'm not sure what the case would be. I'd say it can be useful for comparing system back vs toolbar back on tablets. It might also be useful for seeing how often people have to hit back more the X times. Then we could potentially surface a better UI for them to jumping back in history. Of course speculative, but I see no strong need to remove it. Or you need to mark it as deprecated in actions.xml which you just took ownership for :-) https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1174: On 2016/02/25 18:50:15, dfalcantara wrote: > On 2016/02/25 18:07:41, Ted C wrote: > > can we add an assert false here to catch this in local builds? > > Done, but I'm worried it'll result in a bunch of unparsable monkey failures. Add as much stuff as you can log then to help try to make it more actionable.
https://chromiumcodereview.appspot.com/1727823002/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java (right): https://chromiumcodereview.appspot.com/1727823002/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java:62: * @param histogramName Name to use for the histogram. Mmm, why did you choose to parametrize this? https://chromiumcodereview.appspot.com/1727823002/diff/100001/tools/metrics/a... File tools/metrics/actions/actions.xml (right): https://chromiumcodereview.appspot.com/1727823002/diff/100001/tools/metrics/a... tools/metrics/actions/actions.xml:2529: <action name="CustomTabs.SystemBackFinishedApp"> If recording this as a user action, I think it makes sense to just emit "CustomTabs.SystemBack". The user action is hitting the system back button -- even if the semantics of that action change over time, the user action will remain the same. https://chromiumcodereview.appspot.com/1727823002/diff/100001/tools/metrics/a... tools/metrics/actions/actions.xml:12989: button. Is this metric mutually exclusive with SystemBack, or are they always recorded together in case of navigation? https://chromiumcodereview.appspot.com/1727823002/diff/100001/tools/metrics/h... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1727823002/diff/100001/tools/metrics/h... tools/metrics/histograms/histograms.xml:57912: + background"/> Optional nit: These labels are pretty long. It might be better to write this as <int value="1" label="User pressed back button">The user pressed the system back button, sending Chrome to the background.</int> That way, the label text displayed in the tabular view will be fairly brief, and the more elaborate explanation will be available as hover text. That said, there's always some tension between providing brief labels, and providing sufficiently clear labels... and many (most?) users will not notice the hover text. So, I'll trust your judgement. Just keep in mind that the labels will be displayed as the left column of a tabular view.
https://codereview.chromium.org/1727823002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java (right): https://codereview.chromium.org/1727823002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java:62: * @param histogramName Name to use for the histogram. On 2016/02/25 21:41:25, Ilya Sherman wrote: > Mmm, why did you choose to parametrize this? Just in case we want to track this kind of stat for the CustomTabActivity or WebappActivity, as well. Would have said it'd be useful for DocumentActivity, but that's not going to be around much longer. https://codereview.chromium.org/1727823002/diff/100001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1727823002/diff/100001/tools/metrics/actions/... tools/metrics/actions/actions.xml:2529: <action name="CustomTabs.SystemBackFinishedApp"> On 2016/02/25 21:41:25, Ilya Sherman wrote: > If recording this as a user action, I think it makes sense to just emit > "CustomTabs.SystemBack". The user action is hitting the system back button -- > even if the semantics of that action change over time, the user action will > remain the same. Done. I'll probably end up refactoring out the back pressed histogram in ChromeTabbedActivity in a later CL, but I'd rather have some metrics now than all the metrics later. https://codereview.chromium.org/1727823002/diff/100001/tools/metrics/actions/... tools/metrics/actions/actions.xml:12989: button. On 2016/02/25 21:41:25, Ilya Sherman wrote: > Is this metric mutually exclusive with SystemBack, or are they always recorded > together in case of navigation? After my changes to record SystemBack at the top, SystemBackForNavigation will always be recorded. That being said, I don't think it's all that useful an action now that there's a proper histogram. I'd actually deleted it in an earlier patch. I've just straight up deprecated it. https://codereview.chromium.org/1727823002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1727823002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57912: + background"/> On 2016/02/25 21:41:25, Ilya Sherman wrote: > Optional nit: These labels are pretty long. It might be better to write this as > > <int value="1" label="User pressed back button">The user pressed the system > back button, sending Chrome to the background.</int> > > That way, the label text displayed in the tabular view will be fairly brief, and > the more elaborate explanation will be available as hover text. > > That said, there's always some tension between providing brief labels, and > providing sufficiently clear labels... and many (most?) users will not notice > the hover text. So, I'll trust your judgement. Just keep in mind that the > labels will be displayed as the left column of a tabular view. How's this?
Metrics LGTM, thanks. https://codereview.chromium.org/1727823002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java (right): https://codereview.chromium.org/1727823002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java:62: * @param histogramName Name to use for the histogram. On 2016/02/25 22:18:14, dfalcantara wrote: > On 2016/02/25 21:41:25, Ilya Sherman wrote: > > Mmm, why did you choose to parametrize this? > > Just in case we want to track this kind of stat for the CustomTabActivity or > WebappActivity, as well. Would have said it'd be useful for DocumentActivity, > but that's not going to be around much longer. Seems like the sort of capability that would be best to add once it's needed, rather than as a just-in-case ahead of time.
Thanks! https://codereview.chromium.org/1727823002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java (right): https://codereview.chromium.org/1727823002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/ActivityStopMetrics.java:62: * @param histogramName Name to use for the histogram. On 2016/02/25 23:13:57, Ilya Sherman wrote: > On 2016/02/25 22:18:14, dfalcantara wrote: > > On 2016/02/25 21:41:25, Ilya Sherman wrote: > > > Mmm, why did you choose to parametrize this? > > > > Just in case we want to track this kind of stat for the CustomTabActivity or > > WebappActivity, as well. Would have said it'd be useful for DocumentActivity, > > but that's not going to be around much longer. > > Seems like the sort of capability that would be best to add once it's needed, > rather than as a just-in-case ahead of time. Fair enough. I've hard-coded the histogram name for now since it's easy enough to add in the capability later.
The CQ bit was checked by dfalcantara@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org, tedchoc@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1727823002/#ps160001 (title: "Hard-coded histogram name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1727823002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1727823002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dfalcantara@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1727823002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1727823002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dfalcantara@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1727823002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1727823002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dfalcantara@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1727823002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1727823002/160001
Message was sent while issue was closed.
Description was changed from ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. The SystemBack action records whether back was hit, while a histogram tracks specifically what happened when this happened. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. * Recombines the two handleBackPressed methods. They were separated initially because the system back behavior required it for one of the prototypes but that isn't the case anymore. BUG=582539,588809 ========== to ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. The SystemBack action records whether back was hit, while a histogram tracks specifically what happened when this happened. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. * Recombines the two handleBackPressed methods. They were separated initially because the system back behavior required it for one of the prototypes but that isn't the case anymore. BUG=582539,588809 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. The SystemBack action records whether back was hit, while a histogram tracks specifically what happened when this happened. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. * Recombines the two handleBackPressed methods. They were separated initially because the system back behavior required it for one of the prototypes but that isn't the case anymore. BUG=582539,588809 ========== to ========== [Herb] Add metrics * Adds metrics for the system back button, the return button, and the Open in Chrome button. The SystemBack action records whether back was hit, while a histogram tracks specifically what happened when this happened. * Adds a histogram that tracks why ChromeTabbedActivity was stopped. * Edits the back pressed logic in ChromeTabbedActivity#handleBackPressed() so that it's possible to return false if the action really, really isn't handled. * Recombines the two handleBackPressed methods. They were separated initially because the system back behavior required it for one of the prototypes but that isn't the case anymore. BUG=582539,588809 Committed: https://crrev.com/480e2efb79e326b1bd940a795eda5f62711bfc5e Cr-Commit-Position: refs/heads/master@{#377803} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/480e2efb79e326b1bd940a795eda5f62711bfc5e Cr-Commit-Position: refs/heads/master@{#377803} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
