Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533523002:
Add Geolocation.PermissionState histogram. (Closed)
DescriptionAdd Geolocation.PermissionState histogram.
The GeolocationHeader.PermissionState histogram counts the location requests
for each combination of Location Source, M+ Chrome App permission and Chrome
domain omnibox permission. For cases where a location could be sent it
further divides the counts into whether the location was sent or not.
BUG=668923
Committed: https://crrev.com/fff2d84dfc1e5b0c38728cf242755e9c2f175013
Cr-Commit-Position: refs/heads/master@{#438329}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add default: to all switches. Fix lines over 100 chars. #
Total comments: 1
Patch Set 3 : Shorten constant names to allow formatting; use concat instead of +. #Patch Set 4 : More formatting fixes. #
Total comments: 7
Patch Set 5 : Change enums to use formula. Update histrograms.xml. #
Total comments: 36
Patch Set 6 : Address comments. Return to listed enums. #
Total comments: 15
Patch Set 7 : Address comments. Change switch/case to if/then/else. #Patch Set 8 : Use ChromeActivityTestCaseBase<ChromeActivity> in tests. #
Total comments: 2
Patch Set 9 : Finer grained control over UI threads in tests. #Patch Set 10 : Add back to histograms.xml lines deleted during merge conflict. #
Total comments: 10
Patch Set 11 : Address comments. Changes are small. #
Total comments: 7
Patch Set 12 : Fix method comments. #
Total comments: 10
Patch Set 13 : Address reviewer comments. #Patch Set 14 : Remove test that requires high accuracy location. #
Messages
Total messages: 77 (49 generated)
Description was changed from ========== Add Geolocation.PermisisonState histogram. The GeolocationHeader.PermissionState histogram counts the location requests for each combination of Location Source, M+ Chrome App permission and Chrome domain omnibox permission. For cases where a location could be sent it further divides the counts into whether the location was sent or not. BUG= ========== to ========== Add Geolocation.PermisisonState histogram. The GeolocationHeader.PermissionState histogram counts the location requests for each combination of Location Source, M+ Chrome App permission and Chrome domain omnibox permission. For cases where a location could be sent it further divides the counts into whether the location was sent or not. BUG= ==========
The CQ bit was checked by pdyson@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...
pdyson@chromium.org changed reviewers: + kcarattini@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2533523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:370: ? UMA_PERMISSION_HIGH_ACCURACY_APP_GRANTED_DOMAIN_GRANTED_LOCATION_ATTACHED "git cl format" created these lines longer than 100 characters. They failed the presubmit check, so I'll change them back and never run "git cl format" again on this cl, I guess.
The CQ bit was checked by pdyson@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2533523002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:374: ? UMA_PERMISSION_HIGH_ACCURACY_APP_GRANTED_DOMAIN_GRANTED_LOCATION_ATTACHED Moved them back here, but now get an error that the indent is wrong. If I correct the indent I get a line too long error.
On 2016/11/25 06:58:28, pdyson wrote: > https://codereview.chromium.org/2533523002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java > (right): > > https://codereview.chromium.org/2533523002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:374: > ? UMA_PERMISSION_HIGH_ACCURACY_APP_GRANTED_DOMAIN_GRANTED_LOCATION_ATTACHED > Moved them back here, but now get an error that the indent is wrong. If I > correct the indent I get a line too long error. Thanks for doing this! You can go ahead and file a bug for this (component should be Internals>Permissions>CrowdConsent, assign to yourself and cc me) and then add it to the BUG= line in the cl description. Kendra
Description was changed from ========== Add Geolocation.PermisisonState histogram. The GeolocationHeader.PermissionState histogram counts the location requests for each combination of Location Source, M+ Chrome App permission and Chrome domain omnibox permission. For cases where a location could be sent it further divides the counts into whether the location was sent or not. BUG= ========== to ========== Add Geolocation.PermissionState histogram. The GeolocationHeader.PermissionState histogram counts the location requests for each combination of Location Source, M+ Chrome App permission and Chrome domain omnibox permission. For cases where a location could be sent it further divides the counts into whether the location was sent or not. BUG=668923 ==========
The CQ bit was checked by pdyson@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...
pdyson@chromium.org changed reviewers: + isherman@chromium.org
Thanks, Paul. https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:47: // Values for the histogram GeolocationHeader.PermissionState. Please document that this enum-like-construct is used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:358: int locationSource, int appPermission, int domainPermission, boolean locationAttached) { This function implementation is pretty hard to follow. Would it be easier to implement this using mathematical operators, rather than nested switch statements? https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:546: "GeolocationHeader.PermissionState", result, UMA_PERM_MAX); Please update histograms.xml to include a reference to this histogram.
The CQ bit was checked by pdyson@chromium.org to run a CQ dry run
https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:47: // Values for the histogram GeolocationHeader.PermissionState. On 2016/11/29 03:16:15, Ilya Sherman wrote: > Please document that this enum-like-construct is used to back an UMA histogram, > and should therefore be treated as append-only. Done. https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:358: int locationSource, int appPermission, int domainPermission, boolean locationAttached) { On 2016/11/29 03:16:15, Ilya Sherman wrote: > This function implementation is pretty hard to follow. Would it be easier to > implement this using mathematical operators, rather than nested switch > statements? I thought it may be a little dangerous to perform mathematical operations to arrive at ENUM values, but it is simpler and as you have suggested it I have done that now. I've added comments to make it clear what is going on. https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:546: "GeolocationHeader.PermissionState", result, UMA_PERM_MAX); On 2016/11/29 03:16:15, Ilya Sherman wrote: > Please update histograms.xml to include a reference to this histogram. Done.
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: This issue passed the CQ dry run.
dominickn@chromium.org changed reviewers: + dominickn@chromium.org
https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:59: public static final int UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_LOCATION = 0; You might want to use @IntDef here to help with the boilerplate checking of these enum-like constants. See PermissionDialogController.java for an example. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:254: WindowAndroid window = new WindowAndroid(context); I'm almost certain that creating a new WindowAndroid here to get the activity is not the right way to do this. A better way would be to modify the two callsites of GeolocationHeader#getGeoHeader to pass a Tab object to this method, and then call tab.getWindowAndroid().getActivity().get(). https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:288: return locationPermission == ContentSetting.BLOCK; Nit: remove the local variable and directly compare the result of the method call against ContentSetting.BLOCK https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:296: static ContentSetting locationPermissionForUrl(Uri uri, boolean isIncognito) { Nit: call this locationContentSettingForUrl https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:336: int locationSource; Nit: this variable is unused. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:347: Uri uri = Uri.parse(url); Nit: remove the local variable and inline Uri.parse(url) into the locationPermissionForUrl call. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:348: final ContentSetting domainPermission = locationPermissionForUrl(uri, isIncognito); Nit: I don't think this needs to be final? https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:350: case BLOCK: I would make BLOCK the default case, in case new ContentSetting types are added in the future. BLOCK default is safest. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:367: int value = 9 * locationSource + 3 * appPermission + domainPermission; This calculation is quite magic and makes me very uneasy. A bigger comment, removing the magic numbers, and even a step by step explanation of how this is calculated is definitely necessary. Even then, it may be better just to use ifs to step our way through; that will probably be more maintainable. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:76: GeolocationTracker.setLocationForTesting(null); This line here erases the mock location set in this test. That may be the source of the flakiness you're seeing.
https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:76: GeolocationTracker.setLocationForTesting(null); On 2016/11/30 00:08:46, dominickn wrote: > This line here erases the mock location set in this test. That may be the source > of the flakiness you're seeing. Though now that I look at it again, it shouldn't really matter by this point. Hmm. Can you link to a trybot where we see that the GPS provider is used, not network?
https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:367: int value = 9 * locationSource + 3 * appPermission + domainPermission; On 2016/11/30 00:08:46, dominickn wrote: > This calculation is quite magic and makes me very uneasy. > > A bigger comment, removing the magic numbers, and even a step by step > explanation of how this is calculated is definitely necessary. Even then, it may > be better just to use ifs to step our way through; that will probably be more > maintainable. FWIW, I agree that it would be best to clarify how these enum constant values are being interpolated. However, if y'all prefer to use if-stmts or switch-stmts, I'm okay with that too, even though I think it's actually more likely to be error-prone due to copy/pasta bugs. Note that if you do use an equation like this one, you probably don't need the list of enum constants at the top of the file. https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19024: +<histogram name="GeolocationHeader.PermissionState"> Please add an enum attribute for this histogram. https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19024: +<histogram name="GeolocationHeader.PermissionState"> nit: Please group this with the existing Geolocation histograms, by inserting a dot between "Geolocation" and "Header" (both here and in the Java code that records this histogram). https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19025: + <owner>mvanouwerkerk@chromium.org</owner> Hmm, how did you choose Michael to be an owner for this histogram? I notice that he's not even reviewing this CL. Is that intentional, or copy/pasta? https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19027: + Counts Geolocation requests for each combination of location source, M+ The phrase "M+ Chrome permission" is fairly confusing IMO -- I think "M+" refers to Android versions, but that's not super obvious from the context here. I'd recommend either being more descriptive, or less -- the enum labels should clarify what the exact breakdown is. The summary is more intended to convey what the overall metric's purpose is, and the precise conditions under which it's recorded. https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19030: + was sent or not. Hmm, what does it mean to divide cases "where a location could not be sent" into "whether the location was sent"? Should one of those phrases be something else? Or am I misreading this somehow?
mvanouwerkerk@ -- I am adding a new Geolocation UMA histogram (and will be adding a few more in another cl) as a starter project in Chrome. Who is best to be the owner of the histogram? I'm asking you because you are the owner of the previous Geolocation histograms. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:59: public static final int UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_LOCATION = 0; On 2016/11/30 00:08:45, dominickn wrote: > You might want to use @IntDef here to help with the boilerplate checking of > these enum-like constants. See PermissionDialogController.java for an example. Done. I've added this here and also to LOCATION_SOURCE_* and PERMISSION_*. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:254: WindowAndroid window = new WindowAndroid(context); On 2016/11/30 at 00:08:46, dominickn wrote: > I'm almost certain that creating a new WindowAndroid here to get the activity is not the right way to do this. > > A better way would be to modify the two callsites of GeolocationHeader#getGeoHeader to pass a Tab object to this method, and then call tab.getWindowAndroid().getActivity().get(). I have made this change, but now I need to mock a Tab for testing. I've had a look around but I haven't found how to do that. Can you point me to an example? https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:288: return locationPermission == ContentSetting.BLOCK; On 2016/11/30 00:08:45, dominickn wrote: > Nit: remove the local variable and directly compare the result of the method > call against ContentSetting.BLOCK Done. Still getting the hang of how much to put on one line in Java. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:296: static ContentSetting locationPermissionForUrl(Uri uri, boolean isIncognito) { On 2016/11/30 00:08:45, dominickn wrote: > Nit: call this locationContentSettingForUrl Done. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:336: int locationSource; On 2016/11/30 00:08:45, dominickn wrote: > Nit: this variable is unused. Done. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:347: Uri uri = Uri.parse(url); On 2016/11/30 00:08:45, dominickn wrote: > Nit: remove the local variable and inline Uri.parse(url) into the > locationPermissionForUrl call. Done. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:348: final ContentSetting domainPermission = locationPermissionForUrl(uri, isIncognito); On 2016/11/30 00:08:45, dominickn wrote: > Nit: I don't think this needs to be final? Done. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:350: case BLOCK: On 2016/11/30 00:08:45, dominickn wrote: > I would make BLOCK the default case, in case new ContentSetting types are added > in the future. BLOCK default is safest. Done. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:367: int value = 9 * locationSource + 3 * appPermission + domainPermission; On 2016/11/30 00:33:35, Ilya Sherman wrote: > On 2016/11/30 00:08:46, dominickn wrote: > > This calculation is quite magic and makes me very uneasy. > > > > A bigger comment, removing the magic numbers, and even a step by step > > explanation of how this is calculated is definitely necessary. Even then, it > may > > be better just to use ifs to step our way through; that will probably be more > > maintainable. > > FWIW, I agree that it would be best to clarify how these enum constant values > are being interpolated. However, if y'all prefer to use if-stmts or > switch-stmts, I'm okay with that too, even though I think it's actually more > likely to be error-prone due to copy/pasta bugs. > > Note that if you do use an equation like this one, you probably don't need the > list of enum constants at the top of the file. I've put this back to using the switch statements. It's long and ugly, but I'd rather be more explicit. Is there a nicer way of doing these switches? https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:76: GeolocationTracker.setLocationForTesting(null); On 2016/11/30 00:21:06, dominickn wrote: > On 2016/11/30 00:08:46, dominickn wrote: > > This line here erases the mock location set in this test. That may be the > source > > of the flakiness you're seeing. > > Though now that I look at it again, it shouldn't really matter by this point. > Hmm. Can you link to a trybot where we see that the GPS provider is used, not > network? I got the error with patch set 3 running linux_android_rel_ng. When I ran with patch set 5 linux_android_rel_ng failed but did not seem to run this test. patch set 3: https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... patch set 5: https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19024: +<histogram name="GeolocationHeader.PermissionState"> On 2016/11/30 00:33:35, Ilya Sherman wrote: > Please add an enum attribute for this histogram. Done. https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19024: +<histogram name="GeolocationHeader.PermissionState"> On 2016/11/30 00:33:35, Ilya Sherman wrote: > nit: Please group this with the existing Geolocation histograms, by inserting a > dot between "Geolocation" and "Header" (both here and in the Java code that > records this histogram). Done. The labels for the enums are longer than one line. I suspect this will cause a problem. Do I need to make them shorter? https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19025: + <owner>mvanouwerkerk@chromium.org</owner> On 2016/11/30 00:33:35, Ilya Sherman wrote: > Hmm, how did you choose Michael to be an owner for this histogram? I notice > that he's not even reviewing this CL. Is that intentional, or copy/pasta? I kept the same owner as the other Geolocation histograms. This is a starter project for me, so I don't expect to continue working on this code. I'll cc mvanouwerkerk@chromium.org and ask him who should own this. https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19027: + Counts Geolocation requests for each combination of location source, M+ On 2016/11/30 00:33:35, Ilya Sherman wrote: > The phrase "M+ Chrome permission" is fairly confusing IMO -- I think "M+" refers > to Android versions, but that's not super obvious from the context here. I'd > recommend either being more descriptive, or less -- the enum labels should > clarify what the exact breakdown is. The summary is more intended to convey > what the overall metric's purpose is, and the precise conditions under which > it's recorded. Ok. I've made this shorter. https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19030: + was sent or not. On 2016/11/30 00:33:35, Ilya Sherman wrote: > Hmm, what does it mean to divide cases "where a location could not be sent" into > "whether the location was sent"? Should one of those phrases be something else? > Or am I misreading this somehow? Acknowledged.
Thanks for doing this :) https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:254: WindowAndroid window = new WindowAndroid(context); On 2016/11/30 08:00:43, pdyson wrote: > On 2016/11/30 at 00:08:46, dominickn wrote: > > I'm almost certain that creating a new WindowAndroid here to get the activity > is not the right way to do this. > > > > A better way would be to modify the two callsites of > GeolocationHeader#getGeoHeader to pass a Tab object to this method, and then > call tab.getWindowAndroid().getActivity().get(). > > I have made this change, but now I need to mock a Tab for testing. I've had a > look around but I haven't found how to do that. Can you point me to an example? You'll need to change the test case to extend ChromeActivityTestCaseBase<ChromeActivity>. That's a different subclass of InstrumentationTestCase that sets up a Chrome activity and tab for you. Using that, you'll be able to grab the current tab with getActivity().getActivityTab(). See PermissionTestCaseBase.java for an example - you'll just need to override startMainActivity() like the PermissionTestCaseBase does and I *think* that's it. https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:76: GeolocationTracker.setLocationForTesting(null); On 2016/11/30 08:00:43, pdyson wrote: > On 2016/11/30 00:21:06, dominickn wrote: > > On 2016/11/30 00:08:46, dominickn wrote: > > > This line here erases the mock location set in this test. That may be the > > source > > > of the flakiness you're seeing. > > > > Though now that I look at it again, it shouldn't really matter by this point. > > Hmm. Can you link to a trybot where we see that the GPS provider is used, not > > network? > > I got the error with patch set 3 running linux_android_rel_ng. When I ran with > patch set 5 linux_android_rel_ng failed but did not seem to run this test. > > patch set 3: > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > patch set 5: > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > Interesting. Once you have the latest version up, let's try seeing whether the bots don't like it, and we can also test it locally with various location switch configurations to see if we can track down the issue. https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19024: +<histogram name="GeolocationHeader.PermissionState"> On 2016/11/30 08:00:43, pdyson wrote: > On 2016/11/30 00:33:35, Ilya Sherman wrote: > > nit: Please group this with the existing Geolocation histograms, by inserting > a > > dot between "Geolocation" and "Header" (both here and in the Java code that > > records this histogram). > > Done. The labels for the enums are longer than one line. I suspect this will > cause a problem. Do I need to make them shorter? The labels definitely need to be shorter if possible, or else it makes reading the histogram challenging. You could put more context in the summary to make it clearer what's going on / explain shortenings. What about: High acc., app allowed, domain denied, location sent High acc., app allowed, domain denied, location not sent etc.? https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:230: if (!isGeoHeaderEnabledForUrl(context, url, isIncognito, true)) { Nit: reverse this conditional and move lines 234 - 245 in here so that we don't do the (potentially expensive) location query if the geo header isn't enabled for this URL. Then move line 231 into an else case in this conditional. https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:241: if (locationAge > MAX_LOCATION_AGE) { Nit: this can be an else if, and you can probably remove the location variable since it isn't used elsewhere and just inline the method call in the else if () https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:247: @LocationSource Nit: the @annotation is usually inlined with the variable in cases like this. https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:413: switch (locationSource) { This will be much more compact if you use conditionals rather than switches. With switches, you need a bunch of redundant default cases (because the IntDef isn't smart enough for switch/case to not need it); with conditionals, it would look like: if (locationSource == LOCATION_SOURCE_HIGH_ACCURACY) { if (appPermission == PERMISSION_GRANTED) { if (domainPermission == PERMISSION_GRANTED) { return locationAttached ? UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_LOCATION : UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_NO_LOCATION; } else if (domainPermission == PERMISSION_PROMPT) { return locationAttached ? UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_PROMPT_LOCATION : UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_PROMPT_LOCATION; } else if (domainPermission == PERMISSION_BLOCKED) { return UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_BLOCKED; } } else if (appPermission == PERMISSION_PROMPT) { ... } else if (appPermission == PERMISSION_BLOCKED) { ... } } return UMA_PERM_UNKNOWN; This is definitely quite large and potentially subject to copy-pasta issues like Ilya said, but I think it's more reviewable and checkable this way too. https://codereview.chromium.org/2533523002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:96: Tab tab = new Tab(0, false, window); You'll be able to get rid of these WindowAndroid / Tab constructions once you move the test case to inheriting from ChromeActivityTestCaseBase. https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18962: + <owner>mvanouwerkerk@chromium.org</owner> You can probably add kcarattini@ and myself as owners here.
https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88409: + <int value="42" label="Location mode unknown."/> By the way, I didn't notice before: It's typically best to list "unknown" as the first entry. That way, if you ever add additional entries, it won't end up buried somewhere in the middle of the list.
https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:602: "Geolocation.Header.PermissionState", result, UMA_PERM_MAX); Instead of UMA_PERM_MAX, you want UMA_PERM_MAX + 1. Or, rather, I'd recommend replacing UMA_PERM_MAX with UMA_PERM_COUNT, and setting that to 43 rather than 42. But basically enum histograms expect an overflow bucket that's never used, which is essentially a bit of a hack that allows the histogram to be resized later.
On 2016/12/01 at 00:30:27, dominickn wrote: > Thanks for doing this :) > > https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): > > https://codereview.chromium.org/2533523002/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:254: WindowAndroid window = new WindowAndroid(context); > On 2016/11/30 08:00:43, pdyson wrote: > > On 2016/11/30 at 00:08:46, dominickn wrote: > > > I'm almost certain that creating a new WindowAndroid here to get the activity > > is not the right way to do this. > > > > > > A better way would be to modify the two callsites of > > GeolocationHeader#getGeoHeader to pass a Tab object to this method, and then > > call tab.getWindowAndroid().getActivity().get(). > > > > I have made this change, but now I need to mock a Tab for testing. I've had a > > look around but I haven't found how to do that. Can you point me to an example? > > You'll need to change the test case to extend ChromeActivityTestCaseBase<ChromeActivity>. That's a different subclass of InstrumentationTestCase that sets up a Chrome activity and tab for you. > > Using that, you'll be able to grab the current tab with getActivity().getActivityTab(). See PermissionTestCaseBase.java for an example - you'll just need to override startMainActivity() like the PermissionTestCaseBase does and I *think* that's it. > > https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... > File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): > > https://codereview.chromium.org/2533523002/diff/80001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:76: GeolocationTracker.setLocationForTesting(null); > On 2016/11/30 08:00:43, pdyson wrote: > > On 2016/11/30 00:21:06, dominickn wrote: > > > On 2016/11/30 00:08:46, dominickn wrote: > > > > This line here erases the mock location set in this test. That may be the > > > source > > > > of the flakiness you're seeing. > > > > > > Though now that I look at it again, it shouldn't really matter by this point. > > > Hmm. Can you link to a trybot where we see that the GPS provider is used, not > > > network? > > > > I got the error with patch set 3 running linux_android_rel_ng. When I ran with > > patch set 5 linux_android_rel_ng failed but did not seem to run this test. > > > > patch set 3: > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > > > patch set 5: > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > > > Interesting. Once you have the latest version up, let's try seeing whether the bots don't like it, and we can also test it locally with various location switch configurations to see if we can track down the issue. > > https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:19024: +<histogram name="GeolocationHeader.PermissionState"> > On 2016/11/30 08:00:43, pdyson wrote: > > On 2016/11/30 00:33:35, Ilya Sherman wrote: > > > nit: Please group this with the existing Geolocation histograms, by inserting > > a > > > dot between "Geolocation" and "Header" (both here and in the Java code that > > > records this histogram). > > > > Done. The labels for the enums are longer than one line. I suspect this will > > cause a problem. Do I need to make them shorter? > > The labels definitely need to be shorter if possible, or else it makes reading the histogram challenging. You could put more context in the summary to make it clearer what's going on / explain shortenings. What about: > > High acc., app allowed, domain denied, location sent > High acc., app allowed, domain denied, location not sent > > etc.? > > https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): > > https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:230: if (!isGeoHeaderEnabledForUrl(context, url, isIncognito, true)) { > Nit: reverse this conditional and move lines 234 - 245 in here so that we don't do the (potentially expensive) location query if the geo header isn't enabled for this URL. Then move line 231 into an else case in this conditional. > > https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:241: if (locationAge > MAX_LOCATION_AGE) { > Nit: this can be an else if, and you can probably remove the location variable since it isn't used elsewhere and just inline the method call in the else if () > > https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:247: @LocationSource > Nit: the @annotation is usually inlined with the variable in cases like this. > > https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:413: switch (locationSource) { > This will be much more compact if you use conditionals rather than switches. With switches, you need a bunch of redundant default cases (because the IntDef isn't smart enough for switch/case to not need it); with conditionals, it would look like: > > if (locationSource == LOCATION_SOURCE_HIGH_ACCURACY) { > if (appPermission == PERMISSION_GRANTED) { > if (domainPermission == PERMISSION_GRANTED) { > return locationAttached ? UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_LOCATION > : UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_NO_LOCATION; > } else if (domainPermission == PERMISSION_PROMPT) { > return locationAttached > ? UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_PROMPT_LOCATION > : UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_PROMPT_LOCATION; > } else if (domainPermission == PERMISSION_BLOCKED) { > return UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_BLOCKED; > } > } else if (appPermission == PERMISSION_PROMPT) { > ... > } else if (appPermission == PERMISSION_BLOCKED) { > ... > } > } > > return UMA_PERM_UNKNOWN; > > This is definitely quite large and potentially subject to copy-pasta issues like Ilya said, but I think it's more reviewable and checkable this way too. > > https://codereview.chromium.org/2533523002/diff/100001/chrome/android/javates... > File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): > > https://codereview.chromium.org/2533523002/diff/100001/chrome/android/javates... > chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:96: Tab tab = new Tab(0, false, window); > You'll be able to get rid of these WindowAndroid / Tab constructions once you move the test case to inheriting from ChromeActivityTestCaseBase. > > https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:18962: + <owner>mvanouwerkerk@chromium.org</owner> > You can probably add kcarattini@ and myself as owners here. When I try and use ChromeActivityTestCaseBase<ChromeActivity> I get the error: ../../chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:30: error: constructor ChromeActivityTestCaseBase in class ChromeActivityTestCaseBase<T> cannot be applied to given types; public class GeolocationHeaderTest extends ChromeActivityTestCaseBase<ChromeActivity> { ^ required: Class<ChromeActivity> found: no arguments reason: actual and formal argument lists differ in length where T is a type-variable: T extends ChromeActivity declared in class ChromeActivityTestCaseBase Note: Some input files additionally use or override a deprecated API. 1 error 100 warnings Not sure how to fix this. I'll keep trying things.
https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19024: +<histogram name="GeolocationHeader.PermissionState"> On 2016/12/01 at 00:30:26, dominickn wrote: > On 2016/11/30 08:00:43, pdyson wrote: > > On 2016/11/30 00:33:35, Ilya Sherman wrote: > > > nit: Please group this with the existing Geolocation histograms, by inserting > > a > > > dot between "Geolocation" and "Header" (both here and in the Java code that > > > records this histogram). > > > > Done. The labels for the enums are longer than one line. I suspect this will > > cause a problem. Do I need to make them shorter? > > The labels definitely need to be shorter if possible, or else it makes reading the histogram challenging. You could put more context in the summary to make it clearer what's going on / explain shortenings. What about: > > High acc., app allowed, domain denied, location sent > High acc., app allowed, domain denied, location not sent > > etc.? Ok. I've made them shorter and explained the abbreviations in the description. https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:230: if (!isGeoHeaderEnabledForUrl(context, url, isIncognito, true)) { On 2016/12/01 at 00:30:26, dominickn wrote: > Nit: reverse this conditional and move lines 234 - 245 in here so that we don't do the (potentially expensive) location query if the geo header isn't enabled for this URL. Then move line 231 into an else case in this conditional. Done. I've added a comment above the changed line 258. Should I have added "location == null" to that if clause even though it is redundant? Is it better to do that and be safe here? Or just a comment without the check? https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:241: if (locationAge > MAX_LOCATION_AGE) { On 2016/12/01 at 00:30:26, dominickn wrote: > Nit: this can be an else if, and you can probably remove the location variable since it isn't used elsewhere and just inline the method call in the else if () Done. For some reason I was under the impression that Java style liked these things explicitly defined before being used. I guess not. https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:247: @LocationSource On 2016/12/01 at 00:30:26, dominickn wrote: > Nit: the @annotation is usually inlined with the variable in cases like this. I had it that way, but "git cl format" moved the annotation to the line before. Is there a way of marking this so that "git cl format" does not keep changing it? https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:413: switch (locationSource) { On 2016/12/01 at 00:30:26, dominickn wrote: > This will be much more compact if you use conditionals rather than switches. With switches, you need a bunch of redundant default cases (because the IntDef isn't smart enough for switch/case to not need it); with conditionals, it would look like: > > if (locationSource == LOCATION_SOURCE_HIGH_ACCURACY) { > if (appPermission == PERMISSION_GRANTED) { > if (domainPermission == PERMISSION_GRANTED) { > return locationAttached ? UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_LOCATION > : UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_NO_LOCATION; > } else if (domainPermission == PERMISSION_PROMPT) { > return locationAttached > ? UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_PROMPT_LOCATION > : UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_PROMPT_LOCATION; > } else if (domainPermission == PERMISSION_BLOCKED) { > return UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_BLOCKED; > } > } else if (appPermission == PERMISSION_PROMPT) { > ... > } else if (appPermission == PERMISSION_BLOCKED) { > ... > } > } > > return UMA_PERM_UNKNOWN; > > This is definitely quite large and potentially subject to copy-pasta issues like Ilya said, but I think it's more reviewable and checkable this way too. Done. I'm new to Java so it's surprising that if/then/else is preferred over switch/case here as this seems a typical case for using switch/case. But it is more compact without all the default cases, so I've changed it. https://codereview.chromium.org/2533523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:602: "Geolocation.Header.PermissionState", result, UMA_PERM_MAX); On 2016/12/01 at 00:36:18, Ilya Sherman wrote: > Instead of UMA_PERM_MAX, you want UMA_PERM_MAX + 1. Or, rather, I'd recommend replacing UMA_PERM_MAX with UMA_PERM_COUNT, and setting that to 43 rather than 42. But basically enum histograms expect an overflow bucket that's never used, which is essentially a bit of a hack that allows the histogram to be resized later. Done. https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18962: + <owner>mvanouwerkerk@chromium.org</owner> On 2016/12/01 at 00:30:26, dominickn wrote: > You can probably add kcarattini@ and myself as owners here. Done. https://codereview.chromium.org/2533523002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88409: + <int value="42" label="Location mode unknown."/> On 2016/12/01 at 00:34:17, Ilya Sherman wrote: > By the way, I didn't notice before: It's typically best to list "unknown" as the first entry. That way, if you ever add additional entries, it won't end up buried somewhere in the middle of the list. Done.
The CQ bit was checked by pdyson@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by pdyson@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
When using getActivity().getActivityTab() how do I set the incognito bit? It seems the only way to set this bit is in the Tab() constructor, so I don't see how to change it once I have it.
Metrics LGTM, thanks. https://codereview.chromium.org/2533523002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88285: + <int value="0" label="Location mode unknown."/> Optional nit: I'd omit the periods at the ends of these descriptions, since they don't make much sense in the UI where these metrics are surfaced.
The CQ bit was checked by pdyson@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 tests should pass now. https://codereview.chromium.org/2533523002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88285: + <int value="0" label="Location mode unknown."/> On 2016/12/05 at 23:01:41, Ilya Sherman wrote: > Optional nit: I'd omit the periods at the ends of these descriptions, since they don't make much sense in the UI where these metrics are surfaced. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
lgtm % nits. You'll need to send this to possibly tedchoc for OWNERs review. https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:247: @LocationSource I did a quick grep of the codebase and it seems more common to inline these custom annotations (despite what clang-format wants). https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:313: * Returns the geolocation permission. Nit: clarify that this is the activity-level geolocation permission. https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:341: * Returns the location permission for sharing their location with url (e.g. via the Nit: clarify that this is the browser-level geolocation content setting for the provided uri. https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:392: * Returns the domain permission. Nit: clarify this is the browser-level geolocation permission for the url. https://codereview.chromium.org/2533523002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:38: Context targetContext = ContextUtils.getApplicationContext(); This variable isn't used anymore, remove (that's why the android_clang_dbg bot isn't compiling)
The CQ bit was checked by pdyson@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: This issue passed the CQ dry run.
pdyson@chromium.org changed reviewers: + tedchoc@chromium.org
H Paul, This is looking good! Just some nits. Thanks for doing this, Kendra https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:95: public static final int UMA_PERM_MAX = 42; 43? https://codereview.chromium.org/2533523002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:310: * Returns the activity-level geolocation permission. It's not clear to me what the difference between activity-level permission and app-level permission is. Can you add more in the comment? https://codereview.chromium.org/2533523002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:338: * Returns the browser-level location permission for sharing their location with url (e.g. via browser-level -> origin-level? permission -> content setting https://codereview.chromium.org/2533523002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:339: * the fix line wrapping https://codereview.chromium.org/2533523002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19238: + Counts Geolocation requests for omnibox searches sliced by various Please clarify that this is for clank omnibox only.
https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:247: @LocationSource On 2016/12/08 at 00:03:10, dominickn wrote: > I did a quick grep of the codebase and it seems more common to inline these custom annotations (despite what clang-format wants). Done. https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:313: * Returns the geolocation permission. On 2016/12/08 at 00:03:10, dominickn wrote: > Nit: clarify that this is the activity-level geolocation permission. Done. https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:341: * Returns the location permission for sharing their location with url (e.g. via the On 2016/12/08 at 00:03:10, dominickn wrote: > Nit: clarify that this is the browser-level geolocation content setting for the provided uri. Done. https://codereview.chromium.org/2533523002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:392: * Returns the domain permission. On 2016/12/08 at 00:03:10, dominickn wrote: > Nit: clarify this is the browser-level geolocation permission for the url. Done. https://codereview.chromium.org/2533523002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:38: Context targetContext = ContextUtils.getApplicationContext(); On 2016/12/08 at 00:03:10, dominickn wrote: > This variable isn't used anymore, remove (that's why the android_clang_dbg bot isn't compiling) Done. https://codereview.chromium.org/2533523002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:310: * Returns the activity-level geolocation permission. On 2016/12/08 at 05:45:21, kcarattini wrote: > It's not clear to me what the difference between activity-level permission and app-level permission is. Can you add more in the comment? This should be app-level. I've fixed it. https://codereview.chromium.org/2533523002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:339: * the On 2016/12/08 at 05:45:21, kcarattini wrote: > fix line wrapping Done. https://codereview.chromium.org/2533523002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533523002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19238: + Counts Geolocation requests for omnibox searches sliced by various On 2016/12/08 at 05:45:21, kcarattini wrote: > Please clarify that this is for clank omnibox only. Done.
https://codereview.chromium.org/2533523002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:316: Activity activity = tab.getWindowAndroid().getActivity().get(); can the following be: return tab.getWindowAndroid().canRequestPermission(Manifest.permission.ACCESS_COARSE_LOCATION) ? PERMISSION_PROMPT : PERMISSION_BLOCKED; https://codereview.chromium.org/2533523002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:375: if (!LocationManager.PASSIVE_PROVIDER.equals(provider)) { why do you need this line/outer conditional? seems like the following two lines should be sufficient https://codereview.chromium.org/2533523002/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:88: runTestOnUiThread(new Runnable() { I think we more often use ThreadUtils.runOnUiThreadBlocking, no need for the try catch in that case https://codereview.chromium.org/2533523002/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:102: assertNull(header); i'd probably combine the assertNull, assertNotNull into a single method (something like assertHeaderState that takes a shouldBeNull) https://codereview.chromium.org/2533523002/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:191: permissions.concat(Integer.toString(i) + ":" + Integer.toString(count) + " "); you shouldn't need the Integer.toString
The CQ bit was checked by pdyson@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/2533523002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2533523002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:316: Activity activity = tab.getWindowAndroid().getActivity().get(); On 2016/12/10 at 01:06:09, Ted C (OOO 12.8.16) wrote: > can the following be: > > return tab.getWindowAndroid().canRequestPermission(Manifest.permission.ACCESS_COARSE_LOCATION) > ? PERMISSION_PROMPT : PERMISSION_BLOCKED; I've made this change as it seems to make sense to me, but I'm new to Chrome and Android, so I've also asked the reviewers of my design doc to make sure this is what they wanted. https://codereview.chromium.org/2533523002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:375: if (!LocationManager.PASSIVE_PROVIDER.equals(provider)) { On 2016/12/10 at 01:06:09, Ted C (OOO 12.8.16) wrote: > why do you need this line/outer conditional? seems like the following two lines should be sufficient Fixed. https://codereview.chromium.org/2533523002/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java (right): https://codereview.chromium.org/2533523002/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:88: runTestOnUiThread(new Runnable() { On 2016/12/10 at 01:06:09, Ted C (OOO 12.8.16) wrote: > I think we more often use ThreadUtils.runOnUiThreadBlocking, no need for the try catch in that case Done. https://codereview.chromium.org/2533523002/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:102: assertNull(header); On 2016/12/10 at 01:06:09, Ted C (OOO 12.8.16) wrote: > i'd probably combine the assertNull, assertNotNull into a single method (something like assertHeaderState that takes a shouldBeNull) I've made that method and changed "expectNull" to "shouldBeNull" to be consistent. https://codereview.chromium.org/2533523002/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:191: permissions.concat(Integer.toString(i) + ":" + Integer.toString(count) + " "); On 2016/12/10 at 01:06:09, Ted C (OOO 12.8.16) wrote: > you shouldn't need the Integer.toString Done.
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
The CQ bit was checked by pdyson@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_...)
I removed the test as it assumed that location was "high accuracy". A test was failing the CQ dry run as it was running on a set up that was "gps only" rather than "high accuracy". Last week Dominick told me that these permissions could not be mocked out, so I have removed this test to stop it being flaky. As I have all the required approvals and it is passing CQ I will submit tomorrow.
The CQ bit was checked by pdyson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, dominickn@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2533523002/#ps260001 (title: "Remove test that requires high accuracy location.")
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": 260001, "attempt_start_ts": 1481670104926380,
"parent_rev": "d27e710ae23a7932d4e50f1d3d95498ae65642e9", "commit_rev":
"9eb784a4547b5190d9412f897fa85fdaa7878027"}
Message was sent while issue was closed.
Description was changed from ========== Add Geolocation.PermissionState histogram. The GeolocationHeader.PermissionState histogram counts the location requests for each combination of Location Source, M+ Chrome App permission and Chrome domain omnibox permission. For cases where a location could be sent it further divides the counts into whether the location was sent or not. BUG=668923 ========== to ========== Add Geolocation.PermissionState histogram. The GeolocationHeader.PermissionState histogram counts the location requests for each combination of Location Source, M+ Chrome App permission and Chrome domain omnibox permission. For cases where a location could be sent it further divides the counts into whether the location was sent or not. BUG=668923 Review-Url: https://codereview.chromium.org/2533523002 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add Geolocation.PermissionState histogram. The GeolocationHeader.PermissionState histogram counts the location requests for each combination of Location Source, M+ Chrome App permission and Chrome domain omnibox permission. For cases where a location could be sent it further divides the counts into whether the location was sent or not. BUG=668923 Review-Url: https://codereview.chromium.org/2533523002 ========== to ========== Add Geolocation.PermissionState histogram. The GeolocationHeader.PermissionState histogram counts the location requests for each combination of Location Source, M+ Chrome App permission and Chrome domain omnibox permission. For cases where a location could be sent it further divides the counts into whether the location was sent or not. BUG=668923 Committed: https://crrev.com/fff2d84dfc1e5b0c38728cf242755e9c2f175013 Cr-Commit-Position: refs/heads/master@{#438329} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/fff2d84dfc1e5b0c38728cf242755e9c2f175013 Cr-Commit-Position: refs/heads/master@{#438329} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
