Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754103003:
Adds metric to help assess last_n impact on tab restores. (Closed)
|
Created:
3 years, 9 months ago by carlosk Modified:
3 years, 9 months ago CC:
chromium-reviews, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org, jianli, dewittj Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds metric to help assess last_n impact on tab restores.
This change adds a new metric to help us assess the impact of last_n on
the cases where an existing tab has to have its contents restored. This
change only affects Chrome on Android.
We track tab restores in Tab and call the appropriate, newly introduced
methods in OfflinePageUtils where the reporting actually happens.
BUG=688588
Review-Url: https://codereview.chromium.org/2754103003
Cr-Commit-Position: refs/heads/master@{#458530}
Committed: https://chromium.googlesource.com/chromium/src/+/e9f7809b63666b9c8c558dbde9397b4ce23fe119
Patch Set 1 #
Total comments: 8
Patch Set 2 : Removed canSaveUrl method because it was already implemented elsewhere. #
Total comments: 12
Patch Set 3 : Changed histogram enum values and code now uses a lookup table. #
Total comments: 3
Patch Set 4 : Moved constants definition location and made them private. #
Total comments: 2
Patch Set 5 : Added restore parameter to observer calls; updated implementation accordingly. #
Total comments: 3
Patch Set 6 : Fixed many uses of the changed API that I missed before. #
Total comments: 2
Patch Set 7 : Reverted the observer API; resetting the restoring flag after observer calls. #
Messages
Total messages: 43 (26 generated)
The CQ bit was checked by carlosk@chromium.org 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...
carlosk@chromium.org changed reviewers: + dimich@chromium.org
dimich@: PTAL. dewittj@, jianli@: FYI. https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:744: } else if (canSaveUrl(validatedUrl) && !tab.isIncognito()) { Are there other known cases where a page would not be allowed to be saved offline? https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:762: : TabRestoreType.FAILED; Are there other net errors that characterize a Dino page?
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive by https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:89: private static class TabRestoreType { Why not define it as a separate class? Alternative is to do all that on C++ side and generate the java version of the enum. https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:101: public static final int LAST = CRASHED; Why not call it COUNT and add standard UMA comments? https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:778: private static boolean canSaveUrl(String url) { how about OPBridge::CanSavePage?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:89: private static class TabRestoreType { On 2017/03/16 20:59:20, fgorski wrote: > Why not define it as a separate class? > > Alternative is to do all that on C++ side and generate the java version of the > enum. These are being used only internally in this class file so it seemed better to just keep them here. There's no need for them in C++ either. https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:101: public static final int LAST = CRASHED; On 2017/03/16 20:59:21, fgorski wrote: > Why not call it COUNT and add standard UMA comments? > I didn't understand what you meant by "standard UMA comments" but I did change from LAST to COUNT. Mind that I copied this from some place in the Java codebase so the "model" exists! :) https://codereview.chromium.org/2754103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:778: private static boolean canSaveUrl(String url) { On 2017/03/16 20:59:21, fgorski wrote: > how about OPBridge::CanSavePage? Done.
https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:742: tabRestoreType = TabRestoreType.WHILE_ONLINE_COULD_BE_SAVED_OFFLINE; // == 1 What does this measure? Not clear. https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:747: if (!isConnected()) tabRestoreType += TabRestoreType.WHILE_OFFLINE; // == 4 I'd have this more straightforward, w/o doing math on enum values. It is harder to read and error-prone in the future... The alternative to a long chain of 'ifs' is to have a table, like | is online | has_offline_page | is last_n | UMA_enum_value https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1527: protected void didFinishPageLoad(String validatedUrl) { You probalby can just tab.getURL() instead of passing a url...
https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:97: public static final int CRASHED = 10; Please add this: // NOTE: always keep this entry at the end. Add new result types only // immediately above this line. Make sure to update the corresponding // histogram enum accordingly. https://codereview.chromium.org/2754103003/diff/20001/components/offline_page... File components/offline_pages/core/offline_page_model.cc (right): https://codereview.chromium.org/2754103003/diff/20001/components/offline_page... components/offline_pages/core/offline_page_model.cc:27: // Note: Must be kept in sync with Java implementation in remove, please. https://codereview.chromium.org/2754103003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2754103003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:103065: + <int value="0" label="WhileOnline"/> This can have spaces and sentence case.
Thanks. https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:97: public static final int CRASHED = 10; On 2017/03/17 03:42:51, fgorski wrote: > Please add this: > // NOTE: always keep this entry at the end. Add new result types only > // immediately above this line. Make sure to update the corresponding > // histogram enum accordingly. Done. https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:742: tabRestoreType = TabRestoreType.WHILE_ONLINE_COULD_BE_SAVED_OFFLINE; // == 1 On 2017/03/17 01:39:51, Dmitry Titov wrote: > What does this measure? Not clear. This allows us put in a separate bucket tab restores that succeeded but were to non offline save-able pages (anything non HTTP/HTTPS or in an incognito tab). The more important/common case is the NTP in the offline case. I added further explanations to the histogram enum entries on why these cases are tracked separately. I also renamed WHILE_O(N|FF)LINE_CANT_BE_SAVED_OFFLINE and repurpose it to catch the opposite case so that WHILE_O(N|FF)LINE would track what we actually care about. This seems easier to read and interpret. WDYT? https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:747: if (!isConnected()) tabRestoreType += TabRestoreType.WHILE_OFFLINE; // == 4 On 2017/03/17 01:39:51, Dmitry Titov wrote: > I'd have this more straightforward, w/o doing math on enum values. It is harder > to read and error-prone in the future... > > The alternative to a long chain of 'ifs' is to have a table, like > | is online | has_offline_page | is last_n | UMA_enum_value > Done. I had considered a lookup table before too but preferred this one because it was shorter. I agree readability is better now. https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2754103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1527: protected void didFinishPageLoad(String validatedUrl) { On 2017/03/17 01:39:51, Dmitry Titov wrote: > You probalby can just tab.getURL() instead of passing a url... Done. https://codereview.chromium.org/2754103003/diff/20001/components/offline_page... File components/offline_pages/core/offline_page_model.cc (right): https://codereview.chromium.org/2754103003/diff/20001/components/offline_page... components/offline_pages/core/offline_page_model.cc:27: // Note: Must be kept in sync with Java implementation in On 2017/03/17 03:42:51, fgorski wrote: > remove, please. Done. https://codereview.chromium.org/2754103003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2754103003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:103065: + <int value="0" label="WhileOnline"/> On 2017/03/17 03:42:51, fgorski wrote: > This can have spaces and sentence case. Done. Indeed much better! I also learned about and added descriptions to each item that show up as hover text in UMA.
lgtm with nit https://codereview.chromium.org/2754103003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:104: */ nit. These values should probably go above with other static final items.
lgtm with same nit as Filip had: https://codereview.chromium.org/2754103003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:104: */ On 2017/03/17 22:57:29, fgorski wrote: > nit. These values should probably go above with other static final items. +1, and they should not be public...
carlosk@chromium.org changed reviewers: + isherman@chromium.org, tedchoc@chromium.org
Thanks. tedchoc@: PTAL at Tab.java. isherman@: PTAL at histograms. https://codereview.chromium.org/2754103003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2754103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:104: */ On 2017/03/17 23:30:46, Dmitry Titov wrote: > On 2017/03/17 22:57:29, fgorski wrote: > > nit. These values should probably go above with other static final items. > > +1, and they should not be public... Done.
metrics lgtm
https://codereview.chromium.org/2754103003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2754103003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1529: OfflinePageUtils.finishedRestorePageLoad(this); can we change onPageLoadFinished, onPageLoadFailed, and onCrash to include whether it was being restored? Ideally, OfflinePageUtils could track this by updating the RecentTabTracker to be a bit more generic. The onCrash observer notification can move into handleTabCrash as it doesn't really need to be in the TabWebContentsObserver class.
The CQ bit was checked by carlosk@chromium.org 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 checked by carlosk@chromium.org to run a CQ dry run
Patchset #5 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. tedchoc@: PTAL at my inline comments. https://codereview.chromium.org/2754103003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2754103003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1529: OfflinePageUtils.finishedRestorePageLoad(this); On 2017/03/20 18:08:18, Ted C wrote: > can we change onPageLoadFinished, onPageLoadFailed, and onCrash to include > whether it was being restored? > > Ideally, OfflinePageUtils could track this by updating the RecentTabTracker to > be a bit more generic. > > The onCrash observer notification can move into handleTabCrash as it doesn't > really need to be in the TabWebContentsObserver class. Done. https://codereview.chromium.org/2754103003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/EmptyTabObserver.java (right): https://codereview.chromium.org/2754103003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/EmptyTabObserver.java:16: public class EmptyTabObserver implements TabObserver { Presubmit checks were sad at me because of the space between the curly braces for empty implementations. So to make consistent I reformatted the whole file to follow the current enforced standards. https://codereview.chromium.org/2754103003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2754103003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:899: observers.next().onPageLoadFinished(this, isBeingRestored()); It seems to me this is triggered only due to user-driven actions so this looks like the right way to provide the restore information in this case. WDYT? https://codereview.chromium.org/2754103003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1544: mIsBeingRestored = false; This is the only place I was a bit unsure about moving the restore flag reset to the end of the method because there are more complex calls happening before it. I looked into some of them and they seemed unrelated enough to restore/not-restore so I made the change. Hopefully trybots will scream if anything is effected? The other methods below are simple enough to make the change safe.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by carlosk@chromium.org 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...
https://codereview.chromium.org/2754103003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2754103003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1544: mIsBeingRestored = false; Hmm...by moving setting mIsBeingRestored you are negating the need to add it to the observer as all the observers could just query isBeingRestored. I think to keep the logic the same, you need to pull out a local variable at the top and set it to false initially. Or since there are currently only two places that look at isBeingRestored() (readermode and during close), I think we could potentially just move the booleans as well. But we should pick one or the other :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by carlosk@chromium.org 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...
Thanks! Yeah, how could I have missed that?! :) https://codereview.chromium.org/2754103003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2754103003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1544: mIsBeingRestored = false; On 2017/03/21 18:22:53, Ted C wrote: > Hmm...by moving setting mIsBeingRestored you are negating the need to add it to > the observer as all the observers could just query isBeingRestored. I think to > keep the logic the same, you need to pull out a local variable at the top and > set it to false initially. > > Or since there are currently only two places that look at isBeingRestored() > (readermode and during close), I think we could potentially just move the > booleans as well. > > But we should pick one or the other :-) Done.
lgtm
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 carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, fgorski@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2754103003/#ps140001 (title: "Reverted the observer API; resetting the restoring flag after observer calls.")
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": 140001, "attempt_start_ts": 1490127310138710,
"parent_rev": "6442d6e4c7b405310f22a1e8c191faf6e1763425", "commit_rev":
"e9f7809b63666b9c8c558dbde9397b4ce23fe119"}
Message was sent while issue was closed.
Description was changed from ========== Adds metric to help assess last_n impact on tab restores. This change adds a new metric to help us assess the impact of last_n on the cases where an existing tab has to have its contents restored. This change only affects Chrome on Android. We track tab restores in Tab and call the appropriate, newly introduced methods in OfflinePageUtils where the reporting actually happens. BUG=688588 ========== to ========== Adds metric to help assess last_n impact on tab restores. This change adds a new metric to help us assess the impact of last_n on the cases where an existing tab has to have its contents restored. This change only affects Chrome on Android. We track tab restores in Tab and call the appropriate, newly introduced methods in OfflinePageUtils where the reporting actually happens. BUG=688588 Review-Url: https://codereview.chromium.org/2754103003 Cr-Commit-Position: refs/heads/master@{#458530} Committed: https://chromium.googlesource.com/chromium/src/+/e9f7809b63666b9c8c558dbde939... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e9f7809b63666b9c8c558dbde939... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
