Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1159023005:
Log the amount of time that the RecentTabsPage is in the foreground to UMA (Closed)
DescriptionLog the amount of time that the RecentTabsPage is in the foreground to UMA
The logged time excludes:
- Time that the RecentTabsPage is open but Chrome is in the background
- Time that the RecentTabsPage is open but a different tab is in the foreground
BUG=477054
TEST=None
Committed: https://crrev.com/766491f596a7326a098edf3c9c8526f8d3e917b8
Cr-Commit-Position: refs/heads/master@{#332661}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 23 (7 generated)
pkotwicz@chromium.org changed reviewers: + nyquist@chromium.org
Patchset #1 (id:1) has been deleted
nyquist@ PTAL I am unsure whether the time that the "Recent Tabs" page is in the foreground is the most useful. I can see the time that the "Recent Tabs" page is open (regardless of whether it was visible during that time) as being useful as well.
On 2015/05/30 22:20:14, pkotwicz wrote: > nyquist@ PTAL > > I am unsure whether the time that the "Recent Tabs" page is in the foreground is > the most useful. > I can see the time that the "Recent Tabs" page is open (regardless of whether it > was visible during that time) as being useful as well. Do you mean open, but in a different tab? Or just open and not foregrounded?
https://codereview.chromium.org/1159023005/diff/20001/chrome/android/java_sta... File chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/RecentTabsPage.java (right): https://codereview.chromium.org/1159023005/diff/20001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/RecentTabsPage.java:91: // |mInForeground| will be updated once |mView| is attached to the window. Nit: {@link #mInForeground} and {@link #mView}, etc. throughout this file. https://codereview.chromium.org/1159023005/diff/20001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/RecentTabsPage.java:116: mForegroundTimeMs = System.currentTimeMillis(); You would want to use SystemClock#elapsedRealtime() here and below. It's guaranteed to be monotonic, regardless of changes to time through the network, etc. See http://developer.android.com/reference/android/os/SystemClock.html for details. https://codereview.chromium.org/1159023005/diff/20001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/RecentTabsPage.java:118: RecordHistogram.recordTimesHistogram("RecentTabsPage.TimeVisibleAndroid", Are we guaranteed to get a call to this method with |true| before ever getting a call with |false|? If yes, is that guaranteed just now based on how this class is used, or is it generally true with the current implementation of this class? https://codereview.chromium.org/1159023005/diff/20001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/RecentTabsPage.java:186: // The value of View::isAttachedToWindow() changes after onViewDetachedFromWindow() Nit: {@link View#isAttachedWindow()} and {@link #onViewDetachedFromWindow(View)} etc., throughout this file.
nyquist@ can you please take another look? https://codereview.chromium.org/1159023005/diff/20001/chrome/android/java_sta... File chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/RecentTabsPage.java (right): https://codereview.chromium.org/1159023005/diff/20001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/RecentTabsPage.java:116: mForegroundTimeMs = System.currentTimeMillis(); On 2015/06/01 14:09:13, nyquist wrote: > You would want to use SystemClock#elapsedRealtime() here and below. > It's guaranteed to be monotonic, regardless of changes to time through the > network, etc. > See http://developer.android.com/reference/android/os/SystemClock.html for > details. Done. https://codereview.chromium.org/1159023005/diff/20001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/RecentTabsPage.java:118: RecordHistogram.recordTimesHistogram("RecentTabsPage.TimeVisibleAndroid", I do not completely understand the intent of your question. I think that we are guaranteed that this method will be called with the parameter isAttachedToWindow == true before it is called with isAttachedToWindow == false. We are however not guaranteed that the first time that updateForegroundState() that isForeground == true. (It is possible for the recent tabs page to be opened via a JS timeout while Chrome is in the background)
lgtm
pkotwicz@chromium.org changed reviewers: + isherman@chromium.org
isherman@ for tools/metrics
https://codereview.chromium.org/1159023005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1159023005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32687: +<histogram name="RecentsTabsPage.TimeVisibleAndroid" units="milliseconds"> It looks like you're introducing a new group name, "RecentTabsPage", into the top-level namespace. That might be appropriate; but before you do, please double-check whether there is any existing group that is appropriate. If not, please think about whether there's a way to generalize the new group name so that it's likely to carve out a reasonably sized portion of the top-level namespace. https://codereview.chromium.org/1159023005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32691: + and Chrome is in the foreground on Android. When is this recorded -- when the "Recent Tabs" page is closed or Chrome is backgrounded?
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
isherman@ can you please take another look? https://codereview.chromium.org/1159023005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1159023005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32687: +<histogram name="RecentsTabsPage.TimeVisibleAndroid" units="milliseconds"> I think the new namespace makes sense. The "Recent Tabs" page is a top level page. We also have a top level namespace for the "New Tab" page (NewTabPage.*) and the "History" page (WebHistory.*) https://codereview.chromium.org/1159023005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32691: + and Chrome is in the foreground on Android. I have updated the comment to be clearer. The metric is recorded whenever the page moves out of the foreground. This includes when the page is closed, when the user switches to a different tab and when Chrome is backgrounded.
https://codereview.chromium.org/1159023005/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1159023005/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:32744: +<histogram name="RecentTabsPage.TimeVisibleAndroid" units="milliseconds"> Given Recent Tabs is a subpage, and part of NewTabPage, it could be argued that it would be OK to just use: NewTabPage.RecentTabsPageTimeVisibleAndroid I don't feel strongly about this though.
https://codereview.chromium.org/1159023005/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1159023005/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:32744: +<histogram name="RecentTabsPage.TimeVisibleAndroid" units="milliseconds"> NewTabPage.RecentTabsPageTimeVisibleAndroid is too long for my liking
https://codereview.chromium.org/1159023005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1159023005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32687: +<histogram name="RecentsTabsPage.TimeVisibleAndroid" units="milliseconds"> On 2015/06/02 15:35:43, pkotwicz wrote: > I think the new namespace makes sense. The "Recent Tabs" page is a top level > page. We also have a top level namespace for the "New Tab" page (NewTabPage.*) > and the "History" page (WebHistory.*) Are you planning to introduce several additional metrics for this page? If so, then it's probably reasonable to have a top-level group. But if this is essentially a one-off metric, then I think it would be best to think more about whether it makes sense to fit into another group. FWIW, "NewTabPage.RecentTabsPage.TimeVisibleAndroid" is not unreasonably long, compared to other histogram names. The dashboard provides typeahead autocomplete, so it's not like you'll need to be typing the whole thing out.
isherman@ can you please take another look? I was thinking of this as a one-off metric. "NewTabPage.RecentTabsPage.TimeVisibleAndroid" it is then!
LGTM, thanks.
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1159023005/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159023005/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/766491f596a7326a098edf3c9c8526f8d3e917b8 Cr-Commit-Position: refs/heads/master@{#332661} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
