Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721293002:
Show the Android Location Settings Dialog when sites want permission. (Closed)
|
Created:
3 years, 9 months ago by benwells Modified:
3 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, mlamouri+watch-geolocation_chromium.org, raymes+watch_chromium.org, Michael van Ouwerkerk, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow the Android Location Settings Dialog when sites want permission.
The Location Settings Dialog (LSD) will be shown when sites which have
location permission request location, but the device has location
switched off.
BUG=673201
Review-Url: https://codereview.chromium.org/2721293002
Cr-Commit-Position: refs/heads/master@{#454708}
Committed: https://chromium.googlesource.com/chromium/src/+/e962df5122998010c9db55d0fb46993f058e7db4
Patch Set 1 #
Total comments: 8
Patch Set 2 : Updated #
Total comments: 4
Patch Set 3 : Rebase and some more stuff #
Total comments: 1
Patch Set 4 : Feedback #
Total comments: 5
Depends on Patchset: Messages
Total messages: 38 (19 generated)
The CQ bit was checked by benwells@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
benwells@chromium.org changed reviewers: + qfiard@google.com, raymes@chromium.org
Seems like trybots aren't happy cos the dependent patch needs a rebase, but can maybe review anyway.
lgtm I rebased my change to head today, let me know if you are still having troubles with trybots. https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:159: base::Bind( You will need to switch to base::BindOnce since I switched to use OnceCallbacks.
raymes - i think i'll add a test to this today, so maybe wait for that before reviewing
Some thoughts below. Thanks! https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:85: if (!location_settings_->CanSitesRequestLocationPermission(web_contents)) { I think we will need to change CanSitesRequestLocationPermission? Right now it will return false if the system permission isn't granted. But we will want it to return true in that case, as long as the system permission can be /requested/. We will also need to incorporate the backoff into that check. If we do that, then we won't need to check those values later on, we will just show the prompt if it's needed. Also, for GetPermissionStatus() we will need to split up those checks to give the right result. Something like: if (has system permission AND has chrome permission AND has content setting) return allow; if (can request system permission AND can request chrome permission AND content setting is ask) return prompt; return block; Note that the definition of "can request system permission" is different to what is currently implemented in CanPromptToEnableSystemLocationSetting. CanPromptToEnableSystemLocationSetting will currently return false if location is already enabled, whereas in the above code it would need to return true to work properly. https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:150: } Just to clarify, this is intentionally different from the logic we use in SearchGeolocationService? It looks like this will get the origin of any default search engine, not just Google.
https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:85: if (!location_settings_->CanSitesRequestLocationPermission(web_contents)) { On 2017/03/02 03:04:04, raymes wrote: > I think we will need to change CanSitesRequestLocationPermission? Right now it > will return false if the system permission isn't granted. But we will want it to > return true in that case, as long as the system permission can be /requested/. > We will also need to incorporate the backoff into that check. > > If we do that, then we won't need to check those values later on, we will just > show the prompt if it's needed. > > Also, for GetPermissionStatus() we will need to split up those checks to give > the right result. Something like: > if (has system permission AND has chrome permission AND has content setting) > return allow; > > if (can request system permission AND can request chrome permission AND content > setting is ask) > return prompt; > > return block; > > Note that the definition of "can request system permission" is different to what > is currently implemented in CanPromptToEnableSystemLocationSetting. > CanPromptToEnableSystemLocationSetting will currently return false if location > is already enabled, whereas in the above code it would need to return true to > work properly. Yep ... you're right - this won't work. I think it's confusing the the check for whether location is on embedded in the CanPromptToEnable... call. Also this CanSitesRequestLocationPermission is confusing too, I think it is confusing with 'Request' in the name - what if it doesnt' need to request? Instead we could have the following functions on LocationSettings: - IsLocationAccessPossible - would be true if location is enabled on the device (at device or chrome permission level), or it isn't and the prompts required to enabled it can be shown. Would take into account backoff, so would have SEARCH or DEFAULT as a parameter - we'd call that in RequestPermission and block early if it is false - ShouldShowLocationSettingsDialog - returns true if system permission is off and the prompt can be shown (incl. backoff) - would be called in NotifyPermissionSet if the content setting is ALLOW - ShowLocationSettingsDialog - resets backoff and shows the dialog, called in NotifyPermissionSet I think all of this can happen in the C++ with the Java side (i.e. qfiard's patch that this depends on) staying the same. WDYT? qfiard - WDYT as well? https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:150: } On 2017/03/02 03:04:04, raymes wrote: > Just to clarify, this is intentionally different from the logic we use in > SearchGeolocationService? It looks like this will get the origin of any default > search engine, not just Google. Yep, that's correct.
https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:85: if (!location_settings_->CanSitesRequestLocationPermission(web_contents)) { On 2017/03/02 04:19:28, benwells wrote: > On 2017/03/02 03:04:04, raymes wrote: > > I think we will need to change CanSitesRequestLocationPermission? Right now it > > will return false if the system permission isn't granted. But we will want it > to > > return true in that case, as long as the system permission can be /requested/. > > We will also need to incorporate the backoff into that check. > > > > If we do that, then we won't need to check those values later on, we will just > > show the prompt if it's needed. > > > > Also, for GetPermissionStatus() we will need to split up those checks to give > > the right result. Something like: > > if (has system permission AND has chrome permission AND has content setting) > > return allow; > > > > if (can request system permission AND can request chrome permission AND > content > > setting is ask) > > return prompt; > > > > return block; > > > > Note that the definition of "can request system permission" is different to > what > > is currently implemented in CanPromptToEnableSystemLocationSetting. > > CanPromptToEnableSystemLocationSetting will currently return false if location > > is already enabled, whereas in the above code it would need to return true to > > work properly. > > Yep ... you're right - this won't work. > > I think it's confusing the the check for whether location is on embedded in the > CanPromptToEnable... call. Also this CanSitesRequestLocationPermission is > confusing too, I think it is confusing with 'Request' in the name - what if it > doesnt' need to request? > > Instead we could have the following functions on LocationSettings: > - IsLocationAccessPossible > - would be true if location is enabled on the device (at device or chrome > permission level), or it isn't and the prompts required to enabled it can be > shown. Would take into account backoff, so would have SEARCH or DEFAULT as a > parameter > - we'd call that in RequestPermission and block early if it is false Sounds good to me. I think we would still need to split this bit up to make it possible to distinguish between having granted each of the permissions and whether a prompt will be shown in order to implement GetPermissionStatus. We can leave that until a subsequent patch though, but just making a note in case it would make it easier to implement things like that initially. > - ShouldShowLocationSettingsDialog > - returns true if system permission is off and the prompt can be shown (incl. > backoff) > - would be called in NotifyPermissionSet if the content setting is ALLOW > - ShowLocationSettingsDialog > - resets backoff and shows the dialog, called in NotifyPermissionSet > > I think all of this can happen in the C++ with the Java side (i.e. qfiard's > patch that this depends on) staying the same. > > WDYT? > > qfiard - WDYT as well?
https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:85: if (!location_settings_->CanSitesRequestLocationPermission(web_contents)) { On 2017/03/02 04:40:18, raymes wrote: > On 2017/03/02 04:19:28, benwells wrote: > > On 2017/03/02 03:04:04, raymes wrote: > > > I think we will need to change CanSitesRequestLocationPermission? Right now > it > > > will return false if the system permission isn't granted. But we will want > it > > to > > > return true in that case, as long as the system permission can be > /requested/. > > > We will also need to incorporate the backoff into that check. > > > > > > If we do that, then we won't need to check those values later on, we will > just > > > show the prompt if it's needed. > > > > > > Also, for GetPermissionStatus() we will need to split up those checks to > give > > > the right result. Something like: > > > if (has system permission AND has chrome permission AND has content setting) > > > return allow; > > > > > > if (can request system permission AND can request chrome permission AND > > content > > > setting is ask) > > > return prompt; > > > > > > return block; > > > > > > Note that the definition of "can request system permission" is different to > > what > > > is currently implemented in CanPromptToEnableSystemLocationSetting. > > > CanPromptToEnableSystemLocationSetting will currently return false if > location > > > is already enabled, whereas in the above code it would need to return true > to > > > work properly. > > > > Yep ... you're right - this won't work. > > > > I think it's confusing the the check for whether location is on embedded in > the > > CanPromptToEnable... call. Also this CanSitesRequestLocationPermission is > > confusing too, I think it is confusing with 'Request' in the name - what if it > > doesnt' need to request? > > > > Instead we could have the following functions on LocationSettings: > > - IsLocationAccessPossible > > - would be true if location is enabled on the device (at device or chrome > > permission level), or it isn't and the prompts required to enabled it can be > > shown. Would take into account backoff, so would have SEARCH or DEFAULT as a > > parameter > > - we'd call that in RequestPermission and block early if it is false > > Sounds good to me. I think we would still need to split this bit up to make it > possible to distinguish between having granted each of the permissions and > whether a prompt will be shown in order to implement GetPermissionStatus. We can > leave that until a subsequent patch though, but just making a note in case it > would make it easier to implement things like that initially. In GetPermissoinStatus, if the content setting is ALLOW, we would: if IsLocationAccessPossible is false, return BLOCK if ShouldShowLocationSettingsDialog is true, return PROMPT if PermissionUpdateInfoBarDelegate::ShouldShowPermissionInfobar is true, return PROMPT. Would that work? > > > - ShouldShowLocationSettingsDialog > > - returns true if system permission is off and the prompt can be shown > (incl. > > backoff) > > - would be called in NotifyPermissionSet if the content setting is ALLOW > > - ShowLocationSettingsDialog > > - resets backoff and shows the dialog, called in NotifyPermissionSet > > > > I think all of this can happen in the C++ with the Java side (i.e. qfiard's > > patch that this depends on) staying the same. > > > > WDYT? > > > > qfiard - WDYT as well? >
I'm not sure - it's really complicated. 1) In order to return ALLOW we need all of : -Content setting == allow -Chrome permission == allow -System permission == allow 2) In order to return ask we need all of: -Content setting != block -IsLocationAccessPossible (which is equivalent to chrome permission != block and system permission != block) 3) In order to return block we would need: -content setting == block or -!IsLocationAccessPossible (which is equivalent to chrome permission == block or system permission == block) Do you agree with the above? I'm not sure how to determine (1) using the current APIs, except possibly by deduction. But deduction feels bug-prone here :P On Thu, 2 Mar 2017 at 15:48 <benwells@chromium.org> wrote: https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:85: if (!location_settings_->CanSitesRequestLocationPermission(web_contents)) { On 2017/03/02 04:40:18, raymes wrote: > On 2017/03/02 04:19:28, benwells wrote: > > On 2017/03/02 03:04:04, raymes wrote: > > > I think we will need to change CanSitesRequestLocationPermission? Right now > it > > > will return false if the system permission isn't granted. But we will want > it > > to > > > return true in that case, as long as the system permission can be > /requested/. > > > We will also need to incorporate the backoff into that check. > > > > > > If we do that, then we won't need to check those values later on, we will > just > > > show the prompt if it's needed. > > > > > > Also, for GetPermissionStatus() we will need to split up those checks to > give > > > the right result. Something like: > > > if (has system permission AND has chrome permission AND has content setting) > > > return allow; > > > > > > if (can request system permission AND can request chrome permission AND > > content > > > setting is ask) > > > return prompt; > > > > > > return block; > > > > > > Note that the definition of "can request system permission" is different to > > what > > > is currently implemented in CanPromptToEnableSystemLocationSetting. > > > CanPromptToEnableSystemLocationSetting will currently return false if > location > > > is already enabled, whereas in the above code it would need to return true > to > > > work properly. > > > > Yep ... you're right - this won't work. > > > > I think it's confusing the the check for whether location is on embedded in > the > > CanPromptToEnable... call. Also this CanSitesRequestLocationPermission is > > confusing too, I think it is confusing with 'Request' in the name - what if it > > doesnt' need to request? > > > > Instead we could have the following functions on LocationSettings: > > - IsLocationAccessPossible > > - would be true if location is enabled on the device (at device or chrome > > permission level), or it isn't and the prompts required to enabled it can be > > shown. Would take into account backoff, so would have SEARCH or DEFAULT as a > > parameter > > - we'd call that in RequestPermission and block early if it is false > > Sounds good to me. I think we would still need to split this bit up to make it > possible to distinguish between having granted each of the permissions and > whether a prompt will be shown in order to implement GetPermissionStatus. We can > leave that until a subsequent patch though, but just making a note in case it > would make it easier to implement things like that initially. In GetPermissoinStatus, if the content setting is ALLOW, we would: if IsLocationAccessPossible is false, return BLOCK if ShouldShowLocationSettingsDialog is true, return PROMPT if PermissionUpdateInfoBarDelegate::ShouldShowPermissionInfobar is true, return PROMPT. Would that work? > > > - ShouldShowLocationSettingsDialog > > - returns true if system permission is off and the prompt can be shown > (incl. > > backoff) > > - would be called in NotifyPermissionSet if the content setting is ALLOW > > - ShowLocationSettingsDialog > > - resets backoff and shows the dialog, called in NotifyPermissionSet > > > > I think all of this can happen in the C++ with the Java side (i.e. qfiard's > > patch that this depends on) staying the same. > > > > WDYT? > > > > qfiard - WDYT as well? > https://codereview.chromium.org/2721293002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
ShouldShowPermissionInfobar will get us whether the Chrome permission is allowed. We may want to factor out a version that just checks whether a single permission is available for clarity. I think we should possibly add a function which tells us whether the system permission is allowed - I think that could make the code clearer. On Thu, 2 Mar 2017 at 16:46 Raymes Khoury <raymes@chromium.org> wrote: > I'm not sure - it's really complicated. > > 1) In order to return ALLOW we need all of : > -Content setting == allow > -Chrome permission == allow > -System permission == allow > > 2) In order to return ask we need all of: > -Content setting != block > -IsLocationAccessPossible (which is equivalent to chrome permission != > block and system permission != block) > > 3) In order to return block we would need: > -content setting == block or > -!IsLocationAccessPossible (which is equivalent to chrome permission == > block or system permission == block) > > Do you agree with the above? I'm not sure how to determine (1) using the > current APIs, except possibly by deduction. But deduction feels bug-prone > here :P > > On Thu, 2 Mar 2017 at 15:48 <benwells@chromium.org> wrote: > > > > https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... > File > chrome/browser/geolocation/geolocation_permission_context_android.cc > (right): > > > https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... > chrome/browser/geolocation/geolocation_permission_context_android.cc:85: > if > (!location_settings_->CanSitesRequestLocationPermission(web_contents)) { > On 2017/03/02 04:40:18, raymes wrote: > > On 2017/03/02 04:19:28, benwells wrote: > > > On 2017/03/02 03:04:04, raymes wrote: > > > > I think we will need to change CanSitesRequestLocationPermission? > Right now > > it > > > > will return false if the system permission isn't granted. But we > will want > > it > > > to > > > > return true in that case, as long as the system permission can be > > /requested/. > > > > We will also need to incorporate the backoff into that check. > > > > > > > > If we do that, then we won't need to check those values later on, > we will > > just > > > > show the prompt if it's needed. > > > > > > > > Also, for GetPermissionStatus() we will need to split up those > checks to > > give > > > > the right result. Something like: > > > > if (has system permission AND has chrome permission AND has > content setting) > > > > return allow; > > > > > > > > if (can request system permission AND can request chrome > permission AND > > > content > > > > setting is ask) > > > > return prompt; > > > > > > > > return block; > > > > > > > > Note that the definition of "can request system permission" is > different to > > > what > > > > is currently implemented in > CanPromptToEnableSystemLocationSetting. > > > > CanPromptToEnableSystemLocationSetting will currently return false > if > > location > > > > is already enabled, whereas in the above code it would need to > return true > > to > > > > work properly. > > > > > > Yep ... you're right - this won't work. > > > > > > I think it's confusing the the check for whether location is on > embedded in > > the > > > CanPromptToEnable... call. Also this > CanSitesRequestLocationPermission is > > > confusing too, I think it is confusing with 'Request' in the name - > what if it > > > doesnt' need to request? > > > > > > Instead we could have the following functions on LocationSettings: > > > - IsLocationAccessPossible > > > - would be true if location is enabled on the device (at device or > chrome > > > permission level), or it isn't and the prompts required to enabled > it can be > > > shown. Would take into account backoff, so would have SEARCH or > DEFAULT as a > > > parameter > > > - we'd call that in RequestPermission and block early if it is > false > > > > Sounds good to me. I think we would still need to split this bit up to > make it > > possible to distinguish between having granted each of the permissions > and > > whether a prompt will be shown in order to implement > GetPermissionStatus. We can > > leave that until a subsequent patch though, but just making a note in > case it > > would make it easier to implement things like that initially. > > In GetPermissoinStatus, if the content setting is ALLOW, we would: > if IsLocationAccessPossible is false, return BLOCK > if ShouldShowLocationSettingsDialog is true, return PROMPT > if PermissionUpdateInfoBarDelegate::ShouldShowPermissionInfobar is true, > return PROMPT. > > Would that work? > > > > > > > - ShouldShowLocationSettingsDialog > > > - returns true if system permission is off and the prompt can be > shown > > (incl. > > > backoff) > > > - would be called in NotifyPermissionSet if the content setting is > ALLOW > > > - ShowLocationSettingsDialog > > > - resets backoff and shows the dialog, called in > NotifyPermissionSet > > > > > > I think all of this can happen in the C++ with the Java side (i.e. > qfiard's > > > patch that this depends on) staying the same. > > > > > > WDYT? > > > > > > qfiard - WDYT as well? > > > > https://codereview.chromium.org/2721293002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by benwells@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.
Updated based on chats today. PTAL.
benwells@chromium.org changed reviewers: + dominickn@chromium.org
+dominickn as raymes is out today
lgtm https://codereview.chromium.org/2721293002/diff/20001/chrome/browser/android/... File chrome/browser/android/mock_location_settings.h (right): https://codereview.chromium.org/2721293002/diff/20001/chrome/browser/android/... chrome/browser/android/mock_location_settings.h:17: static void SetLocationStatus(bool android, bool system); Nit: can this be has_android_permission and system_geolocation_enabled? https://codereview.chromium.org/2721293002/diff/20001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/20001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:252: content::WebContents* web_contents = I think you should null-check web_contents here, since we could have a race condition where web_contents is in the process of being closed when this callback is invoked. content::RenderFrameHost::FromID can return nullptr and WebContentsUserData::FromWebContents will do a null-dereference if given a null pointer.
I only looked at the implementation in the permission context. It looks much better now, thanks so much for breaking up those checks!! I left some notes for future work but it looks good from my brief look. Don't wait for my lg. https://codereview.chromium.org/2721293002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:231: } nit: (for future thought) Since the user already clicked 'allow' we might actually want to store the original content setting, so that they won't be reprompted for that site next time. They might still get the LSD/Android prompt the next time but at least they won't get the chrome one. Also, we might want to move the codepaths together for requesting LSD and Chrome permission. Right now they happen in different places in the flow, but it might simplify assumptions to make them happen at the same point. We might also want to consider how these changes will affect some of the metrics we're currently recording in PermissionContextBase. For example I think right now this will record "PermissionGranted" even if they deny the LSD. Maybe that's what we want. I'm not sure if we use those metrics anyway though and maybe we should consider removing them..
The CQ bit was checked by benwells@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...
benwells@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc for chrome/browser/android and chrome/android owners review https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:159: base::Bind( On 2017/03/01 20:38:49, qfiard wrote: > You will need to switch to base::BindOnce since I switched to use OnceCallbacks. Done. https://codereview.chromium.org/2721293002/diff/20001/chrome/browser/android/... File chrome/browser/android/mock_location_settings.h (right): https://codereview.chromium.org/2721293002/diff/20001/chrome/browser/android/... chrome/browser/android/mock_location_settings.h:17: static void SetLocationStatus(bool android, bool system); On 2017/03/03 00:08:23, dominickn wrote: > Nit: can this be has_android_permission and system_geolocation_enabled? Done. https://codereview.chromium.org/2721293002/diff/20001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/20001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:252: content::WebContents* web_contents = On 2017/03/03 00:08:23, dominickn wrote: > I think you should null-check web_contents here, since we could have a race > condition where web_contents is in the process of being closed when this > callback is invoked. content::RenderFrameHost::FromID can return nullptr and > WebContentsUserData::FromWebContents will do a null-dereference if given a null > pointer. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2721293002/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:150: callback, persist, content_setting)); for my own knowledge if the weakptr is dead by the time this comes back, will the callback be called? just deleted? https://codereview.chromium.org/2721293002/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:250: if (requesting_origin == embedding_origin) { Again curious, but should we only be showing this if it is not CONTENT_SETTING_BLOCK?
https://codereview.chromium.org/2721293002/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:150: callback, persist, content_setting)); On 2017/03/03 19:37:26, Ted C wrote: > for my own knowledge if the weakptr is dead by the time this comes back, will > the callback be called? just deleted? The callback won't be called, it will just deleted / cleaned up. This is what weak pointers are good for :) https://codereview.chromium.org/2721293002/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:250: if (requesting_origin == embedding_origin) { On 2017/03/03 19:37:26, Ted C wrote: > Again curious, but should we only be showing this if it is not > CONTENT_SETTING_BLOCK? The code in MaybeShowDisclosure calls back into the SearchGeolocationService, which checks the content setting and the DSE setting. Note this code hasn't changed in this CL, just moved.
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qfiard@google.com, dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2721293002/#ps60001 (title: "Feedback")
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": 60001, "attempt_start_ts": 1488583159803140,
"parent_rev": "366bdf31d06b91b2a79a98c444e063057f92de34", "commit_rev":
"e962df5122998010c9db55d0fb46993f058e7db4"}
Message was sent while issue was closed.
Description was changed from ========== Show the Android Location Settings Dialog when sites want permission. The Location Settings Dialog (LSD) will be shown when sites which have location permission request location, but the device has location switched off. BUG=673201 ========== to ========== Show the Android Location Settings Dialog when sites want permission. The Location Settings Dialog (LSD) will be shown when sites which have location permission request location, but the device has location switched off. BUG=673201 Review-Url: https://codereview.chromium.org/2721293002 Cr-Commit-Position: refs/heads/master@{#454708} Committed: https://chromium.googlesource.com/chromium/src/+/e962df5122998010c9db55d0fb46... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e962df5122998010c9db55d0fb46...
Message was sent while issue was closed.
https://codereview.chromium.org/2721293002/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2721293002/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:150: callback, persist, content_setting)); On 2017/03/03 23:19:13, benwells wrote: > On 2017/03/03 19:37:26, Ted C wrote: > > for my own knowledge if the weakptr is dead by the time this comes back, will > > the callback be called? just deleted? > > The callback won't be called, it will just deleted / cleaned up. This is what > weak pointers are good for :) And I guess if a caller wanted to ensure their callback was "always" called, they'd need to override the destructor and clean up there. Only applicable if their timeline was longer than the geolocation permission context. |
