Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2110463004:
[ImportantStorage] Blacklisted reason metrics (Closed)
Description[ImportantStorage] Blacklisted reason metrics
R=tedchoc@chromium.org,twellington@chromium.org
BUG=622468, 605761
Committed: https://crrev.com/ec925b03671a9d153fcda20d6a951869b0c24b3e
Cr-Commit-Position: refs/heads/master@{#404226}
Patch Set 1 #
Total comments: 8
Patch Set 2 : build, and histograms #
Total comments: 6
Patch Set 3 : comments #
Total comments: 1
Patch Set 4 : histograms update #Patch Set 5 : Fixed domain empty check #
Messages
Total messages: 25 (8 generated)
Hello, these are more metrics for important storage. Can you PTAL? Thanks, Daniel
https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/important_sites_util.cc:211: for (const std::string& blacklisted_site : blacklisted_sites) { would it not be possible to just gather the content settings for the particular blacklisted sites? Then loop over the black listed sites instead of the content setting for all sites? I would expect the blacklisted list to be quite small and the content settings "might" not be. https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/important_sites_util.cc:222: UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason", i'm slightly worried that this won't scale as we add different options. We don't want to have an explosion of combinations for each new addition.
https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/important_sites_util.cc:211: for (const std::string& blacklisted_site : blacklisted_sites) { On 2016/06/29 at 16:14:54, Ted C wrote: > would it not be possible to just gather the content settings for the particular blacklisted sites? Then loop over the black listed sites instead of the content setting for all sites? > > I would expect the blacklisted list to be quite small and the content settings "might" not be. The main issue here is that we don't have GURLs to give it. We just have registerable domains, which is eTLD+1. We can't really look up content settings on that large of a granularity. https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/important_sites_util.cc:222: UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason", On 2016/06/29 at 16:14:54, Ted C wrote: > i'm slightly worried that this won't scale as we add different options. We don't want to have an explosion of combinations for each new addition. Initially, we want all of this data to determine which signal was better. We're not going to want a huge cross of everything if we add more sources, this is purely for this initial dialog experiment. I can add a comment about that if that helps.
https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/important_sites_util.cc:211: for (const std::string& blacklisted_site : blacklisted_sites) { On 2016/06/29 19:20:59, dmurph wrote: > On 2016/06/29 at 16:14:54, Ted C wrote: > > would it not be possible to just gather the content settings for the > particular blacklisted sites? Then loop over the black listed sites instead of > the content setting for all sites? > > > > I would expect the blacklisted list to be quite small and the content settings > "might" not be. > > The main issue here is that we don't have GURLs to give it. We just have > registerable domains, which is eTLD+1. We can't really look up content settings > on that large of a granularity. Hmm...that is unfortunate. We actually have all of this information when we build the list of important sites right? I wonder if we should be pushing all this information up from that point so we wouldn't need to do this recalculation. Does that seem feasible? If not, I can approve this change but it just seems...unfortunate. https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/important_sites_util.cc:222: UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason", On 2016/06/29 19:20:59, dmurph wrote: > On 2016/06/29 at 16:14:54, Ted C wrote: > > i'm slightly worried that this won't scale as we add different options. We > don't want to have an explosion of combinations for each new addition. > > Initially, we want all of this data to determine which signal was better. We're > not going to want a huge cross of everything if we add more sources, this is > purely for this initial dialog experiment. I can add a comment about that if > that helps. Ack...no comment needed
https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/important_sites_util.cc:211: for (const std::string& blacklisted_site : blacklisted_sites) { On 2016/06/29 at 19:44:19, Ted C wrote: > On 2016/06/29 19:20:59, dmurph wrote: > > On 2016/06/29 at 16:14:54, Ted C wrote: > > > would it not be possible to just gather the content settings for the > > particular blacklisted sites? Then loop over the black listed sites instead of > > the content setting for all sites? > > > > > > I would expect the blacklisted list to be quite small and the content settings > > "might" not be. > > > > The main issue here is that we don't have GURLs to give it. We just have > > registerable domains, which is eTLD+1. We can't really look up content settings > > on that large of a granularity. > > Hmm...that is unfortunate. We actually have all of this information when we build the list of important sites right? I wonder if we should be pushing all this information up from that point so we wouldn't need to do this recalculation. > > Does that seem feasible? If not, I can approve this change but it just seems...unfortunate. Unfortunately this seems really complicated, we would have to somehow hold onto a handle in java to a c++ object... or maybe an integer that's used as a bitmask? It seems fragile and complicated, so even though this is terrible (and I know it), it's the least-bad in my mind. On the bright side, if we go forward with important sites being a thing, we can rework how we record stats by saving these things instead of generating them on the fly.
lgtm https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/2110463004/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/important_sites_util.cc:211: for (const std::string& blacklisted_site : blacklisted_sites) { On 2016/06/29 21:59:26, dmurph wrote: > On 2016/06/29 at 19:44:19, Ted C wrote: > > On 2016/06/29 19:20:59, dmurph wrote: > > > On 2016/06/29 at 16:14:54, Ted C wrote: > > > > would it not be possible to just gather the content settings for the > > > particular blacklisted sites? Then loop over the black listed sites instead > of > > > the content setting for all sites? > > > > > > > > I would expect the blacklisted list to be quite small and the content > settings > > > "might" not be. > > > > > > The main issue here is that we don't have GURLs to give it. We just have > > > registerable domains, which is eTLD+1. We can't really look up content > settings > > > on that large of a granularity. > > > > Hmm...that is unfortunate. We actually have all of this information when we > build the list of important sites right? I wonder if we should be pushing all > this information up from that point so we wouldn't need to do this > recalculation. > > > > Does that seem feasible? If not, I can approve this change but it just > seems...unfortunate. > > Unfortunately this seems really complicated, we would have to somehow hold onto > a handle in java to a c++ object... or maybe an integer that's used as a > bitmask? It seems fragile and complicated, so even though this is terrible (and > I know it), it's the least-bad in my mind. I think the bitmask would be the most reasonable approach. I wouldn't necessarily hold onto a C++ object from java, but you'd end up passing a list of strings and a corresponding list of ints up to java in your call above. But I'm fine punting on that for now as I don't think it is as complicated as you think, it is still complicated and a bunch of plumbing. > > On the bright side, if we go forward with important sites being a thing, we can > rework how we record stats by saving these things instead of generating them on > the fly.
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
dmurph@chromium.org changed reviewers: + mpearson@chromium.org
+Mark for histograms.xml change
https://codereview.chromium.org/2110463004/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/2110463004/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util.cc:22: enum ReasonStatTypes { Insert standard warning here. https://codereview.chromium.org/2110463004/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util.cc:246: } Why is none of the above not a possibility? https://codereview.chromium.org/2110463004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2110463004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55102: + This is recorded when we blacklist sites from being cleared when the user This first sentence implies that it's only recorded once when the user clears browsing data. Please rephrase.
https://codereview.chromium.org/2110463004/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/2110463004/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util.cc:22: enum ReasonStatTypes { On 2016/07/06 at 19:10:42, Mark P wrote: > Insert standard warning here. Done. https://codereview.chromium.org/2110463004/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util.cc:246: } On 2016/07/06 at 19:10:42, Mark P wrote: > Why is none of the above not a possibility? Added. It shouldn't occur (as we only use the above measurements to determine if a site is important), but I added it for future/ just in case. https://codereview.chromium.org/2110463004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2110463004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55102: + This is recorded when we blacklist sites from being cleared when the user On 2016/07/06 at 19:10:42, Mark P wrote: > This first sentence implies that it's only recorded once when the user clears browsing data. Please rephrase. Done.
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
histograms.xml lgtm https://codereview.chromium.org/2110463004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2110463004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55103: + clear browsing data. It tells us which signals were used to show the given nit: omit "us" optional nit: tells -> indicates
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2110463004/#ps80001 (title: "Fixed domain empty check")
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 #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [ImportantStorage] Blacklisted reason metrics R=tedchoc@chromium.org,twellington@chromium.org BUG=622468,605761 ========== to ========== [ImportantStorage] Blacklisted reason metrics R=tedchoc@chromium.org,twellington@chromium.org BUG=622468,605761 Committed: https://crrev.com/ec925b03671a9d153fcda20d6a951869b0c24b3e Cr-Commit-Position: refs/heads/master@{#404226} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ec925b03671a9d153fcda20d6a951869b0c24b3e Cr-Commit-Position: refs/heads/master@{#404226} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
