Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2096363003:
[ImportantSites] Adding usage metrics. (Closed)
Description[ImportantSites] Adding usage metrics.
BUG=622468, 605761
Committed: https://crrev.com/357700a996c2e31ad34d6498d68ef0da0b6474c0
Cr-Commit-Position: refs/heads/master@{#403317}
Patch Set 1 #
Total comments: 25
Patch Set 2 : comments #Patch Set 3 : comments #
Total comments: 15
Patch Set 4 : comments #
Total comments: 26
Patch Set 5 : comments #Patch Set 6 : comments & test fix #
Total comments: 10
Patch Set 7 : comments & null checks #Patch Set 8 : Switched percent to be out of 20 #Messages
Total messages: 28 (6 generated)
dmurph@chromium.org changed reviewers: + mpearson@chromium.org, tedchoc@chromium.org, twellington@chromium.org
Hello! I'm adding some usage metrics for our Important Storage feature. This isn't all of them, but just the basic usage ones. Can you PTAL? Daniel
The CQ bit was checked by dmurph@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:364: "History.ClearBrowsingData.ImportantDialogShown", false); Are we trying to determine how often the dialog shows? If so, should this only get logged if the feature is enabled? https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:588: RecordHistogram.recordCount100Histogram( Since the possible number of deselected domains is low, I think this should use recordCustomCountHistogram() https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:589: "History.ClearBrowsingData.NumImportantDeselected", deselectedDomains.length); I don't see a corresponding entry in histograms.xml for this one. https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:248: RecordHistogram.recordCount100Histogram( Same recommendation here wrt to recordCustomCountHistogram() https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:249: "ClearBrowsingDialog.NumImportant", mImportantDomains.length); This still needs an entry in histograms.xml, right? I like "History.ClearBrowsingData.NumImportant" since it doesn't introduce a new top-level "ClearBrowsingDialog". https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:327: RecordHistogram.recordBooleanHistogram("Storage.AndroidStorageSettings.Clear", true); Should this record a user action instead of a histogram or is false recorded somewhere? https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:17528: + Recorded when the user presses the 'clear' button in the Clear Browsing nit: clear browsing data dialog? https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:17533: +<histogram name="History.ClearBrowsingData.NumImportant"> Should this be NumImportantDeselected? https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:17548: + is deserialized (resumed) from the background. Out of curiosity, what are we going to use this metric for? https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:54831: +<histogram name="Storage.AndroidManage.Button" enum="AndroidManageSpaceButton"> nit: ActionTaken seems like a better name than Button. https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:54836: + of that button disables UMA recording (as we factory reset the app). Is there a way we can get an idea of how many users clear app data vs take one of the other actions? Maybe we can ask UMA folks if there's a way to force an upload before data is cleared? If not, we can brainstorm some other options.
https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:125: RecordHistogram.recordBooleanHistogram( RecordHistogram requires the native library to be loaded. If you fail to init the native library, then it will crash, so I don't think this is a metric you'll be able to record. Similarly below, you need to make sure you're not trying to log any metrics in the event you don't initial the native library. So I think you need to guard the metric in the clear all data block
https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:364: "History.ClearBrowsingData.ImportantDialogShown", false); On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > Are we trying to determine how often the dialog shows? If so, should this only get logged if the feature is enabled? Done. https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:588: RecordHistogram.recordCount100Histogram( On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > Since the possible number of deselected domains is low, I think this should use recordCustomCountHistogram() Done. https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:589: "History.ClearBrowsingData.NumImportantDeselected", deselectedDomains.length); On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > I don't see a corresponding entry in histograms.xml for this one. Done. https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:248: RecordHistogram.recordCount100Histogram( On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > Same recommendation here wrt to recordCustomCountHistogram() Done. https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:249: "ClearBrowsingDialog.NumImportant", mImportantDomains.length); On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > This still needs an entry in histograms.xml, right? > > I like "History.ClearBrowsingData.NumImportant" since it doesn't introduce a new top-level "ClearBrowsingDialog". Done. https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:125: RecordHistogram.recordBooleanHistogram( On 2016/06/27 at 20:45:42, Ted C wrote: > RecordHistogram requires the native library to be loaded. If you fail to init the native library, then it will crash, so I don't think this is a metric you'll be able to record. > > Similarly below, you need to make sure you're not trying to log any metrics in the event you don't initial the native library. So I think you need to guard the metric in the clear all data block That makes sense. I guess we won't really know those stats then.
Oops, forgot to add UserActions. Doing that now.
https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/2096363003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:327: RecordHistogram.recordBooleanHistogram("Storage.AndroidStorageSettings.Clear", true); On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > Should this record a user action instead of a histogram or is false recorded somewhere? Done. https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:17528: + Recorded when the user presses the 'clear' button in the Clear Browsing On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > nit: clear browsing data dialog? Done. https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:17533: +<histogram name="History.ClearBrowsingData.NumImportant"> On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > Should this be NumImportantDeselected? Yes, thanks. https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:17548: + is deserialized (resumed) from the background. On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > Out of curiosity, what are we going to use this metric for? Changed to a user action. https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:54831: +<histogram name="Storage.AndroidManage.Button" enum="AndroidManageSpaceButton"> On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > nit: ActionTaken seems like a better name than Button. Done. https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:54831: +<histogram name="Storage.AndroidManage.Button" enum="AndroidManageSpaceButton"> On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > nit: ActionTaken seems like a better name than Button. Done. https://codereview.chromium.org/2096363003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:54836: + of that button disables UMA recording (as we factory reset the app). On 2016/06/27 at 20:37:53, Theresa Wellington wrote: > Is there a way we can get an idea of how many users clear app data vs take one of the other actions? Maybe we can ask UMA folks if there's a way to force an upload before data is cleared? > > If not, we can brainstorm some other options. Since the app reset logic is part of the OS itself, we would probably have to block until the histogram is uploaded. Since we're not directly looking for this metric right now, I'm fine with punting this problem till later.
lgtm https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:244: if (savedInstanceState != null || mImportantDomains == null || mFaviconURLs == null) { Is this going to collide with your other change? https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:216: "Storage.AndroidManage.ActionTaken", CLEAR_APP_DATA_OPTION, nit: add a note that this won't actually get uploaded here too for folks that don't read the histograms.xml string https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:939: <action name="AndroidManageSpace"> nit: Android.ManageSpace? https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13513: <action name="SiteSettings_Storage_ClearAll"> nit: Use MobileWebsiteSettings in the name to match "MobileWebsiteSettingsOpenedFromMenu" and "MobileWebsiteSettingsOpenedFromToolbar"? https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17575: + This is the number sites that we think are important to the user. nit: number of sites https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55125: +<histogram name="Storage.AndroidManage.ActionTaken" nit: Android.ManageSpace.ActionTaken?
https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:354: boolean haveImportantSites = this looks like it reversed the meaning of this function...shouldn't this be this now? mSortedImportantDomains != null && mSortedImportantDomains.length > 0
https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:354: boolean haveImportantSites = On 2016/06/29 15:59:14, Ted C wrote: > this looks like it reversed the meaning of this function...shouldn't this be > this now? > > mSortedImportantDomains != null && mSortedImportantDomains.length > 0 Ah, yes.. seems so to me as well. Is there a test that catches this? If not, I think there should be.
https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:354: boolean haveImportantSites = On 2016/06/29 at 18:25:30, Theresa Wellington wrote: > On 2016/06/29 15:59:14, Ted C wrote: > > this looks like it reversed the meaning of this function...shouldn't this be > > this now? > > > > mSortedImportantDomains != null && mSortedImportantDomains.length > 0 > > Ah, yes.. seems so to me as well. Is there a test that catches this? If not, I think there should be. Thanks! Yes there is, I just didn't run tests, my bad. https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:244: if (savedInstanceState != null || mImportantDomains == null || mFaviconURLs == null) { On 2016/06/29 at 01:24:45, Theresa Wellington wrote: > Is this going to collide with your other change? Ah, yeah, removing https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/2096363003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:216: "Storage.AndroidManage.ActionTaken", CLEAR_APP_DATA_OPTION, On 2016/06/29 at 01:24:45, Theresa Wellington wrote: > nit: add a note that this won't actually get uploaded here too for folks that don't read the histograms.xml string Done. https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:939: <action name="AndroidManageSpace"> On 2016/06/29 at 01:24:45, Theresa Wellington wrote: > nit: Android.ManageSpace? Done. https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13513: <action name="SiteSettings_Storage_ClearAll"> On 2016/06/29 at 01:24:45, Theresa Wellington wrote: > nit: Use MobileWebsiteSettings in the name to match "MobileWebsiteSettingsOpenedFromMenu" and "MobileWebsiteSettingsOpenedFromToolbar"? Done. https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17575: + This is the number sites that we think are important to the user. On 2016/06/29 at 01:24:46, Theresa Wellington wrote: > nit: number of sites Done. https://codereview.chromium.org/2096363003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55125: +<histogram name="Storage.AndroidManage.ActionTaken" On 2016/06/29 at 01:24:46, Theresa Wellington wrote: > nit: Android.ManageSpace.ActionTaken? Done.
lgtm w/ naming comment https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:327: RecordUserAction.record("MobileWebsiteSettingsStorageClearAll"); Sorry to contradict twellington@, but WebsiteSettings is a somewhat unfortunate name that refers to the page info dialog (i.e. if you click the lock icon in the omnibox or the (i) icon in the overview). Is that triggered from here or just general settings? You might be paving the way for general settings stuff, so maybe just MobileSettings instead?
https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:357: "History.ClearBrowsingData.ImportantSitesPresent", haveImportantSites); You forgot to add this to histograms.xml. https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:406: "History.ClearBrowsingData.OnCreate", savedInstanceState == null); You forgot to add this to histograms.xml. https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:579: mSortedImportantDomains.length, 0, mMaxImportantSites + 1, mMaxImportantSites + 1); I do not think this will work if mMaxImportantSites changes in the course of a single Chrome session. What I think happens is that during the first call you end up creating the histogram and allocating the buckets, and then later calls cannot expand the histogram size. Can you make mMaxImportantSites a constant? ditto below https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:51: // Do not change these constants, they are used with UMA histograms. (except for the MAX entry) https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:54: private static final int CLEAR_APP_DATA_OPTION = 2; optional nit: consider starting all the options with OPTION_ rather than putting it as a suffix. https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:55: private static final int OPTION_BOUNDARY = 3; please use MAX OR NUM_ENTRIES or something; BOUNDARY is not a typical. https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:906: This is recorded when the user navigates to 'Manage Space' from the Android nit: "This is recorded when the user navigates" -> "User navigated" https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:907: settings for Chrome. Is this the app information panel in the "Apps" section of Android settings? If so, can you add that as another sentence there for clarity. I'm asking for this clarification because I cannot find a 'Manage Space' section in any settings menu on my devices. Is this a new feature? https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9345: Recorded when the user clicks on the 'Clear All' button in the Storage "Recorded when the user clicks" -> "User clicked" or even simply "Click" https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17584: + Recorded when we fetch important sites in the clear browsing data screen. "we fetch" should be past tense because it's only recorded after they're fetched (and you know how many there are) "we've successfully fetched" for example (especially if fetching can fail) https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17595: + clearing. perhaps add: The default has all sites selected. https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17595: + clearing. Also, how useful is this without knowing the number of sites still selected? Maybe we should figure out how to record the pair (num selected, num deselected) or their ratio or something. Think about what question you're trying to answer by measuring this.
https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:357: "History.ClearBrowsingData.ImportantSitesPresent", haveImportantSites); On 2016/06/29 at 20:20:31, Mark P wrote: > You forgot to add this to histograms.xml. Oops, it was supposed to be ImportantDialogShown. https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:406: "History.ClearBrowsingData.OnCreate", savedInstanceState == null); On 2016/06/29 at 20:20:31, Mark P wrote: > You forgot to add this to histograms.xml. Made it an action instead. https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:579: mSortedImportantDomains.length, 0, mMaxImportantSites + 1, mMaxImportantSites + 1); On 2016/06/29 at 20:20:31, Mark P wrote: > I do not think this will work if mMaxImportantSites changes in the course of a single Chrome session. What I think happens is that during the first call you end up creating the histogram and allocating the buckets, and then later calls cannot expand the histogram size. > > Can you make mMaxImportantSites a constant? > > ditto below It's actually a constant, we just have the plumbing to only store it in one place. I'll add some comments about that, is that alright? https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:51: // Do not change these constants, they are used with UMA histograms. On 2016/06/29 at 20:20:32, Mark P wrote: > (except for the MAX entry) Done. https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:54: private static final int CLEAR_APP_DATA_OPTION = 2; On 2016/06/29 at 20:20:32, Mark P wrote: > optional nit: consider starting all the options with OPTION_ rather than putting it as a suffix. Done. https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:55: private static final int OPTION_BOUNDARY = 3; On 2016/06/29 at 20:20:32, Mark P wrote: > please use MAX OR NUM_ENTRIES or something; BOUNDARY is not a typical. Done. https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:327: RecordUserAction.record("MobileWebsiteSettingsStorageClearAll"); On 2016/06/29 at 19:49:42, Ted C wrote: > Sorry to contradict twellington@, but WebsiteSettings is a somewhat unfortunate name that refers to the page info dialog (i.e. if you click the lock icon in the omnibox or the (i) icon in the overview). > > Is that triggered from here or just general settings? > > You might be paving the way for general settings stuff, so maybe just MobileSettings instead? Done. https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:906: This is recorded when the user navigates to 'Manage Space' from the Android On 2016/06/29 at 20:20:32, Mark P wrote: > nit: "This is recorded when the user navigates" -> "User navigated" Done. https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:907: settings for Chrome. On 2016/06/29 at 20:20:32, Mark P wrote: > Is this the app information panel in the "Apps" section of Android settings? If so, can you add that as another sentence there for clarity. > > I'm asking for this clarification because I cannot find a 'Manage Space' section in any settings menu on my devices. Is this a new feature? Yes, it's a new feature. Fixed. https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9345: Recorded when the user clicks on the 'Clear All' button in the Storage On 2016/06/29 at 20:20:32, Mark P wrote: > "Recorded when the user clicks" -> "User clicked" or even simply "Click" Done. https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17584: + Recorded when we fetch important sites in the clear browsing data screen. On 2016/06/29 at 20:20:32, Mark P wrote: > "we fetch" should be past tense because it's only recorded after they're fetched (and you know how many there are) > "we've successfully fetched" for example (especially if fetching can fail) Done. https://codereview.chromium.org/2096363003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17595: + clearing. On 2016/06/29 at 20:20:32, Mark P wrote: > perhaps add: > The default has all sites selected. Done, added another histogram about this. Forgot we also wanted percentage.
https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:579: mSortedImportantDomains.length, 0, mMaxImportantSites + 1, mMaxImportantSites + 1); On 2016/06/29 21:56:06, dmurph wrote: > On 2016/06/29 at 20:20:31, Mark P wrote: > > I do not think this will work if mMaxImportantSites changes in the course of a > single Chrome session. What I think happens is that during the first call you > end up creating the histogram and allocating the buckets, and then later calls > cannot expand the histogram size. > > > > Can you make mMaxImportantSites a constant? > > > > ditto below > > It's actually a constant, we just have the plumbing to only store it in one > place. I'll add some comments about that, is that alright? Looks fine to me now. https://codereview.chromium.org/2096363003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:604: deselectedDomains.length * 100 / mSortedImportantDomains.length); I'm a little confused by what you're doing here. Why are you dividing by mSortedImportantDomains? The code and description earlier implies that the number of sites you showed the user is at most mMaxImportantSites == 3, or more precisely domains.length. Am I wrong? https://codereview.chromium.org/2096363003/diff/100001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2096363003/diff/100001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:75: // This should never change, as it's used for UMA metrics. You don't need this warning. The problem I envisioned had to do with it changing during a single session. If it changes between sessions (say, during a binary update), that's fine. https://codereview.chromium.org/2096363003/diff/100001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:688: // This should never return a different constant, as it's used for UMA metrics. more correct: // This value should no change during a sessions, as it's used for UMA metrics. https://codereview.chromium.org/2096363003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096363003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:17582: +<histogram name="History.ClearBrowsingData.ImportantDeselectedPercent"> units="%"? https://codereview.chromium.org/2096363003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:17597: + dialog. We record 'true' when the important sites dialog is shown. when -> if ? (i.e., the clear browsing dialog might not have an important site dialog within it. Is that true?)
https://codereview.chromium.org/2096363003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2096363003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:604: deselectedDomains.length * 100 / mSortedImportantDomains.length); On 2016/06/30 at 05:29:09, Mark P wrote: > I'm a little confused by what you're doing here. Why are you dividing by mSortedImportantDomains? The code and description earlier implies that the number of sites you showed the user is at most mMaxImportantSites == 3, or more precisely domains.length. Am I wrong? Here we want to figure out the percent of sites that were deselected, based on the number we showed to the user (as we could be showing less than the max). https://codereview.chromium.org/2096363003/diff/100001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2096363003/diff/100001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:75: // This should never change, as it's used for UMA metrics. On 2016/06/30 at 05:29:09, Mark P wrote: > You don't need this warning. The problem I envisioned had to do with it changing during a single session. If it changes between sessions (say, during a binary update), that's fine. Ah, ok. https://codereview.chromium.org/2096363003/diff/100001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:688: // This should never return a different constant, as it's used for UMA metrics. On 2016/06/30 at 05:29:09, Mark P wrote: > more correct: > // This value should no change during a sessions, as it's used for UMA metrics. Done. https://codereview.chromium.org/2096363003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096363003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:17582: +<histogram name="History.ClearBrowsingData.ImportantDeselectedPercent"> On 2016/06/30 at 05:29:09, Mark P wrote: > units="%"? Ah, right, thanks. https://codereview.chromium.org/2096363003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:17597: + dialog. We record 'true' when the important sites dialog is shown. On 2016/06/30 at 05:29:09, Mark P wrote: > when -> if > ? > > (i.e., the clear browsing dialog might not have an important site dialog within it. Is that true?) That sounds better, thanks.
made the change to the percent part, so that it's out of 20 instead.
metrics lgtm
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2096363003/#ps140001 (title: "Switched percent to be out of 20")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [ImportantSites] Adding usage metrics. BUG=622468,605761 ========== to ========== [ImportantSites] Adding usage metrics. BUG=622468,605761 Committed: https://crrev.com/357700a996c2e31ad34d6498d68ef0da0b6474c0 Cr-Commit-Position: refs/heads/master@{#403317} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/357700a996c2e31ad34d6498d68ef0da0b6474c0 Cr-Commit-Position: refs/heads/master@{#403317} |
