Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843693002:
Add UMA metrics regarding default browser status (Closed)
|
Created:
3 years, 8 months ago by David Trainor- moved to gerrit Modified:
3 years, 7 months ago CC:
chromium-reviews, gayane+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA metrics regarding default browser status
In order to better understand Chrome usage, add metrics that help us
figure out if Chrome is the default browser on the system or not.
BUG=704223
Review-Url: https://codereview.chromium.org/2843693002
Cr-Commit-Position: refs/heads/master@{#469204}
Committed: https://chromium.googlesource.com/chromium/src/+/63ac5c77d19bc3a7da903ff1bd448890f40a5a3b
Patch Set 1 #
Total comments: 30
Patch Set 2 : Cleanup of the CL #Patch Set 3 : Added final and comments #
Total comments: 5
Patch Set 4 : Addressed comments #
Total comments: 6
Patch Set 5 : Rebased #
Depends on Patchset: Messages
Total messages: 19 (10 generated)
dtrainor@chromium.org changed reviewers: + nyquist@chromium.org
fyi. haven't tested this yet.
I know you haven't tested this, etc., yet, but I just wanted to say that I like the structure of the CL, with the data object, the AsyncTask, etc. I think it makes total sense! Since I was already looking, I had a few notes on a couple of things that wasn't 100% clear to me. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:33: public class DefaultBrowserInfo { Nit: This only has static members, should we make the class final in addition to the private constructor? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:40: private static final int DEFAULT_STATE_CHROME_DEFAULT_ONLY_BROWSERS = 0; Nit: "...DEFAULT_ONLY_BROWSER"? (remove S?) https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:55: public boolean isDefault; Nit: I wonder if it would be clearer to name this isChromeDefault, and isChromeSystem below. WDYT? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:57: public boolean isSystemDefault; A super tiny nit here: Since there might be (I think?) multiple system browsers, should this be named: "isDefaultSystem" instead? It feels like the "default" part is the important part. With the current ordering I tend to read it as "is the one and only system-browser in fact the default". WDYT? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:164: List<ResolveInfo> handlers = pm.queryIntentActivities(intent, 0); Could one app have multiple activities that can view the intent? If so, does that affect the values? Like; FooBrowser has FooActivity and BarActivity that can both open the URL. Would that for example make it so that you can't just set browserCount to handlers.size()? I guess we could have a unique Set<String> of package names, and use the resulting size of that as the browserCount? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:168: boolean isSelf = TextUtils.equals(context.getPackageName(), thisPackage); I'm not sure when this would ever be false? Did you mean handler.activityInfo.packageName or something along those lines instead of context.getPackageName()? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:176: return null; Nit: This makes us ignore all the results. I think that is fine, but maybe add a short comment to make the reader aware of this being fully intentional? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:182: } else if (isDefault && isSystem) { Does the |else if| mean that isSystemDefault will never be set for Chrome? If so, is that intentional? I could totally see us logging that as a different scenario, since we're already tracking isDefault and isSystem in a separate, but I just wanted to check if it was intentional. If it's intentional, maybe we could simplify the last part with just setting the value to true, since we're checking isDefault && isSystem in the else-if clause? And if it wasn't intentional, maybe we could simplify the last part a little bit? Maybe something like: ### if (isSelf) { ... what you have now ... } if (isDefault) { info.hasDefault = true; info.isSystemDefault = isSystem; } ### WDYT? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:196: "Mobile.DefaultBrowser.BrowserCount", info.browserCount); Do you ever set browserCount to something else than 0? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:207: if (info.isDefault && info.browserCount == 1) { Nit: What you have is fine, but did you look into whether it's easier to read if you do an outer if-statement like: if (info.isDefault) { if (info.browserCount == 1) { ... } else if ( ... ) ... ... } else { ... } I tried it out locally, and I don't think it looked horrible. I don't feel strongly though. But for myself, it did make it clearer that we might have a couple of cases that are harder to reach than others: (info.isDefault && !info.isSystem) and (!info.isDefault && !info.hasDefault). https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:212: defaultState = DEFAULT_STATE_CHROME_DEFAULT_NOT_SYSTEM; I wonder if Chrome is default there will be at least one browser, so one of the two states above would have probably hit? I.e. maybe this one is unreachable in practice if info.isDefault is true? Also, I'm not sure what we in fact want to report at that point? I mean, these cases are kind of overlapping. Maybe we should discuss this offline? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:215: } else if (!info.isDefault && !info.isSystemDefault) { If we're in the state where "!isDefault", would either this case or the previous case always hit? I.e. would the last !info.hasDefault check below unreachable? https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:217: } else if (!info.isDefault && !info.hasDefault) { Nit: I wonder; would this be clearer if we extracted this out into its own private static method? I mean, the whole: int defaultState = getDefaultState(info); That way you could have if (!info.hasDefault()) as an early return, and the DEFAULT_STATE_OTHER as the fallback last return? Also, if you end up doing the if-else for the isDefault, the else-clause also becomes a pretty if (isSystemDefault) {...} else {...}. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:224: .executeOnExecutor( If you feel like this indent is weird, follow along on https://bugs.chromium.org/p/chromium/issues/detail?id=710441 or https://bugs.llvm.org/show_bug.cgi?id=32627 :-) https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:226: } catch (RejectedExecutionException ex) { Oooh... nice! Thanks!
The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run
ptal thanks! https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:33: public class DefaultBrowserInfo { On 2017/04/25 06:17:51, nyquist wrote: > Nit: This only has static members, should we make the class final in addition to > the private constructor? IIUC you can't override if the constructor is private, but final is a nice touch anyway. Will add! https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:40: private static final int DEFAULT_STATE_CHROME_DEFAULT_ONLY_BROWSERS = 0; On 2017/04/25 06:17:51, nyquist wrote: > Nit: "...DEFAULT_ONLY_BROWSER"? (remove S?) Acknowledged. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:55: public boolean isDefault; On 2017/04/25 06:17:51, nyquist wrote: > Nit: I wonder if it would be clearer to name this isChromeDefault, and > isChromeSystem below. WDYT? Done. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:57: public boolean isSystemDefault; On 2017/04/25 06:17:52, nyquist wrote: > A super tiny nit here: Since there might be (I think?) multiple system browsers, > should this be named: "isDefaultSystem" instead? It feels like the "default" > part is the important part. With the current ordering I tend to read it as "is > the one and only system-browser in fact the default". WDYT? Done. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:164: List<ResolveInfo> handlers = pm.queryIntentActivities(intent, 0); On 2017/04/25 06:17:51, nyquist wrote: > Could one app have multiple activities that can view the intent? If so, does > that affect the values? Like; FooBrowser has FooActivity and BarActivity that > can both open the URL. Would that for example make it so that you can't just set > browserCount to handlers.size()? > > I guess we could have a unique Set<String> of package names, and use the > resulting size of that as the browserCount? Done. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:168: boolean isSelf = TextUtils.equals(context.getPackageName(), thisPackage); On 2017/04/25 06:17:51, nyquist wrote: > I'm not sure when this would ever be false? > Did you mean handler.activityInfo.packageName or something along those lines > instead of context.getPackageName()? Done. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:176: return null; On 2017/04/25 06:17:51, nyquist wrote: > Nit: This makes us ignore all the results. I think that is fine, but maybe add a > short comment to make the reader aware of this being fully intentional? Done. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:182: } else if (isDefault && isSystem) { On 2017/04/25 06:17:52, nyquist wrote: > Does the |else if| mean that isSystemDefault will never be set for Chrome? > If so, is that intentional? I could totally see us logging that as a different > scenario, since we're already tracking isDefault and isSystem in a separate, but > I just wanted to check if it was intentional. > > If it's intentional, maybe we could simplify the last part with just setting the > value to true, since we're checking isDefault && isSystem in the else-if clause? > > > And if it wasn't intentional, maybe we could simplify the last part a little > bit? Maybe something like: > > ### > if (isSelf) { > ... what you have now ... > } > > if (isDefault) { > info.hasDefault = true; > info.isSystemDefault = isSystem; > } > ### > > WDYT? Done. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:196: "Mobile.DefaultBrowser.BrowserCount", info.browserCount); On 2017/04/25 06:17:52, nyquist wrote: > Do you ever set browserCount to something else than 0? Done. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:207: if (info.isDefault && info.browserCount == 1) { On 2017/04/25 06:17:52, nyquist wrote: > Nit: What you have is fine, but did you look into whether it's easier to read if > you do an outer if-statement like: > > if (info.isDefault) { > if (info.browserCount == 1) { ... } > else if ( ... ) ... > ... > } else { > ... > } > > I tried it out locally, and I don't think it looked horrible. I don't feel > strongly though. > But for myself, it did make it clearer that we might have a couple of cases that > are harder to reach than others: (info.isDefault && !info.isSystem) and > (!info.isDefault && !info.hasDefault). Done. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:212: defaultState = DEFAULT_STATE_CHROME_DEFAULT_NOT_SYSTEM; On 2017/04/25 06:17:52, nyquist wrote: > I wonder if Chrome is default there will be at least one browser, so one of the > two states above would have probably hit? I.e. maybe this one is unreachable in > practice if info.isDefault is true? > > Also, I'm not sure what we in fact want to report at that point? I mean, these > cases are kind of overlapping. Maybe we should discuss this offline? Acknowledged. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:215: } else if (!info.isDefault && !info.isSystemDefault) { On 2017/04/25 06:17:51, nyquist wrote: > If we're in the state where "!isDefault", would either this case or the previous > case always hit? I.e. would the last !info.hasDefault check below unreachable? Acknowledged. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:217: } else if (!info.isDefault && !info.hasDefault) { On 2017/04/25 06:17:51, nyquist wrote: > Nit: I wonder; would this be clearer if we extracted this out into its own > private static method? I mean, the whole: > int defaultState = getDefaultState(info); > > That way you could have if (!info.hasDefault()) as an early return, and the > DEFAULT_STATE_OTHER as the fallback last return? > Also, if you end up doing the if-else for the isDefault, the else-clause also > becomes a pretty if (isSystemDefault) {...} else {...}. Acknowledged. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:224: .executeOnExecutor( On 2017/04/25 06:17:51, nyquist wrote: > If you feel like this indent is weird, follow along on > https://bugs.chromium.org/p/chromium/issues/detail?id=710441 or > https://bugs.llvm.org/show_bug.cgi?id=32627 :-) Acknowledged. https://codereview.chromium.org/2843693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:226: } catch (RejectedExecutionException ex) { On 2017/04/25 06:17:52, nyquist wrote: > Oooh... nice! Thanks! Acknowledged.
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...)
lgtm https://codereview.chromium.org/2843693002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2843693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:52: int NO_DEFAULT = 0; clang-format!! y u no make sense!? https://codereview.chromium.org/2843693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:186: if (isSamePackage(context, ri)) info.isChromeSystem = isSystemPackage(ri); Optional nit: Is isChromeSystem really necessary? Or could we use info.isDefaultSystem on line 222 below instead? We seem to only check isChromeSystem if Chrome is already the default browser. That being said though, I really don't feel strongly about this, and I think the current version is also very easy to read below, but you can remove this line if you remove the isChromeSystem member, making this loop incredibly short and to the point. https://codereview.chromium.org/2843693002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2843693002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:30351: + Android: Whether or not the device has a default browser and whether or Nit: Is rietveld making fun of me, os is there a triple space after the last |whether|?
dtrainor@chromium.org changed reviewers: + isherman@chromium.org
isherman@ ptal at histograms.xml. Thanks! https://codereview.chromium.org/2843693002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2843693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:186: if (isSamePackage(context, ri)) info.isChromeSystem = isSystemPackage(ri); On 2017/04/27 05:20:06, nyquist (nychthemeron ping) wrote: > Optional nit: Is isChromeSystem really necessary? > Or could we use info.isDefaultSystem on line 222 below instead? We seem to only > check isChromeSystem if Chrome is already the default browser. > > That being said though, I really don't feel strongly about this, and I think the > current version is also very easy to read below, but you can remove this line if > you remove the isChromeSystem member, making this loop incredibly short and to > the point. Fair point! https://codereview.chromium.org/2843693002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2843693002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:30351: + Android: Whether or not the device has a default browser and whether or On 2017/04/27 05:20:06, nyquist (nychthemeron ping) wrote: > Nit: Is rietveld making fun of me, os is there a triple space after the last > |whether|? ... secret.
Metrics LGTM % comments: https://codereview.chromium.org/2843693002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2843693002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:40: * MobileDefaultBrowserState in histograms.xml. Please also document that this enum should be treated as append-only. https://codereview.chromium.org/2843693002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:57: int BOUNDARY = 5; nit: Is it correct that these lines are not indented? It looks surprising to me... https://codereview.chromium.org/2843693002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2843693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:30333: +<histogram name="Mobile.DefaultBrowser.BrowserCount.ChromeDefault"> Please use a histogram_suffixes element to avoid repeating the docs for this histogram; and please add a brief label for each suffix, so that it's extra clear what the histogram is measuring.
https://codereview.chromium.org/2843693002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2843693002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:40: * MobileDefaultBrowserState in histograms.xml. On 2017/04/28 21:14:30, Ilya Sherman wrote: > Please also document that this enum should be treated as append-only. Done. https://codereview.chromium.org/2843693002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:57: int BOUNDARY = 5; On 2017/04/28 21:14:30, Ilya Sherman wrote: > nit: Is it correct that these lines are not indented? It looks surprising to > me... Presubmit and git cl format are fighting. This was the one that passes the checks :(. https://codereview.chromium.org/2843693002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2843693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:30333: +<histogram name="Mobile.DefaultBrowser.BrowserCount.ChromeDefault"> On 2017/04/28 21:14:30, Ilya Sherman wrote: > Please use a histogram_suffixes element to avoid repeating the docs for this > histogram; and please add a brief label for each suffix, so that it's extra > clear what the histogram is measuring. Done.
The CQ bit was checked by dtrainor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2843693002/#ps80001 (title: "Rebased")
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": 80001, "attempt_start_ts": 1493848277812980,
"parent_rev": "9d0f65d61b81fdce1e9a6ffd581cb9e69a429d81", "commit_rev":
"63ac5c77d19bc3a7da903ff1bd448890f40a5a3b"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA metrics regarding default browser status In order to better understand Chrome usage, add metrics that help us figure out if Chrome is the default browser on the system or not. BUG=704223 ========== to ========== Add UMA metrics regarding default browser status In order to better understand Chrome usage, add metrics that help us figure out if Chrome is the default browser on the system or not. BUG=704223 Review-Url: https://codereview.chromium.org/2843693002 Cr-Commit-Position: refs/heads/master@{#469204} Committed: https://chromium.googlesource.com/chromium/src/+/63ac5c77d19bc3a7da903ff1bd44... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/63ac5c77d19bc3a7da903ff1bd44... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
