Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2880663004:
Adding visible networks to the Geolocation Header. (Closed)
DescriptionAdding visible networks to the Geolocation Header.
Up to 2 wifis and 2 cells are included, only when permissions are
granted but location is not available or older than 5 minutes.
BUG=718475
Review-Url: https://codereview.chromium.org/2880663004
Cr-Commit-Position: refs/heads/master@{#474605}
Committed: https://chromium.googlesource.com/chromium/src/+/3560f09e08797cb8e758c2524902945b9dfcfc72
Patch Set 1 #Patch Set 2 : Adding visible networks to the Geolocation Header. #Patch Set 3 : Adding visible networks to the Geolocation Header. #Patch Set 4 : Adding visible networks to the Geolocation Header. #Patch Set 5 : Adding visible networks to the Geolocation Header. #Patch Set 6 : Adding visible networks to the Geolocation Header. #Patch Set 7 : Adding visible networks to the Geolocation Header. #
Total comments: 27
Patch Set 8 : Adding visible networks to the Geolocation Header. #Patch Set 9 : Adding visible networks to the Geolocation Header. #Patch Set 10 : Adding visible networks to the Geolocation Header. #
Total comments: 2
Patch Set 11 : Adding visible networks to the Geolocation Header. #Patch Set 12 : Adding visible networks to the Geolocation Header. #Patch Set 13 : Adding visible networks to the Geolocation Header. #Patch Set 14 : Adding visible networks to the Geolocation Header. #Patch Set 15 : Adding visible networks to the Geolocation Header. #Patch Set 16 : Adding visible networks to the Geolocation Header. #Patch Set 17 : Adding visible networks to the Geolocation Header. #Patch Set 18 : Adding visible networks to the Geolocation Header. #Patch Set 19 : Adding visible networks to the Geolocation Header. #
Messages
Total messages: 39 (28 generated)
Description was changed from ========== Adding visible networks to the Geolocation Header. Up to 2 wifis and 2 cells are included, only when permissions are granted but location is not available or older than 5 minutes. BUG=718475 ========== to ========== Adding visible networks to the Geolocation Header. Up to 2 wifis and 2 cells are included, only when permissions are granted but location is not available or older than 5 minutes. BUG=718475 ==========
lbargu@google.com changed reviewers: + tedchoc@chromium.org
This should be the last CL. Will be submitted after these two: https://codereview.chromium.org/2884013002/ https://codereview.chromium.org/2883063003/ https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:209: */ Ignore changes to this class. See https://codereview.chromium.org/2884013002/ https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:15: class VisibleNetworksTracker { Ignore changes to this file. See https://codereview.chromium.org/2883063003/ https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java_so... File chrome/android/java_sources.gni (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java_so... chrome/android/java_sources.gni:729: "java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java", Ignore this change. See https://codereview.chromium.org/2883063003/
lbargu@google.com changed reviewers: + dougt@chromium.org
https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:295: public static String getGeoHeader(String url, Tab tab) { This method is getting a bit unwieldy. We should follow up and attempt to refactor. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:302: ChromeFeatureList.isEnabled(ChromeFeatureList.XGEO_VISIBLE_NETWORKS); I should have caught this in the last review, but you may want to write a comment above this line. Something like: // XGEO_VISIBLE_NETWORKS // When this feature is enabled, we will send visible WiFi Access Points as part of the X-GEO HTTP Header so that we can // better position the client, or in the case where there is no lat/long or bad accuracy we might be able to geolocation // server side for a better position. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:307: locationToAttach = GeolocationTracker.getLastKnownLocation( Since this is on the hot path, I think we should collect how long this call takes. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:331: > REFRESH_LOCATION_AGE) { I would have turned this logic around a bit and just have a longer if stm. if (isXGeoVisibleNetworksEnabled && locationToAttach != null && (getLocationSource() == LOCATION_SOURCE_HIGH_ACCURACY || getLocationSource() == LOCATION_SOURCE_BATTERY_SAVING) && GeolocationTracker.getLocationAge(locationToAttach) > REFRESH_LOCATION_AGE { .... } I would probably also just factor this out into a private mthod that returns a VisibleNetworks object (or null) in a follow up? https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:333: // but we didn't attach location or it's older than the refresh time. no need for this comment. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:361: if (locationToAttach != null) { nit: avoid the block, and just return early: if (locationToAttach == null) return null; https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:394: } Would something like the following work? if (locationProtoEncoding != null) { String result = XGEO_HEADER_PREFIX + LOCATION_PROTO_PREFIX + locationProtoEncoding; if (visibleNetworksProtoEncoding != null) result += LOCATION_PROTO_SEPARATOR + visibleNetworksProtoEncoding; return result; }
https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:394: } On 2017/05/18 02:33:15, dougt wrote: > Would something like the following work? > > if (locationProtoEncoding != null) { > String result = XGEO_HEADER_PREFIX + LOCATION_PROTO_PREFIX + > locationProtoEncoding; > if (visibleNetworksProtoEncoding != null) > result += LOCATION_PROTO_SEPARATOR + visibleNetworksProtoEncoding; > return result; > } We should use a StringBuilder if we go with this approach. If we dropped the trailing space from the proto prefix, could we just do: if (locaitonProtoEncoding == null && visibleNetworksProtoEncoding == null) return null; StringBuilder header = new StringBuilder(XGEO_HEADER_PREFIX + LOCATION_PROTO_PREFIX); if (locationProtoEncoding != null) { header.append(LOCATION_PROTO_SEPARATOR).append(locationProtoEncoding); } if (visibleNetworksProtoEncoding != null) { header.append(LOCATION_PROTO_SEPARATOR).append(visibleNetworksProtoEncoding); } If we can't drop the trailing space, then it gets slightly more annoying where we will need to check locaitonProtoEncoding != null in the second if and only add the separator in that case. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:403: if (sUseAppPermissionGrantedForTesting) return sAppPermissionGrantedForTesting; can we use http://robolectric.org/javadoc/latest/org/robolectric/shadows/ShadowPackageMa... instead of introducing this static? https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:896: extraVisibleWifi != null ? new HashSet<>(Arrays.asList(extraVisibleWifi)) : null, I would use newHashSet here https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationTracker.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationTracker.java:83: if (sUseLocationAgeForTesting) return sLocationAgeForTesting; same comment as the other cl where I think we can use the shadow system clock to do this
https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:295: public static String getGeoHeader(String url, Tab tab) { On 2017/05/18 02:33:15, dougt wrote: > This method is getting a bit unwieldy. We should follow up and attempt to > refactor. Agree. Already added all new functionality to private methods, but added a TODO. Will fix in a follow up CL. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:302: ChromeFeatureList.isEnabled(ChromeFeatureList.XGEO_VISIBLE_NETWORKS); On 2017/05/18 02:33:15, dougt wrote: > I should have caught this in the last review, but you may want to write a > comment above this line. Something like: > > // XGEO_VISIBLE_NETWORKS > // When this feature is enabled, we will send visible WiFi Access Points as part > of the X-GEO HTTP Header so that we can > // better position the client, or in the case where there is no lat/long or bad > accuracy we might be able to geolocation > // server side for a better position. Done. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:307: locationToAttach = GeolocationTracker.getLastKnownLocation( On 2017/05/18 02:33:15, dougt wrote: > Since this is on the hot path, I think we should collect how long this call > takes. Added TODO. Will add metrics in follow up. For now, I guess we should be able to rely on overall performance metrics. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:331: > REFRESH_LOCATION_AGE) { On 2017/05/18 02:33:15, dougt wrote: > I would have turned this logic around a bit and just have a longer if stm. > > > if (isXGeoVisibleNetworksEnabled && locationToAttach != null && > (getLocationSource() == LOCATION_SOURCE_HIGH_ACCURACY || getLocationSource() == > LOCATION_SOURCE_BATTERY_SAVING) && > GeolocationTracker.getLocationAge(locationToAttach) > REFRESH_LOCATION_AGE { > .... > } > > I would probably also just factor this out into a private mthod that returns a > VisibleNetworks object (or null) in a follow up? > > > > Moved checks to private methods. Should be easier to read now. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:333: // but we didn't attach location or it's older than the refresh time. On 2017/05/18 02:33:15, dougt wrote: > no need for this comment. Done. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:361: if (locationToAttach != null) { On 2017/05/18 02:33:15, dougt wrote: > nit: avoid the block, and just return early: > > if (locationToAttach == null) return null; Done. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:394: } On 2017/05/18 14:43:03, Ted C wrote: > On 2017/05/18 02:33:15, dougt wrote: > > Would something like the following work? > > > > if (locationProtoEncoding != null) { > > String result = XGEO_HEADER_PREFIX + LOCATION_PROTO_PREFIX + > > locationProtoEncoding; > > if (visibleNetworksProtoEncoding != null) > > result += LOCATION_PROTO_SEPARATOR + visibleNetworksProtoEncoding; > > return result; > > } > > We should use a StringBuilder if we go with this approach. > > If we dropped the trailing space from the proto prefix, could we just do: > if (locaitonProtoEncoding == null && visibleNetworksProtoEncoding == null) > return null; > > StringBuilder header = new StringBuilder(XGEO_HEADER_PREFIX + > LOCATION_PROTO_PREFIX); > if (locationProtoEncoding != null) { > header.append(LOCATION_PROTO_SEPARATOR).append(locationProtoEncoding); > } > if (visibleNetworksProtoEncoding != null) { > > header.append(LOCATION_PROTO_SEPARATOR).append(visibleNetworksProtoEncoding); > } > > If we can't drop the trailing space, then it gets slightly more annoying where > we will need to check locaitonProtoEncoding != null in the second if and only > add the separator in that case. Done. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:394: } On 2017/05/18 02:33:15, dougt wrote: > Would something like the following work? > > if (locationProtoEncoding != null) { > String result = XGEO_HEADER_PREFIX + LOCATION_PROTO_PREFIX + > locationProtoEncoding; > if (visibleNetworksProtoEncoding != null) > result += LOCATION_PROTO_SEPARATOR + visibleNetworksProtoEncoding; > return result; > } Done with builder. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:403: if (sUseAppPermissionGrantedForTesting) return sAppPermissionGrantedForTesting; On 2017/05/18 14:43:03, Ted C wrote: > can we use > http://robolectric.org/javadoc/latest/org/robolectric/shadows/ShadowPackageMa... > > instead of introducing this static? It's not available in chromium source code? https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:896: extraVisibleWifi != null ? new HashSet<>(Arrays.asList(extraVisibleWifi)) : null, On 2017/05/18 14:43:03, Ted C wrote: > I would use newHashSet here > > https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... Done. https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationTracker.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationTracker.java:83: if (sUseLocationAgeForTesting) return sLocationAgeForTesting; On 2017/05/18 14:43:03, Ted C wrote: > same comment as the other cl where I think we can use the shadow system clock to > do this Not sure it would work since age is using "current time", not "elapsed time". I can't find a way to shadow current time. Am I missing something?
https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:403: if (sUseAppPermissionGrantedForTesting) return sAppPermissionGrantedForTesting; On 2017/05/18 15:14:08, lbargu wrote: > On 2017/05/18 14:43:03, Ted C wrote: > > can we use > > > http://robolectric.org/javadoc/latest/org/robolectric/shadows/ShadowPackageMa... > > > > instead of introducing this static? > > It's not available in chromium source code? Indeed it is not...sadness. Looking at what is in the codebase: http://robolectric.org/javadoc/2.4/org/robolectric/res/builder/RobolectricPac... It doesn't look like what we'll need. The only other option would be to build our entire own mock context and set that on ContextUtils. I have no idea the implications that would have on the rest of the change. It is one of those things that I think would be worth 5 min of investigation an no more.
https://codereview.chromium.org/2880663004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2880663004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:361: if (locationToAttach == null) return null; FWIW, this block does seem ripe to pull out to a separate function to be parallel with encodeProtoLocation
https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2880663004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:403: if (sUseAppPermissionGrantedForTesting) return sAppPermissionGrantedForTesting; On 2017/05/18 15:25:29, Ted C wrote: > On 2017/05/18 15:14:08, lbargu wrote: > > On 2017/05/18 14:43:03, Ted C wrote: > > > can we use > > > > > > http://robolectric.org/javadoc/latest/org/robolectric/shadows/ShadowPackageMa... > > > > > > instead of introducing this static? > > > > It's not available in chromium source code? > > Indeed it is not...sadness. > > Looking at what is in the codebase: > http://robolectric.org/javadoc/2.4/org/robolectric/res/builder/RobolectricPac... > > It doesn't look like what we'll need. The only other option would be to build > our entire own mock context and set that on ContextUtils. I have no idea the > implications that would have on the rest of the change. It is one of those > things that I think would be worth 5 min of investigation an no more. Tried, but a time drainer, no success :( https://codereview.chromium.org/2880663004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2880663004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:361: if (locationToAttach == null) return null; On 2017/05/18 15:28:13, Ted C wrote: > FWIW, this block does seem ripe to pull out to a separate function to be > parallel with encodeProtoLocation Done. Added test.
The CQ bit was checked by lbargu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lbargu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by lbargu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by lbargu@google.com 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.
The CQ bit was checked by lbargu@google.com 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...
Merged with latest changes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by lbargu@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2880663004/#ps350001 (title: "Adding visible networks to the Geolocation Header.")
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": 350001, "attempt_start_ts": 1495695357912480,
"parent_rev": "4dd065a3be8399b968ae44e93f1997254c173e84", "commit_rev":
"489f685bd2df07147801552d38f1c1b406be5bb7"}
CQ is committing da patch.
Bot data: {"patchset_id": 350001, "attempt_start_ts": 1495695357912480,
"parent_rev": "3f53bac92726138a5a000349f146e490a2652469", "commit_rev":
"3560f09e08797cb8e758c2524902945b9dfcfc72"}
Message was sent while issue was closed.
Description was changed from ========== Adding visible networks to the Geolocation Header. Up to 2 wifis and 2 cells are included, only when permissions are granted but location is not available or older than 5 minutes. BUG=718475 ========== to ========== Adding visible networks to the Geolocation Header. Up to 2 wifis and 2 cells are included, only when permissions are granted but location is not available or older than 5 minutes. BUG=718475 Review-Url: https://codereview.chromium.org/2880663004 Cr-Commit-Position: refs/heads/master@{#474605} Committed: https://chromium.googlesource.com/chromium/src/+/3560f09e08797cb8e758c2524902... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:350001) as https://chromium.googlesource.com/chromium/src/+/3560f09e08797cb8e758c2524902... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
