Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1535883002:
Request location perm. on enabling Physical Web (Closed)
DescriptionRequest location perm. on enabling Physical Web
If we do not already have the location permission, we need to request
it when the user enables the Physical Web feature in the privacy
settings.
BUG=529962
Committed: https://crrev.com/df2dae8631964a108750f2d5bb8f3132c632d59a
Cr-Commit-Position: refs/heads/master@{#367028}
Patch Set 1 #Patch Set 2 : Remove extraneous logging #
Total comments: 6
Patch Set 3 : Enable preference even when location permission rejected #
Total comments: 3
Patch Set 4 : Don't request permission if already granted #
Total comments: 1
Messages
Total messages: 26 (9 generated)
Description was changed from ========== Request location perm. on enabling Physical Web If we do not already have the location permission, we need to request it when the user enables the Physical Web feature in the privacy settings. BUG=529962 ========== to ========== Request location perm. on enabling Physical Web If we do not already have the location permission, we need to request it when the user enables the Physical Web feature in the privacy settings. BUG=529962 ==========
cco3@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@chromium.org changed reviewers: + tedchoc@chromium.org
Unless this needs to quickly get in, I'd love for tedchoc to review this.
On 2015/12/18 23:58:07, nyquist - OOO till 2016-01-04 wrote: > Unless this needs to quickly get in, I'd love for tedchoc to review this. It doesn't need to get in today, but his calendar says he's OOO til 1/9.
cco3@chromium.org changed reviewers: + newt@chromium.org
Adding newt@ since tedchoc@ and nyquist@ are out.
alexandermont@chromium.org changed reviewers: + alexandermont@chromium.org
https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java (right): https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:57: // location permission or lack thereof. The comment here doesn't seem to match the code. The comment says "we will flip the setting to true" (presumably referring to setPhysicalWebEnabled) regardless of whether location permission was granted, but the code only calls setPhysicalWebEnabled(true) if location permission was granted.
Thanks for the review! https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java (right): https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:57: // location permission or lack thereof. On 2015/12/28 18:58:01, alexandermont wrote: > The comment here doesn't seem to match the code. The comment says "we will flip > the setting to true" (presumably referring to setPhysicalWebEnabled) regardless > of whether location permission was granted, but the code only calls > setPhysicalWebEnabled(true) if location permission was granted. Done.
dfalcantara@chromium.org changed reviewers: + dfalcantara@chromium.org
https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java (right): https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:40: setPhysicalWebEnabled(true); Shouldn't this return, or use an else clause? This is immediately dropping down to request permissions that have already been granted. https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:57: // location permission or lack thereof. Is this comment out of sync? It looks like you only set the setting to true if the permission was granted.
https://codereview.chromium.org/1535883002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java (right): https://codereview.chromium.org/1535883002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:70: // location permission or lack thereof. Can you clarify here? Setting it to enabled without having the required permissions would leave the user in a weird broken/limbo state, wouldn't it?
https://codereview.chromium.org/1535883002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java (right): https://codereview.chromium.org/1535883002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:70: // location permission or lack thereof. On 2015/12/28 22:56:38, dfalcantara wrote: > Can you clarify here? Setting it to enabled without having the required > permissions would leave the user in a weird broken/limbo state, wouldn't it? Depends on what you mean by broken. The feature will be on, but we won't actually scan for beacons until we have location permission (which can off course be turned on and off at will in M). We exhibit the same functionality when we don't have a location provider or if bluetooth is turned off, so there are several similar situations to this one.
Guessing a new patch for my comment in PS2 is coming up? https://codereview.chromium.org/1535883002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java (right): https://codereview.chromium.org/1535883002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:70: // location permission or lack thereof. On 2015/12/28 23:14:05, cco3 wrote: > On 2015/12/28 22:56:38, dfalcantara wrote: > > Can you clarify here? Setting it to enabled without having the required > > permissions would leave the user in a weird broken/limbo state, wouldn't it? > > Depends on what you mean by broken. The feature will be on, but we won't > actually scan for beacons until we have location permission (which can off > course be turned on and off at will in M). We exhibit the same functionality > when we don't have a location provider or if bluetooth is turned off, so there > are several similar situations to this one. I guess that's alright. I'm coming from the perspective of the PermissionUpdateInfoBarDelegate, which (I believe) pops up an infobar when a website tries to get access to a permission but Chrome itself doesn't have access (i.e. Chrome's internal setting and the Android setting are out of sync). If you're fine with that scenario, then that's alright.
On 2015/12/28 23:20:09, dfalcantara wrote: > Guessing a new patch for my comment in PS2 is coming up? > > https://codereview.chromium.org/1535883002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java > (right): > > https://codereview.chromium.org/1535883002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:70: > // location permission or lack thereof. > On 2015/12/28 23:14:05, cco3 wrote: > > On 2015/12/28 22:56:38, dfalcantara wrote: > > > Can you clarify here? Setting it to enabled without having the required > > > permissions would leave the user in a weird broken/limbo state, wouldn't it? > > > > Depends on what you mean by broken. The feature will be on, but we won't > > actually scan for beacons until we have location permission (which can off > > course be turned on and off at will in M). We exhibit the same functionality > > when we don't have a location provider or if bluetooth is turned off, so there > > are several similar situations to this one. > > I guess that's alright. I'm coming from the perspective of the > PermissionUpdateInfoBarDelegate, which (I believe) pops up an infobar when a > website tries to get access to a permission but Chrome itself doesn't have > access (i.e. Chrome's internal setting and the Android setting are out of sync). > If you're fine with that scenario, then that's alright. SGTM
https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java (right): https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:40: setPhysicalWebEnabled(true); On 2015/12/28 22:54:22, dfalcantara wrote: > Shouldn't this return, or use an else clause? This is immediately dropping down > to request permissions that have already been granted. Done. https://codereview.chromium.org/1535883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:57: // location permission or lack thereof. On 2015/12/28 22:54:22, dfalcantara wrote: > Is this comment out of sync? It looks like you only set the setting to true if > the permission was granted. Yea, alexandermont@ caught that.
lgtm https://chromiumcodereview.appspot.com/1535883002/diff/60001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java (right): https://chromiumcodereview.appspot.com/1535883002/diff/60001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:35: private void ensureLocationPermission() { nit: you can avoid the extra indentation if you flip the conditional: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { setPhysicalWebEnabled(true); return; } if (getActivity().checkSelfPermission(etc)) { // permission already granted } else { // request location permission }
On 2015/12/28 23:35:56, dfalcantara wrote: > lgtm > > https://chromiumcodereview.appspot.com/1535883002/diff/60001/chrome/android/j... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java > (right): > > https://chromiumcodereview.appspot.com/1535883002/diff/60001/chrome/android/j... > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PhysicalWebPreferenceFragment.java:35: > private void ensureLocationPermission() { > nit: you can avoid the extra indentation if you flip the conditional: > > if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { > setPhysicalWebEnabled(true); > return; > } > > if (getActivity().checkSelfPermission(etc)) { > // permission already granted > } else { > // request location permission > } Understood...I wrote it the way I did because I've had more luck with automatic linters this way (checking if it's OK to use new apis is harder outside a condition).
The CQ bit was checked by cco3@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535883002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535883002/60001
Message was sent while issue was closed.
Description was changed from ========== Request location perm. on enabling Physical Web If we do not already have the location permission, we need to request it when the user enables the Physical Web feature in the privacy settings. BUG=529962 ========== to ========== Request location perm. on enabling Physical Web If we do not already have the location permission, we need to request it when the user enables the Physical Web feature in the privacy settings. BUG=529962 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Request location perm. on enabling Physical Web If we do not already have the location permission, we need to request it when the user enables the Physical Web feature in the privacy settings. BUG=529962 ========== to ========== Request location perm. on enabling Physical Web If we do not already have the location permission, we need to request it when the user enables the Physical Web feature in the privacy settings. BUG=529962 Committed: https://crrev.com/df2dae8631964a108750f2d5bb8f3132c632d59a Cr-Commit-Position: refs/heads/master@{#367028} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/df2dae8631964a108750f2d5bb8f3132c632d59a Cr-Commit-Position: refs/heads/master@{#367028} |
