Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2742373003:
Limit the amount the Location Settings Dialog will be shown to users. (Closed)
|
Created:
3 years, 9 months ago by benwells Modified:
3 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, Michael van Ouwerkerk, mcasas+geolocation_chromium.org, raymes+watch_chromium.org, mlamouri+watch-geolocation_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLimit the amount the Location Settings Dialog will be shown to users.
The Location Settings Dialog (LSD) is shown to users when their device
location is off, but a website tries to use the location via the
geolocation API.
This change introduces a backoff, so that when the user dismisses the
LSD it won't be shown again for a little while. This backof grows - it
starts at one week, then goes to 30 days, then 90 days.
There is a separate backoff for the default search engine, and all other
sites.
BUG=672301
Review-Url: https://codereview.chromium.org/2742373003
Cr-Commit-Position: refs/heads/master@{#458325}
Committed: https://chromium.googlesource.com/chromium/src/+/0b3748a3344c72d94fa98e7bbc7f57be70c65a02
Patch Set 1 #
Total comments: 26
Patch Set 2 : Feedback #Patch Set 3 : Woops, missed one #
Total comments: 1
Patch Set 4 : Nit #
Total comments: 4
Patch Set 5 : Rebase #Patch Set 6 : Feedback #Messages
Total messages: 41 (27 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
benwells@chromium.org changed reviewers: + raymes@chromium.org
there is a little thing to fix (change GURL to char*) but apart from that it's ready to review.
Thanks Ben! All pretty minor comments :) https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:36: bool gHasDSEOriginForTesting = false; Could you just use if (!gDSEOriginForTesting.empty()) instead of adding this variable? https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:37: GURL gDSEOriginForTesting; I think we avoid non-POD globals because it leads to static initializers? You may just want to make this an instance variable? That would avoid test lifetime issues too. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:40: return base::Time::Now() + base::TimeDelta::FromDays(gDayOffsetForTesting); A more standard pattern is to store a base::Clock in a member variable. You can override the clock for testing and then set the time as needed. See, e.g. https://cs.chromium.org/chromium/src/chrome/browser/engagement/site_engagemen... https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:198: // been checked, so check that. nit: maybe explain why the backoff hasn't been checked yet because I think it's subtle. I think it's because the user may have been prompted and so we only really know if we're in backoff after we try to prompt the user. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:236: // If the permission is in the ASK state, backoff is ignored. nit: maybe explain why this is the case here? I think it's because we want it such that if the user sees an infobar and accepts it, then we still display LSD. Basically, if we were going to display an infobar anyway, it's ok to show them the LSD. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:293: void GeolocationPermissionContextAndroid::SetLocationSettingsBackOff( nit: maybe this should be ApplyLocationSettingsBackOff or UpdateLocationSettingsBackOff? https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:337: GeolocationPermissionContextAndroid::GetLocationSettingsDialogContext( nit: I wonder if this is more clearly framed as IsRequestingOriginDSE ? since "Context" is overloaded in this context :) Only one callsite passes on the return value. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.h (right): https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.h:101: bool IsLocationAccessPossible(content::WebContents* web_contents, nit: We might want to explain in a comment somewhere that this doesn't include backoff because access may still be possible even if we're in backoff, so long as the user is asked? https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:392: expected_shown); optional: alternatively you can return HasShownLocationSettingsDialog() here and call EXPECT_TRUE/FALSE at the callsite. It avoids having to document the bools that get passed in. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:689: MockLocationSettings::SetLocationSettingsDialogStatus(true, DENIED); nit: here and elsewhere, please document the boolean for this function https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:724: // First, cancel a LSE prompt on the first non-DSE origin to go into backoff. nit: LSD https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:806: // First, cancel a LSE prompt on the first non-DSE origin to go into backoff. nit: LSD https://codereview.chromium.org/2742373003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2742373003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.h:138: ContentSetting content_setting); Thinking more about why this doesn't feel great to me: I think it's because it's easy for people to break assumptions by overriding this. Specifically, if you don't call back into the base class to get the base class behavior, then you end up breaking all the metrics and dismissal-embargoing behavior. By overriding a function that does nothing at the base level there is no risk of that. What are your thoughts?
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:36: bool gHasDSEOriginForTesting = false; On 2017/03/15 04:28:25, raymes wrote: > Could you just use > if (!gDSEOriginForTesting.empty()) > > instead of adding this variable? Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:37: GURL gDSEOriginForTesting; On 2017/03/15 04:28:25, raymes wrote: > I think we avoid non-POD globals because it leads to static initializers? You > may just want to make this an instance variable? That would avoid test lifetime > issues too. Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:40: return base::Time::Now() + base::TimeDelta::FromDays(gDayOffsetForTesting); On 2017/03/15 04:28:25, raymes wrote: > A more standard pattern is to store a base::Clock in a member variable. You can > override the clock for testing and then set the time as needed. See, e.g. > https://cs.chromium.org/chromium/src/chrome/browser/engagement/site_engagemen... Yep, I know. I think I even added the clock to the site engagement. In this case I thought it was less code to allow a simple day offset. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:198: // been checked, so check that. On 2017/03/15 04:28:25, raymes wrote: > nit: maybe explain why the backoff hasn't been checked yet because I think it's > subtle. I think it's because the user may have been prompted and so we only > really know if we're in backoff after we try to prompt the user. Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:236: // If the permission is in the ASK state, backoff is ignored. On 2017/03/15 04:28:25, raymes wrote: > nit: maybe explain why this is the case here? I think it's because we want it > such that if the user sees an infobar and accepts it, then we still display LSD. > Basically, if we were going to display an infobar anyway, it's ok to show them > the LSD. Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:293: void GeolocationPermissionContextAndroid::SetLocationSettingsBackOff( On 2017/03/15 04:28:25, raymes wrote: > nit: maybe this should be ApplyLocationSettingsBackOff or > UpdateLocationSettingsBackOff? Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.cc:337: GeolocationPermissionContextAndroid::GetLocationSettingsDialogContext( On 2017/03/15 04:28:25, raymes wrote: > nit: I wonder if this is more clearly framed as > IsRequestingOriginDSE ? > > since "Context" is overloaded in this context :) Only one callsite passes on the > return value. Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_android.h (right): https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_android.h:101: bool IsLocationAccessPossible(content::WebContents* web_contents, On 2017/03/15 04:28:25, raymes wrote: > nit: We might want to explain in a comment somewhere that this doesn't include > backoff because access may still be possible even if we're in backoff, so long > as the user is asked? Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:392: expected_shown); On 2017/03/15 04:28:25, raymes wrote: > optional: alternatively you can return HasShownLocationSettingsDialog() here and > call EXPECT_TRUE/FALSE at the callsite. It avoids having to document the bools > that get passed in. Yeah that's much nicer, done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:689: MockLocationSettings::SetLocationSettingsDialogStatus(true, DENIED); On 2017/03/15 04:28:25, raymes wrote: > nit: here and elsewhere, please document the boolean for this function Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:724: // First, cancel a LSE prompt on the first non-DSE origin to go into backoff. On 2017/03/15 04:28:25, raymes wrote: > nit: LSD Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/geolocation/... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:806: // First, cancel a LSE prompt on the first non-DSE origin to go into backoff. On 2017/03/15 04:28:25, raymes wrote: > nit: LSD Done. https://codereview.chromium.org/2742373003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2742373003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.h:138: ContentSetting content_setting); On 2017/03/15 04:28:25, raymes wrote: > Thinking more about why this doesn't feel great to me: I think it's because it's > easy for people to break assumptions by overriding this. Specifically, if you > don't call back into the base class to get the base class behavior, then you end > up breaking all the metrics and dismissal-embargoing behavior. > > By overriding a function that does nothing at the base level there is no risk of > that. What are your thoughts? Done.
lgtm thanks! https://codereview.chromium.org/2742373003/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2742373003/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:36: const char* gDSEOriginForTesting; optional: not sure if you want to explicitly initialize this to nullptr for clarity. I know it will get default-initialized as a global though :)
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.
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2742373003/#ps60001 (title: "Nit")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
benwells@chromium.org changed reviewers: + bauerb@chromium.org, tedchoc@chromium.org
+bauerb for prefs +tedchoc for location settings
LGTM w/ some nits: https://codereview.chromium.org/2742373003/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2742373003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:34: int gDayOffsetForTesting = 0; Nit: Globals are usually named g_underscore_style. https://codereview.chromium.org/2742373003/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.h (right): https://codereview.chromium.org/2742373003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.h:91: std::string LocationSettingsBackOffLevelPref( Nit: From the name it's not totally clear to me what the first two methods do; if you would prepend e.g. Get... here, it might already make it clearer.
ping tedchoc
On 2017/03/19 23:06:03, benwells wrote: > ping tedchoc mock_location_settings - lgtm As a completely bogus excuse for my delay (but a potential suggestion to remove it as an option), I did open this change last week and saw that location settings didn't match exactly either a directory or file name so I wasn't entirely sure what to review (are permission_context* doing location setting "stuff"). In general, I think you only wanted me to basically rubber stamp some trivial changes to two small files (which honestly you could have TBR'd for me anyway), but I got stuck in a bit of decision paralysis about it. But regardless about my excuses, this was my fault and sorry for the delay!!
On 2017/03/20 18:14:54, Ted C wrote: > On 2017/03/19 23:06:03, benwells wrote: > > ping tedchoc > > mock_location_settings - lgtm > > As a completely bogus excuse for my delay (but a potential suggestion to > remove it as an option), I did open this change last week and saw that > location settings didn't match exactly either a directory or file name > so I wasn't entirely sure what to review (are permission_context* doing > location setting "stuff"). > > In general, I think you only wanted me to basically rubber stamp some trivial > changes to two small files (which honestly you could have TBR'd for me anyway), > but I got stuck in a bit of decision paralysis about it. > > But regardless about my excuses, this was my fault and sorry for the delay!! No worries - it's all good!
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.
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, bauerb@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2742373003/#ps100001 (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": 100001, "attempt_start_ts": 1490077401327960,
"parent_rev": "e123782556b8f18f3a070722dd68ab617a930976", "commit_rev":
"0b3748a3344c72d94fa98e7bbc7f57be70c65a02"}
Message was sent while issue was closed.
Description was changed from ========== Limit the amount the Location Settings Dialog will be shown to users. The Location Settings Dialog (LSD) is shown to users when their device location is off, but a website tries to use the location via the geolocation API. This change introduces a backoff, so that when the user dismisses the LSD it won't be shown again for a little while. This backof grows - it starts at one week, then goes to 30 days, then 90 days. There is a separate backoff for the default search engine, and all other sites. BUG=672301 ========== to ========== Limit the amount the Location Settings Dialog will be shown to users. The Location Settings Dialog (LSD) is shown to users when their device location is off, but a website tries to use the location via the geolocation API. This change introduces a backoff, so that when the user dismisses the LSD it won't be shown again for a little while. This backof grows - it starts at one week, then goes to 30 days, then 90 days. There is a separate backoff for the default search engine, and all other sites. BUG=672301 Review-Url: https://codereview.chromium.org/2742373003 Cr-Commit-Position: refs/heads/master@{#458325} Committed: https://chromium.googlesource.com/chromium/src/+/0b3748a3344c72d94fa98e7bbc7f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/0b3748a3344c72d94fa98e7bbc7f...
Message was sent while issue was closed.
(oops, forgot to send this before landing) https://codereview.chromium.org/2742373003/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2742373003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:34: int gDayOffsetForTesting = 0; On 2017/03/16 16:19:34, Bernhard (OOO until Mar 27) wrote: > Nit: Globals are usually named g_underscore_style. Done. https://codereview.chromium.org/2742373003/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.h (right): https://codereview.chromium.org/2742373003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.h:91: std::string LocationSettingsBackOffLevelPref( On 2017/03/16 16:19:35, Bernhard (OOO until Mar 27) wrote: > Nit: From the name it's not totally clear to me what the first two methods do; > if you would prepend e.g. Get... here, it might already make it clearer. Done. |
