Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2962773002:
[Tab Metrics] Added TabManager.SessionRestore.SwitchToTab UMA. (Closed)
Description[Tab Metrics] Added TabManager.SessionRestore.SwitchToTab UMA.
This is the first of two metrics recorded in the TabManager when switching to a
different tab. If there is currently a session restore in progress and the user
switches tabs, we emit an UMA histogram recording the tab loading state. As a
follow-up, we'll add a separate UMA if the TabManager is in a background tab
loading state, once the tab manager has that functionality.
BUG=732837
Review-Url: https://codereview.chromium.org/2962773002
Cr-Commit-Position: refs/heads/master@{#483525}
Committed: https://chromium.googlesource.com/chromium/src/+/97e4ec20d584f59fe5a701cde8b29257b0a0b31d
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove gratuitous TODOs #
Total comments: 2
Patch Set 3 : Fix comments #
Total comments: 7
Patch Set 4 : Addresses histogram.xml comments and adds standard UMA enum warning #
Messages
Total messages: 32 (11 generated)
shaseley@google.com changed reviewers: + fmeawad@chromium.org, lpy@chromium.org, zhenw@chromium.org
LGTM. A general advice, if the current code is consistent and does not look like it is missing something, then I would avoid adding a TODO there. Instead track missing functionality using bugs. If the pending feature is not implemented then nothing to be fixed in the code. https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:953: // restore and background tab loading. nit: This TODO is implicit from the TODOs in the header file.
lgtm with nits. https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:337: // TODO(shaseley): Add background tab loading stats when it gets integrated Merge this TODO into the one in .cc, it doesn't give any information along with the API. https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:40: TAB_IS_NOT_LOADING = 0, I think that TAB_IS_NOT_LOADING is not accurate, a tab can be loaded and is not loading. We can do it in a separate patch.
I thought the plan was to add a stats collector class to do this and all the follow-up stats reporting? Was that still the plan?
https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:953: // restore and background tab loading. On 2017/06/27 21:22:02, fmeawad wrote: > nit: This TODO is implicit from the TODOs in the header file. OK, that makes sense, I'll remove this comment. https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:337: // TODO(shaseley): Add background tab loading stats when it gets integrated On 2017/06/27 22:30:36, lpy wrote: > Merge this TODO into the one in .cc, it doesn't give any information along with > the API. Per fmeawad@'s comment, I was planning to remove the one in the .cc. Is it OK to just remove both since the code is consistent as is? https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2962773002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:40: TAB_IS_NOT_LOADING = 0, On 2017/06/27 22:30:36, lpy wrote: > I think that TAB_IS_NOT_LOADING is not accurate, a tab can be loaded and is not > loading. > > We can do it in a separate patch. I know what you mean, the names are a little confusing in that regard. This enum mirrors the one used in session restore for the same purpose, so I kept the names the same erring on the side of consistency.
> A general advice, if the current code is consistent and does not look like it is > missing something, then I would avoid adding a TODO there. Instead track missing > functionality using bugs. If the pending feature is not implemented then nothing > to be fixed in the code. Thanks fmeawad@, I appreciate the advice.
On 2017/06/27 23:11:52, Zhen Wang wrote: > I thought the plan was to add a stats collector class to do this and all the > follow-up stats reporting? Was that still the plan? I think we said we would *eventually* add the stats collector class, but not necessarily for the first couple of metrics. I can certainly add it though if we decide we should do it now.
On 2017/06/27 23:24:52, shaseley wrote: > On 2017/06/27 23:11:52, Zhen Wang wrote: > > I thought the plan was to add a stats collector class to do this and all the > > follow-up stats reporting? Was that still the plan? > > I think we said we would *eventually* add the stats collector class, but not > necessarily for the first couple of metrics. I can certainly add it though if we > decide we should do it now. My personal opinion is that it may make your and Duc's life easier to have that class earlier. So all later CLs can just share the same stats collector and no rewrites are needed. But I will defer that to Fadi to decide. Another suggestion is that usually people reply comments after uploading a new patch with fixes. So people can read the replies and the new patch in one round.
On 2017/06/27 23:49:46, Zhen Wang wrote: > On 2017/06/27 23:24:52, shaseley wrote: > > On 2017/06/27 23:11:52, Zhen Wang wrote: > > > I thought the plan was to add a stats collector class to do this and all the > > > follow-up stats reporting? Was that still the plan? > > > > I think we said we would *eventually* add the stats collector class, but not > > necessarily for the first couple of metrics. I can certainly add it though if > we > > decide we should do it now. > > My personal opinion is that it may make your and Duc's life easier to have that > class earlier. So all later CLs can just share the same stats collector and no > rewrites are needed. But I will defer that to Fadi to decide. > > Another suggestion is that usually people reply comments after uploading a new > patch with fixes. So people can read the replies and the new patch in one round. I defer to Fadi as well, as as aside: it's a good exercise (for our interns) to clearly see the need for the StatsCollector and then implement it :)
On 2017/06/27 23:49:46, Zhen Wang wrote: > Another suggestion is that usually people reply comments after uploading a new > patch with fixes. So people can read the replies and the new patch in one round. Thanks, that makes sense; I'll make sure to take this into account going forward.
On 2017/06/28 00:07:56, shaseley wrote: > On 2017/06/27 23:49:46, Zhen Wang wrote: > > > Another suggestion is that usually people reply comments after uploading a new > > patch with fixes. So people can read the replies and the new patch in one > round. > > Thanks, that makes sense; I'll make sure to take this into account going > forward. Discussing this offline with Zhen, we should land this CL as is, and have the TabManagerStatsCollector added in a separate CL.
zhenw@google.com changed reviewers: + zhenw@google.com
lgtm with nit https://codereview.chromium.org/2962773002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2962773002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.cc:833: // record it. nit: The comment here is describing the case where the if-check fails. But the situation here is when |old_contents| is set. So maybe tweak it a little. Something like: Only record when switch happens, i.e., when |old_contents| is set. Or you do not even need this comment, because there is comment above saying a tab has switched when |old_contents| is set. Either is fine to me.
lgtm again with chromium account ...
https://codereview.chromium.org/2962773002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2962773002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.cc:833: // record it. On 2017/06/29 15:44:00, zhenw wrote: > nit: The comment here is describing the case where the if-check fails. But the > situation here is when |old_contents| is set. So maybe tweak it a little. > Something like: Only record when switch happens, i.e., when |old_contents| is > set. > > Or you do not even need this comment, because there is comment above saying a > tab has switched when |old_contents| is set. > > Either is fine to me. Done, I changed the comment.
shaseley@google.com changed reviewers: + chrisha@chromium.org, mpearson@chromium.org, panicker@chromium.org
+chrisha@ Hi Chris, PTAL? +mpearson@ Hi Mark, PTAL at histograms.xml/enums.xml? Thanks.
lgtm
https://codereview.chromium.org/2962773002/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2962773002/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:36: enum TabLoadingState { This needs the standard warning about changing values. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2962773002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2962773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:77603: + Number of times that a user switches to a tab during a session restore, I'd rather rephrase this. It's not recording the "number of times". The aggregate (sum) is a number. Here's my attempt: The loading state of a tab at the time a user switched this tab during session restore. https://codereview.chromium.org/2962773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:77604: + broken down by tab loading state. Please add information about: * When I start Chrome and it begin restoring a session, is this initial tab that gets shown first (not explicitly selected, but select nonetheless) recorded? * If I bring a window to the front of a different Chrome window during session restore, is that recorded? Or is it not recorded because I haven't actually switched tabs? * Roughly, when is session restore considered over?
https://codereview.chromium.org/2962773002/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2962773002/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:36: enum TabLoadingState { On 2017/06/29 18:25:51, Mark P wrote: > This needs the standard warning about changing values. > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Done. https://codereview.chromium.org/2962773002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2962773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:77603: + Number of times that a user switches to a tab during a session restore, On 2017/06/29 18:25:51, Mark P wrote: > I'd rather rephrase this. It's not recording the "number of times". The > aggregate (sum) is a number. Here's my attempt: > > The loading state of a tab at the time a user switched this tab during session > restore. Done. https://codereview.chromium.org/2962773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:77604: + broken down by tab loading state. On 2017/06/29 18:25:51, Mark P wrote: > Please add information about: > * When I start Chrome and it begin restoring a session, is this initial tab that > gets shown first (not explicitly selected, but select nonetheless) recorded? > * If I bring a window to the front of a different Chrome window during session > restore, is that recorded? Or is it not recorded because I haven't actually > switched tabs? > * Roughly, when is session restore considered over? Done, hopefully the text here is much clearer now.
xml files lgtm --mark https://codereview.chromium.org/2962773002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2962773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:77604: + broken down by tab loading state. On 2017/06/29 19:45:16, shaseley wrote: > On 2017/06/29 18:25:51, Mark P wrote: > > Please add information about: > > * When I start Chrome and it begin restoring a session, is this initial tab > that > > gets shown first (not explicitly selected, but select nonetheless) recorded? > > * If I bring a window to the front of a different Chrome window during session > > restore, is that recorded? Or is it not recorded because I haven't actually > > switched tabs? > > * Roughly, when is session restore considered over? > > Done, hopefully the text here is much clearer now. Much better now, thanks!
The CQ bit was checked by shaseley@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shaseley@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from lpy@chromium.org, fmeawad@chromium.org, zhenw@chromium.org, zhenw@google.com, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2962773002/#ps60001 (title: "Addresses histogram.xml comments and adds standard UMA enum warning")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498776014106310,
"parent_rev": "7a4f9830425ae18fde542707bb1fbcb617d07768", "commit_rev":
"97e4ec20d584f59fe5a701cde8b29257b0a0b31d"}
Message was sent while issue was closed.
Description was changed from ========== [Tab Metrics] Added TabManager.SessionRestore.SwitchToTab UMA. This is the first of two metrics recorded in the TabManager when switching to a different tab. If there is currently a session restore in progress and the user switches tabs, we emit an UMA histogram recording the tab loading state. As a follow-up, we'll add a separate UMA if the TabManager is in a background tab loading state, once the tab manager has that functionality. BUG=732837 ========== to ========== [Tab Metrics] Added TabManager.SessionRestore.SwitchToTab UMA. This is the first of two metrics recorded in the TabManager when switching to a different tab. If there is currently a session restore in progress and the user switches tabs, we emit an UMA histogram recording the tab loading state. As a follow-up, we'll add a separate UMA if the TabManager is in a background tab loading state, once the tab manager has that functionality. BUG=732837 Review-Url: https://codereview.chromium.org/2962773002 Cr-Commit-Position: refs/heads/master@{#483525} Committed: https://chromium.googlesource.com/chromium/src/+/97e4ec20d584f59fe5a701cde8b2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/97e4ec20d584f59fe5a701cde8b2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
