Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2506193003:
Hide location permission if a search engine is non-Google or its location permission is unset. (Closed)
DescriptionHide location permission if a search engine's location permission is unset.
1. "Location is allowed" will always be shown.
2. If users explicitly blocks location permission, "Location is blocked"
will be still shown, otherwise the message will be hiden if location
permission is unset.
3. If users grant location permission for the whole app, Google search engine's
location will be auto-allowed, while other search engines' are still blocked.
BUG=662525
Committed: https://crrev.com/98326ac9561a1ed6d0cb0b28a96dbce43341605f
Cr-Commit-Position: refs/heads/master@{#433723}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update based on Peter's comments. #Patch Set 3 : Update behavior that even Google's location is unset, location permission info is hidden. #
Total comments: 6
Patch Set 4 : Update based on Dan's comment. #Patch Set 5 : remove new line added at the top of class. #Patch Set 6 : Update based on Peter's suggestion. #
Total comments: 8
Patch Set 7 : Update based on Peter's new comments. #Patch Set 8 : clean and reorganize the code. #
Total comments: 2
Patch Set 9 : Update based on Peter's comment. #Messages
Total messages: 37 (21 generated)
The CQ bit was checked by ltian@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.
ltian@chromium.org changed reviewers: + dfalcantara@chromium.org, pkasting@chromium.org
dfalcantara@chromium.org: Please review changes in chrome/android. pkasting@chromium.org: Please review changes in chrome/browser. Thanks!
I have implementation concerns, but I also have some confusion/concern with the requested overall design, which I've left on the bug. https://codereview.chromium.org/2506193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:318: return keyword.equals(GOOGLE_ENGINE_KEYWORD); This is not the right way to check whether an engine is Google; it will have false positives (for user-edited search engines with google.com as the keyword) and false negatives (for Google engines where searchdomaincheck directs us to use something other than google.com). You probably want TemplateURL::HasGoogleBaseURLs(). https://codereview.chromium.org/2506193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:326: return locationPermission == ContentSetting.ASK; This doesn't seem like the right way of checking if the "permission is not explicitly set". For example, if a user sets the global default to "allow" and a specific engine to "ask", this will report "unset" for that engine, which seems wrong. You probably want to look at the SettingSource field of the content setting's SettingInfo? Check with the content settings guys. OTOH, if what you want is not whether the permission is "unset" but whether it resolves to ASK, then this is, in fact, probably what you want.
The CQ bit was checked by ltian@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.
Description was changed from ========== Hide location permission if a search engine is non-Google or its location permission is unset. 1. "Location is blocked" will be hidden if search engine is non-Google or its location permission is unset. 2. If users explicitly blocks location permission, "Location is blocked" will be still shown. 3. If location permission is unset, its default value will always be ContentSetting.ASK. BUG=662525 ========== to ========== Hide location permission if a search engine's location permission is unset. 1. "Location is allowed" will always be shown. 2. If users explicitly blocks location permission, "Location is blocked" will be still shown, otherwise the message will be hiden if location permission is unset. 3. If users grant location permission for the whole app, Google search engine's location will be auto-allowed, while other search engines' are still blocked. BUG=662525 ==========
The CQ bit was checked by ltian@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/2506193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:318: return keyword.equals(GOOGLE_ENGINE_KEYWORD); On 2016/11/17 10:05:10, Peter Kasting wrote: > This is not the right way to check whether an engine is Google; it will have > false positives (for user-edited search engines with http://google.com as the keyword) > and false negatives (for Google engines where searchdomaincheck directs us to > use something other than http://google.com). > > You probably want TemplateURL::HasGoogleBaseURLs(). Thanks,Peter. I think that is the function I need. https://codereview.chromium.org/2506193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:326: return locationPermission == ContentSetting.ASK; On 2016/11/17 10:05:10, Peter Kasting wrote: > This doesn't seem like the right way of checking if the "permission is not > explicitly set". For example, if a user sets the global default to "allow" and > a specific engine to "ask", this will report "unset" for that engine, which > seems wrong. > > You probably want to look at the SettingSource field of the content setting's > SettingInfo? Check with the content settings guys. > > OTOH, if what you want is not whether the permission is "unset" but whether it > resolves to ASK, then this is, in fact, probably what you want. Discuss with Kingston that our new design is: 1. For a non-Google search engine, if its permission is unset (means no behavior to resolve ASK), the location message will be hidden. 2. For Google search engine, its location message will be hidden only if its location permission is unset and the location permission for the whole app is not granted (if location permission for the whole app is granted, Google search engine's location permission will be auto-granted).
https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:214: if (LocationUtils.getInstance().isSystemLocationSettingEnabled()) { Is this condition, in this nested case, still what we want? For example, it means that we'll show the "location is off" message instead of "location is blocked" when the engine's setting is "block" and the system setting is "off". It means we'll show no setting (to prompt turning on location?) when the engine's setting is "ask" and the system setting is off. These might be correct behaviors, it's just not clear to me how we want these to interact and which kind of setting we want to "override" the others. https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:313: ContentSetting locationPermission = locationSettings.getContentSetting(); This code seems like it partially duplicates locationEnabled(). Is it possible to just modify that function to return an enum instead of a bool? You could even re-use the content setting enum for this (ALLOW, ASK, BLOCK).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:214: if (LocationUtils.getInstance().isSystemLocationSettingEnabled()) { On 2016/11/17 23:12:05, Peter Kasting wrote: > Is this condition, in this nested case, still what we want? For example, it > means that we'll show the "location is off" message instead of "location is > blocked" when the engine's setting is "block" and the system setting is "off". > It means we'll show no setting (to prompt turning on location?) when the > engine's setting is "ask" and the system setting is off. > > These might be correct behaviors, it's just not clear to me how we want these to > interact and which kind of setting we want to "override" the others. That part is necessary. Here the "system location" means the location service across system-wide not location permission for an single app. Just try that if we disable the system location, it will should a clickable message "Turn on location in Android Settings". And by clicking that, it forward to Android location setting page. https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:313: ContentSetting locationPermission = locationSettings.getContentSetting(); On 2016/11/17 23:12:05, Peter Kasting wrote: > This code seems like it partially duplicates locationEnabled(). > > Is it possible to just modify that function to return an enum instead of a bool? > You could even re-use the content setting enum for this (ALLOW, ASK, BLOCK). I thought about it before but the two functions logically has some differences that the locationEnabled() tries to see ContentSetting.ALLOW + GeolocationHeader.isGeoHeaderEnabledForUrl()==true while shouldShowLocatinInfo() checks ContentSetting.ASK + GeolocationHeader.isGeoHeaderEnabledForUrl()==true.
https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:214: if (LocationUtils.getInstance().isSystemLocationSettingEnabled()) { On 2016/11/18 00:17:36, ltian wrote: > On 2016/11/17 23:12:05, Peter Kasting wrote: > > Is this condition, in this nested case, still what we want? For example, it > > means that we'll show the "location is off" message instead of "location is > > blocked" when the engine's setting is "block" and the system setting is "off". > > > It means we'll show no setting (to prompt turning on location?) when the > > engine's setting is "ask" and the system setting is off. > > > > These might be correct behaviors, it's just not clear to me how we want these > to > > interact and which kind of setting we want to "override" the others. > > That part is necessary. Here the "system location" means the location service > across system-wide not location permission for an single app. Just try that if > we disable the system location, it will should a clickable message "Turn on > location in Android Settings". And by clicking that, it forward to Android > location setting page. Right, that was the UI I figured was here. I guess my point is that I don't know whether it might be misleading or unhelpful to do some of our current behavior. I'm not necessarily arguing for doing something different either -- there are arguments both ways for some of these decisions. But for example, if I explicitly block location data for Google, and my system location setting also happens to be off, does showing the equivalent of "click to turn on location" mislead or frustrate, compared to showing "location blocked for Google"? I want to be sure you've thought about all these different interaction cases and are convinced the UX implemented here is the best route :) https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:313: ContentSetting locationPermission = locationSettings.getContentSetting(); On 2016/11/18 00:17:36, ltian wrote: > On 2016/11/17 23:12:05, Peter Kasting wrote: > > This code seems like it partially duplicates locationEnabled(). > > > > Is it possible to just modify that function to return an enum instead of a > bool? > > You could even re-use the content setting enum for this (ALLOW, ASK, BLOCK). > > I thought about it before but the two functions logically has some differences > that the locationEnabled() tries to see ContentSetting.ALLOW + > GeolocationHeader.isGeoHeaderEnabledForUrl()==true while shouldShowLocatinInfo() > checks ContentSetting.ASK + GeolocationHeader.isGeoHeaderEnabledForUrl()==true. I think we want something like the following (oversimplified): locationPermission == ALLOW -> ALLOW locationPermission == ASK -> isGeoHeaderEnabled ? ALLOW : ASK locationPermission == BLOCK -> BLOCK Then, basically, we should show the text if the result is not ASK, and the text should be "allowed" if the result is ALLOW.
On 2016/11/18 00:24:47, Peter Kasting wrote: > https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java > (right): > > https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:214: > if (LocationUtils.getInstance().isSystemLocationSettingEnabled()) { > On 2016/11/18 00:17:36, ltian wrote: > > On 2016/11/17 23:12:05, Peter Kasting wrote: > > > Is this condition, in this nested case, still what we want? For example, it > > > means that we'll show the "location is off" message instead of "location is > > > blocked" when the engine's setting is "block" and the system setting is > "off". > > > > > It means we'll show no setting (to prompt turning on location?) when the > > > engine's setting is "ask" and the system setting is off. > > > > > > These might be correct behaviors, it's just not clear to me how we want > these > > to > > > interact and which kind of setting we want to "override" the others. > > > > That part is necessary. Here the "system location" means the location service > > across system-wide not location permission for an single app. Just try that if > > we disable the system location, it will should a clickable message "Turn on > > location in Android Settings". And by clicking that, it forward to Android > > location setting page. > > Right, that was the UI I figured was here. > > I guess my point is that I don't know whether it might be misleading or > unhelpful to do some of our current behavior. I'm not necessarily arguing for > doing something different either -- there are arguments both ways for some of > these decisions. But for example, if I explicitly block location data for > Google, and my system location setting also happens to be off, does showing the > equivalent of "click to turn on location" mislead or frustrate, compared to > showing "location blocked for Google"? > > I want to be sure you've thought about all these different interaction cases and > are convinced the UX implemented here is the best route :) > Thanks for pointing out. I haven't thought about that part. I guess it is a little misleading for the text. Will talk with Kingston about it and maybe figure out a better wording and fix it in another cl? > https://codereview.chromium.org/2506193003/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:313: > ContentSetting locationPermission = locationSettings.getContentSetting(); > On 2016/11/18 00:17:36, ltian wrote: > > On 2016/11/17 23:12:05, Peter Kasting wrote: > > > This code seems like it partially duplicates locationEnabled(). > > > > > > Is it possible to just modify that function to return an enum instead of a > > bool? > > > You could even re-use the content setting enum for this (ALLOW, ASK, > BLOCK). > > > > I thought about it before but the two functions logically has some differences > > that the locationEnabled() tries to see ContentSetting.ALLOW + > > GeolocationHeader.isGeoHeaderEnabledForUrl()==true while > shouldShowLocatinInfo() > > checks ContentSetting.ASK + > GeolocationHeader.isGeoHeaderEnabledForUrl()==true. > > I think we want something like the following (oversimplified): > > locationPermission == ALLOW -> ALLOW > locationPermission == ASK -> isGeoHeaderEnabled ? ALLOW : ASK > locationPermission == BLOCK -> BLOCK > > Then, basically, we should show the text if the result is not ASK, and the text > should be "allowed" if the result is ALLOW. I think it is a good point, I will try that way.
The CQ bit was checked by ltian@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.
https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:67: private @interface LocationType {} Can you just return the ContentSetting directly? That would simplify a lot of code, and I'm not sure what this separate LocationType thing buys you. https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:315: if (checkGeoHeader && GeolocationHeader.isGeoHeaderEnabledForUrl( Nit: Simpler: if (checkGeoHeader && ...) locationPermission = ContentSetting.ALLOW; Then just return the mapped type directly. (If you return a ContentSetting, this is just a simple return statement.) https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:333: // even its own location permission is unset, location permission is still granted for it. Nit: This comment no longer seems relevant. https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:335: return getLocationPermissionType(position, true) != TYPE_LOCATION_ASK; Nit: Could just inline this into the lone caller, which might be clearer.
https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:67: private @interface LocationType {} On 2016/11/18 20:25:43, Peter Kasting wrote: > Can you just return the ContentSetting directly? That would simplify a lot of > code, and I'm not sure what this separate LocationType thing buys you. Done. https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:315: if (checkGeoHeader && GeolocationHeader.isGeoHeaderEnabledForUrl( On 2016/11/18 20:25:43, Peter Kasting wrote: > Nit: Simpler: > > if (checkGeoHeader && ...) > locationPermission = ContentSetting.ALLOW; > > Then just return the mapped type directly. (If you return a ContentSetting, > this is just a simple return statement.) Done. https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:333: // even its own location permission is unset, location permission is still granted for it. On 2016/11/18 20:25:43, Peter Kasting wrote: > Nit: This comment no longer seems relevant. Done. https://codereview.chromium.org/2506193003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:335: return getLocationPermissionType(position, true) != TYPE_LOCATION_ASK; On 2016/11/18 20:25:43, Peter Kasting wrote: > Nit: Could just inline this into the lone caller, which might be clearer. Done.
I can't sign off on this as it's Java, but this seems reasonable to me, and definitely simpler/safer than the initial approach. Hooray! https://codereview.chromium.org/2506193003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:208: if (getLocationPermissionType(position, true) != ContentSetting.ASK) { Nit: Reverse conditional and arms, so that "else" condition is not mentally a double-negative ("not not equal")
https://codereview.chromium.org/2506193003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2506193003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:208: if (getLocationPermissionType(position, true) != ContentSetting.ASK) { On 2016/11/19 22:28:38, Peter Kasting wrote: > Nit: Reverse conditional and arms, so that "else" condition is not mentally a > double-negative ("not not equal") Done.
lgtm
The CQ bit was checked by ltian@chromium.org
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": 160001, "attempt_start_ts": 1479774482795850,
"parent_rev": "a172d45390d7df6a5d1daa108c6b7febc5123956", "commit_rev":
"0c3a3bae0b2e13aa2f68aceeb9fc48520433c3fb"}
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Hide location permission if a search engine's location permission is unset. 1. "Location is allowed" will always be shown. 2. If users explicitly blocks location permission, "Location is blocked" will be still shown, otherwise the message will be hiden if location permission is unset. 3. If users grant location permission for the whole app, Google search engine's location will be auto-allowed, while other search engines' are still blocked. BUG=662525 ========== to ========== Hide location permission if a search engine's location permission is unset. 1. "Location is allowed" will always be shown. 2. If users explicitly blocks location permission, "Location is blocked" will be still shown, otherwise the message will be hiden if location permission is unset. 3. If users grant location permission for the whole app, Google search engine's location will be auto-allowed, while other search engines' are still blocked. BUG=662525 Committed: https://crrev.com/98326ac9561a1ed6d0cb0b28a96dbce43341605f Cr-Commit-Position: refs/heads/master@{#433723} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/98326ac9561a1ed6d0cb0b28a96dbce43341605f Cr-Commit-Position: refs/heads/master@{#433723} |
