Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627853002:
Show the search geolocation disclosure from geolocation API use. (Closed)
|
Created:
3 years, 11 months ago by benwells Modified:
3 years, 10 months ago CC:
chromium-reviews, awdf+watch_chromium.org, mlamouri+watch-geolocation_chromium.org, Peter Beverloo, mlamouri+watch-notifications_chromium.org, Michael van Ouwerkerk, mlamouri+watch-permissions_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow the search geolocation disclosure from geolocation API use.
The new search geolocation system will automatically grant geolocation
API access to the default search engine. This change shows the
disclosure UI when this happens so the user is aware of it.
This change also fixes a bug with showing the API introduced by an
earlier change that prevented the disclosure from being shown on
omnibox queries.
BUG=674398
Review-Url: https://codereview.chromium.org/2627853002
Cr-Commit-Position: refs/heads/master@{#443553}
Committed: https://chromium.googlesource.com/chromium/src/+/b3c6a28850a816162c59526005665c683ca42308
Patch Set 1 #Patch Set 2 : Fix ChromeOS #
Total comments: 10
Patch Set 3 : Feedback and rebase (sorry) #
Total comments: 6
Patch Set 4 : Feedback; use more of the new stuff #
Total comments: 2
Patch Set 5 : Feedback #Patch Set 6 : Fix tests #
Total comments: 1
Patch Set 7 : Rebase #Patch Set 8 : Marked old ones obsolete #
Messages
Total messages: 35 (22 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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: + raymes@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/android/... File chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:167: // default (i.e. has not been explicitly set or revoked). Do you want to check if it's set to ASK or to the default? The default could potentially be ASK, ALLOW, BLOCK https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:172: if (!(status == CONTENT_SETTING_DEFAULT || status == CONTENT_SETTING_ASK)) I don't think DEFAULT will ever be returned here https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:116: web_contents, id, requesting_origin, embedding_origin, callback, persist, You can actually get at the web_contents from PermissionRequestID using content::WebContents::FromRenderFrameHost( content::RenderFrameHost::FromID(id.render_process_id(), id.render_frame_id())); With that said, I don't feel strongly against specifically passing it in. https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:125: // consistent with the content setting. Is this comment needed/accurate?
https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:116: web_contents, id, requesting_origin, embedding_origin, callback, persist, On 2017/01/12 00:17:12, raymes wrote: > You can actually get at the web_contents from PermissionRequestID using > content::WebContents::FromRenderFrameHost( > content::RenderFrameHost::FromID(id.render_process_id(), > id.render_frame_id())); > > With that said, I don't feel strongly against specifically passing it in. Ah ... that would make this change a lot simpler. Do you have a preference?
https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:116: web_contents, id, requesting_origin, embedding_origin, callback, persist, On 2017/01/12 01:04:17, benwells wrote: > On 2017/01/12 00:17:12, raymes wrote: > > You can actually get at the web_contents from PermissionRequestID using > > content::WebContents::FromRenderFrameHost( > > content::RenderFrameHost::FromID(id.render_process_id(), > > id.render_frame_id())); > > > > With that said, I don't feel strongly against specifically passing it in. > > Ah ... that would make this change a lot simpler. Do you have a preference? I would lean toward not passing it and just grabbing it from the ID.
https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/android/... File chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:167: // default (i.e. has not been explicitly set or revoked). On 2017/01/12 00:17:12, raymes wrote: > Do you want to check if it's set to ASK or to the default? The default could > potentially be ASK, ALLOW, BLOCK Done. https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:172: if (!(status == CONTENT_SETTING_DEFAULT || status == CONTENT_SETTING_ASK)) On 2017/01/12 00:17:12, raymes wrote: > I don't think DEFAULT will ever be returned here Done. https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:116: web_contents, id, requesting_origin, embedding_origin, callback, persist, On 2017/01/12 01:41:17, raymes wrote: > On 2017/01/12 01:04:17, benwells wrote: > > On 2017/01/12 00:17:12, raymes wrote: > > > You can actually get at the web_contents from PermissionRequestID using > > > content::WebContents::FromRenderFrameHost( > > > content::RenderFrameHost::FromID(id.render_process_id(), > > > id.render_frame_id())); > > > > > > With that said, I don't feel strongly against specifically passing it in. > > > > Ah ... that would make this change a lot simpler. Do you have a preference? > > I would lean toward not passing it and just grabbing it from the ID. Done. https://codereview.chromium.org/2627853002/diff/20001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:125: // consistent with the content setting. On 2017/01/12 00:17:12, raymes wrote: > Is this comment needed/accurate? Done.
https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/android/... File chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/android/... chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:88: ShowDisclosureIfRulesPermit(gurl); I guess we don't want to check the android permission in this case? I don't know this code well. https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/android/... chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:219: "GeolocationDisclosure.PreDisclosureContentSetting", Do we need to update histograms.xml? https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:122: if (requesting_origin == embedding_origin) { Should we only do this if content_setting is ALLOW?
https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/android/... File chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/android/... chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:88: ShowDisclosureIfRulesPermit(gurl); On 2017/01/12 05:28:14, raymes wrote: > I guess we don't want to check the android permission in this case? I don't know > this code well. I thought about this last night and deliberately didn't check ... but I now think that's wrong and don't know why I thought that... https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/android/... chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:219: "GeolocationDisclosure.PreDisclosureContentSetting", On 2017/01/12 05:28:14, raymes wrote: > Do we need to update histograms.xml? Done. https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2627853002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:122: if (requesting_origin == embedding_origin) { On 2017/01/12 05:28:14, raymes wrote: > Should we only do this if content_setting is ALLOW? There is already logic in the TabHelper checking the content setting and deciding whether to show the disclosure there. I'd like to keep that logic in one place. But, now that you mention it that logic needs to be updated to check teh DSE setting. It will currently show any time the content setting is ASK, even if the DSE setting is off.
lgtm https://codereview.chromium.org/2627853002/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2627853002/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:133: ->MaybeShowDisclosureForAPIUsage(requesting_origin); Since the origin is already being passed in here and all the other checks are happening in MaybeShowDisclosureForAPIUsage, why not also move the search_helper && search_helper->UseDSEGeolocationSetting in there too? That would simplify this function.
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by 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.
benwells@chromium.org changed reviewers: + isherman@chromium.org, sky@chromium.org
+sky for chrome/ owners +isherman for histograms https://codereview.chromium.org/2627853002/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2627853002/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:133: ->MaybeShowDisclosureForAPIUsage(requesting_origin); On 2017/01/12 06:29:16, raymes wrote: > Since the origin is already being passed in here and all the other checks are > happening in MaybeShowDisclosureForAPIUsage, why not also move the > search_helper && search_helper->UseDSEGeolocationSetting in there too? That > would simplify this function. Great idea! That simplifies this and the tab helper as well :)
I'm not a good reviewer for these files. Could you update OWNERS with reasonable owners?
Metrics LGTM % a comment: https://codereview.chromium.org/2627853002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2627853002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19827: + enum="ContentSetting"> It looks like these are replacing some previously defined histograms. Could you please mark those as <obsolete>?
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, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2627853002/#ps140001 (title: "Marked old ones obsolete")
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": 140001, "attempt_start_ts": 1484313295989370,
"parent_rev": "c3807d52dacc45aa76d0d0f23ddda973e3698288", "commit_rev":
"b3c6a28850a816162c59526005665c683ca42308"}
Message was sent while issue was closed.
Description was changed from ========== Show the search geolocation disclosure from geolocation API use. The new search geolocation system will automatically grant geolocation API access to the default search engine. This change shows the disclosure UI when this happens so the user is aware of it. This change also fixes a bug with showing the API introduced by an earlier change that prevented the disclosure from being shown on omnibox queries. BUG=674398 ========== to ========== Show the search geolocation disclosure from geolocation API use. The new search geolocation system will automatically grant geolocation API access to the default search engine. This change shows the disclosure UI when this happens so the user is aware of it. This change also fixes a bug with showing the API introduced by an earlier change that prevented the disclosure from being shown on omnibox queries. BUG=674398 Review-Url: https://codereview.chromium.org/2627853002 Cr-Commit-Position: refs/heads/master@{#443553} Committed: https://chromium.googlesource.com/chromium/src/+/b3c6a28850a816162c5952600566... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/b3c6a28850a816162c5952600566... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
