Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
356543003:
Audit the last usage of Geolocation and Notification permissions. (Closed)
|
Created:
6 years, 6 months ago by Daniel Nishi Modified:
6 years, 5 months ago CC:
chromium-reviews, markusheintz_, scheib Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAudit the last time the Geolocation and Notification content settings
have been used.
This will be used as part of a resource/permission manager which will allow users to more easily see and modify which permissions are being granted to which websites.
Design Doc: https://docs.google.com/document/d/1oQwmj3AU4QYhTyGrYEGr6zaZhHUfx-wqUgEcQGbUU-U/edit?usp=sharing
BUG=372607
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283909
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Added tests and a few more methods. #
Total comments: 5
Patch Set 3 : Improved encapsulation. #
Total comments: 6
Patch Set 4 : Add auditing for actual usage, too. #
Total comments: 20
Patch Set 5 : Addressing comments and a multi-frame unit test. #
Total comments: 2
Patch Set 6 : Traverse the parent frames. #
Total comments: 2
Patch Set 7 : #Patch Set 8 : Update the utils. #
Total comments: 34
Patch Set 9 : Clean up. #
Total comments: 8
Patch Set 10 : Jam nits. #
Total comments: 4
Messages
Total messages: 39 (0 generated)
dewittj: PTAL @ c/b/notifications/desktop_notification_service.cc mvanouwerkerk: PTAL @ c/b/geolocation/geolocation_permission_context.cc markusheintz: PTAL @ c/b/content_settings/* Thanks!
https://codereview.chromium.org/356543003/diff/60001/chrome/browser/geolocati... File chrome/browser/geolocation/geolocation_permission_context.cc (right): https://codereview.chromium.org/356543003/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_permission_context.cc:208: profile_->GetHostContentSettingsMap()->UpdateLastUsage( Why is this only called for this one case? If you wanted to instrument all cases from a single place, NotifyPermissionSet seems suitable.
https://codereview.chromium.org/356543003/diff/60001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/356543003/diff/60001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.h:202: void UpdateLastUsage(const GURL& primary_url, needs comment https://codereview.chromium.org/356543003/diff/60001/chrome/browser/notificat... File chrome/browser/notifications/desktop_notification_service.cc (right): https://codereview.chromium.org/356543003/diff/60001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.cc:436: profile_->GetHostContentSettingsMap()->UpdateLastUsage( Why isn't this just done inside SetContentSetting? https://codereview.chromium.org/356543003/diff/60001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.cc:518: if (setting == CONTENT_SETTING_ALLOW) { why only content setting allow? This is probably better added to GetContentSetting or down farther in the host content settings map.
Added some more tests to make sure the functionality was working. https://codereview.chromium.org/356543003/diff/60001/chrome/browser/geolocati... File chrome/browser/geolocation/geolocation_permission_context.cc (right): https://codereview.chromium.org/356543003/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_permission_context.cc:208: profile_->GetHostContentSettingsMap()->UpdateLastUsage( On 2014/06/26 10:26:39, Michael van Ouwerkerk wrote: > Why is this only called for this one case? If you wanted to instrument all cases > from a single place, NotifyPermissionSet seems suitable. NotifyPermissionSet doesn't have a reference to |embedder| which is used as the secondary pattern. This handles the case that there is no interactivity on allowing the permission. It could also be in PermissionDecided, but this avoids an if-statement on |allowed|. The interactive case is handled by the update also occurring in the HostContentSettingsMap's |SetContentSetting|. https://codereview.chromium.org/356543003/diff/60001/chrome/browser/notificat... File chrome/browser/notifications/desktop_notification_service.cc (right): https://codereview.chromium.org/356543003/diff/60001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.cc:436: profile_->GetHostContentSettingsMap()->UpdateLastUsage( On 2014/06/26 15:56:51, dewittj wrote: > Why isn't this just done inside SetContentSetting? Good point. I've moved this part into SetContentSetting. https://codereview.chromium.org/356543003/diff/60001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.cc:518: if (setting == CONTENT_SETTING_ALLOW) { On 2014/06/26 15:56:51, dewittj wrote: > why only content setting allow? This is probably better added to > GetContentSetting or down farther in the host content settings map. It's on allow because we're trying to track when the permission is actually given, rather than just when it is requested. Checking it here handles the non-interactive case. GetContentSetting makes |this| const, so I didn't want to modify anything in that call. Would adding a non-const secondary getter that also logged the last usage be preferable?
https://codereview.chromium.org/356543003/diff/80001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/356543003/diff/80001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.cc:305: UpdateLastUsageByPattern(primary_pattern, secondary_pattern, content_type); why is it only these two types? https://codereview.chromium.org/356543003/diff/80001/chrome/browser/notificat... File chrome/browser/notifications/desktop_notification_service.cc (right): https://codereview.chromium.org/356543003/diff/80001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.cc:516: if (setting == CONTENT_SETTING_ALLOW) { I wonder if this would be better done in ChromeContentBrowserClient (if you are only interested in web notifications). https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
https://codereview.chromium.org/356543003/diff/80001/chrome/browser/notificat... File chrome/browser/notifications/desktop_notification_service.cc (right): https://codereview.chromium.org/356543003/diff/80001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.cc:516: if (setting == CONTENT_SETTING_ALLOW) { In other words, I'm wary of a special exception right here instead of an API for content settings that's going to guarantee that the right metrics are recorded.
I would like to understand better why 'last usage' means 'most recently permitted' here. Isn't it also interesting to find a recently denied permission so that the user can confirm his action had an effect, and has an easy path to revoking that decision? Basically, I'm asking for a design document of this feature. Do you have one? The bug is not very informative.
I've updated the description with a link to the design doc (https://docs.google.com/document/d/1oQwmj3AU4QYhTyGrYEGr6zaZhHUfx-wqUgEcQGbUU...). https://codereview.chromium.org/356543003/diff/80001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/356543003/diff/80001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.cc:305: UpdateLastUsageByPattern(primary_pattern, secondary_pattern, content_type); On 2014/06/26 23:17:47, dewittj wrote: > why is it only these two types? At the time of the patch, these are the two types that were being audited for the page. It could be blanket, but then we'd have extra data that probably won't be accessed being stored for certain permissions. https://codereview.chromium.org/356543003/diff/80001/chrome/browser/notificat... File chrome/browser/notifications/desktop_notification_service.cc (right): https://codereview.chromium.org/356543003/diff/80001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.cc:516: if (setting == CONTENT_SETTING_ALLOW) { On 2014/06/27 02:52:03, dewittj wrote: > In other words, I'm wary of a special exception right here instead of an API for > content settings that's going to guarantee that the right metrics are recorded. I've moved this into the HostContentSettingsMap to avoid having special exceptions.
https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider.h (right): https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.h:111: bool owns_clock_; Always own the clock. Have tests pass ownership. Use scoped_ptr. https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.cc:303: if (content_type == CONTENT_SETTINGS_TYPE_GEOLOCATION || not if they block. https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.cc:322: const std::string& resource_identifier) { DCHECK_CURRENTLY_ON(BrowserThread::UI);
desktop_notification* lgtm
jam: PTAL at chrome/browser/chrome_content_browser_client and content/public/browser/content_browser_client dewittj: PTAL at c/b/notifications/notification_browsertest.cc Michael van Ouwerkerk: PTAL at chrome/browser/geolocation/* markusheintz: PTAL at c/b/content_settings/* https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider.h (right): https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.h:111: bool owns_clock_; On 2014/06/27 17:47:39, scheib wrote: > Always own the clock. Have tests pass ownership. Use scoped_ptr. We are now scoped. https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.cc:303: if (content_type == CONTENT_SETTINGS_TYPE_GEOLOCATION || On 2014/06/27 17:47:39, scheib wrote: > not if they block. Fixed. https://codereview.chromium.org/356543003/diff/100001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.cc:322: const std::string& resource_identifier) { On 2014/06/27 17:47:39, scheib wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); Done.
why do we need to add a notification in ContentBrowserClient? Can't Chrome already get this information through the call to RequestGeolocationPermission? The design doc mentions that Chrome on Android already has this UI; what does it use to get this information?
On 2014/06/30 23:46:51, jam wrote: > why do we need to add a notification in ContentBrowserClient? Can't Chrome > already get this information through the call to RequestGeolocationPermission? > The design doc mentions that Chrome on Android already has this UI; what does it > use to get this information? The RequestGeolocationPermission lets Chrome get the information for permission requests, but not for actual uses of the API. This addition would be for individual uses. As for Android, I think it's accessing the information through something like this: https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... but I can't find the "Advanced -> Website Settings" Android code in the Chromium code base. It looks like it might be very similar to the Browser for Android implementation, though (an almost identical interface exists in the Browser for Android). Regardless, I can't find that plumbing in the C++ codebase, only in the Android Java.
https://codereview.chromium.org/356543003/diff/180001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2154: void ChromeContentBrowserClient::UseContentSettingPermissionHelper( Why do you need this method? It makes one simple call to UpdateLastUsage, why not call that directly? Furthermore, the 'Helper' part of its name seems strange, more suitable for a class name, for which there is precedence. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:304: void UseContentSettingPermissionHelper(Profile* web_contents, s/web_contents/profile/ https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider.cc (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.cc:603: if (settings_dictionary) { Why would this be false? A new one is instantiated if not found. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.h:214: void UpdateLastUsage(const GURL& primary_url, Please document this public method. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context.cc (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context.cc:205: ->GetContentSettingAndMaybeUpdateLastUsage( This doesn't make sense to me. If permission has already been granted by the user (CONTENT_SETTING_ALLOW) then this will update the timestamp for Geolocation usage to now. This does not, however, capture the case in the default block below. And it does not capture actual usage of the API, though one may expect it to be used if permission is being requested. Still it is better to actually track usage in a place where geolocation data is sent to the renderer. https://codereview.chromium.org/356543003/diff/180001/content/browser/geoloca... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/356543003/diff/180001/content/browser/geoloca... content/browser/geolocation/geolocation_dispatcher_host.cc:191: GetContentClient()->browser()->UseContentSettingPermission( If this is meant to track actual successful usage of the Geolocation API then I'd suggest doing that in OnLocationUpdate. That's where location data is sent to the renderers. In fact, you'd want to track usage for each RenderFrame that we send the data to, otherwise the tracking is incomplete. https://codereview.chromium.org/356543003/diff/180001/content/public/browser/... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/356543003/diff/180001/content/public/browser/... content/public/browser/content_browser_client.cc:235: content::WebContents* web_contents, delete content:: https://codereview.chromium.org/356543003/diff/180001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/180001/content/public/browser/... content/public/browser/content_browser_client.h:476: // Informs the content settings host map that a permission has been used. I don't think you can refer to the host map here as it is a chrome layer concept and this file is in the content layer. It's an implementation detail. Maybe something like "Informs the embedder that a permission has been used."
https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.h:209: const GURL& primary_url, Do primary_url and secondary_url correspond to requesting_frame and embedder?
https://codereview.chromium.org/356543003/diff/180001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2154: void ChromeContentBrowserClient::UseContentSettingPermissionHelper( On 2014/07/01 11:02:23, Michael van Ouwerkerk wrote: > Why do you need this method? It makes one simple call to UpdateLastUsage, why > not call that directly? Furthermore, the 'Helper' part of its name seems > strange, more suitable for a class name, for which there is precedence. |UseContentSettingPermission| is called from content/browser and the Profile is part of Chrome, so I had the call use the WebContents. We also call the second function from |ChromeContentBrowserClient::ShowDesktopNotification| which has a Profile and needs up update the last usage as well. Thinking about it, though, since it is just one simple call, this second function could be eliminated. Done. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:304: void UseContentSettingPermissionHelper(Profile* web_contents, On 2014/07/01 11:02:23, Michael van Ouwerkerk wrote: > s/web_contents/profile/ "Helper" is gone now, so effectively done. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider.cc (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.cc:603: if (settings_dictionary) { On 2014/07/01 11:02:23, Michael van Ouwerkerk wrote: > Why would this be false? A new one is instantiated if not found. Good point. The defensive check is unneeded. Removed the if-statement. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.h:209: const GURL& primary_url, On 2014/07/01 12:53:36, Michael van Ouwerkerk wrote: > Do primary_url and secondary_url correspond to requesting_frame and embedder? That's how it seems to work from Geolocations permissions context. There's a bit of a discrepancy in how Geolocation & Midi do this and how Notifications does this. Notifications seems to throw the SecurityOrigin in a GURL and say it's both, whereas Geolocation and Midi make a distinction between requesting_frame and embedder. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.h:214: void UpdateLastUsage(const GURL& primary_url, On 2014/07/01 11:02:23, Michael van Ouwerkerk wrote: > Please document this public method. Done. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context.cc (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context.cc:205: ->GetContentSettingAndMaybeUpdateLastUsage( On 2014/07/01 11:02:23, Michael van Ouwerkerk wrote: > This doesn't make sense to me. If permission has already been granted by the > user (CONTENT_SETTING_ALLOW) then this will update the timestamp for Geolocation > usage to now. This does not, however, capture the case in the default block > below. And it does not capture actual usage of the API, though one may expect it > to be used if permission is being requested. Still it is better to actually > track usage in a place where geolocation data is sent to the renderer. This location only tracks if permission is asked for and granted (because it was set in the past). I was thinking that the user could be interested in who was asking for permission, even if they haven't used it yet. This isn't the one trying to track the usage, the |OnStartUpdating| part was supposed to do that, but I'm moving it to OnLocationUpdate by your suggestion. As for the default block, that gets captured if they choose Allow on the request when setting the permission in HostContentSettingsMap. https://codereview.chromium.org/356543003/diff/180001/content/browser/geoloca... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/356543003/diff/180001/content/browser/geoloca... content/browser/geolocation/geolocation_dispatcher_host.cc:191: GetContentClient()->browser()->UseContentSettingPermission( On 2014/07/01 11:02:23, Michael van Ouwerkerk wrote: > If this is meant to track actual successful usage of the Geolocation API then > I'd suggest doing that in OnLocationUpdate. That's where location data is sent > to the renderers. In fact, you'd want to track usage for each RenderFrame that > we send the data to, otherwise the tracking is incomplete. I've moved the call into OnLocationUpdate in the for loop to grab all RenderFrameHosts. Upon further inspection, I noticed that the |requesting_frame| coming into OnStartUpdating actually is just a GURL() without anything in it, anyway. https://codereview.chromium.org/356543003/diff/180001/content/public/browser/... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/356543003/diff/180001/content/public/browser/... content/public/browser/content_browser_client.cc:235: content::WebContents* web_contents, On 2014/07/01 11:02:24, Michael van Ouwerkerk wrote: > delete content:: Done. https://codereview.chromium.org/356543003/diff/180001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/180001/content/public/browser/... content/public/browser/content_browser_client.h:476: // Informs the content settings host map that a permission has been used. On 2014/07/01 11:02:24, Michael van Ouwerkerk wrote: > I don't think you can refer to the host map here as it is a chrome layer concept > and this file is in the content layer. It's an implementation detail. Maybe > something like "Informs the embedder that a permission has been used." Done.
https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.h:209: const GURL& primary_url, On 2014/07/01 18:37:48, Daniel Nishi wrote: > On 2014/07/01 12:53:36, Michael van Ouwerkerk wrote: > > Do primary_url and secondary_url correspond to requesting_frame and embedder? > > That's how it seems to work from Geolocations permissions context. There's a bit > of a discrepancy in how Geolocation & Midi do this and how Notifications does > this. > > Notifications seems to throw the SecurityOrigin in a GURL and say it's both, > whereas Geolocation and Midi make a distinction between requesting_frame and > embedder. Right. It's for distinguishing between iframes and top level frames. We'll need to sort out the terminology for this across chrome, but that's for a later cl, probably for someone else. https://codereview.chromium.org/356543003/diff/180001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context.cc (right): https://codereview.chromium.org/356543003/diff/180001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context.cc:205: ->GetContentSettingAndMaybeUpdateLastUsage( On 2014/07/01 18:37:48, Daniel Nishi wrote: > On 2014/07/01 11:02:23, Michael van Ouwerkerk wrote: > > This doesn't make sense to me. If permission has already been granted by the > > user (CONTENT_SETTING_ALLOW) then this will update the timestamp for > Geolocation > > usage to now. This does not, however, capture the case in the default block > > below. And it does not capture actual usage of the API, though one may expect > it > > to be used if permission is being requested. Still it is better to actually > > track usage in a place where geolocation data is sent to the renderer. > > This location only tracks if permission is asked for and granted (because it was > set in the past). I was thinking that the user could be interested in who was > asking for permission, even if they haven't used it yet. > > This isn't the one trying to track the usage, the |OnStartUpdating| part was > supposed to do that, but I'm moving it to OnLocationUpdate by your suggestion. > > As for the default block, that gets captured if they choose Allow on the request > when setting the permission in HostContentSettingsMap. Cool, thanks. https://codereview.chromium.org/356543003/diff/200001/content/browser/geoloca... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/356543003/diff/200001/content/browser/geoloca... content/browser/geolocation/geolocation_dispatcher_host.cc:138: i->first->GetLastCommittedURL().GetOrigin(), If I understand correctly, this should be the url of the top level frame, in case this is an iframe. If that is true, I think you'll need to traverse the parent frames until you find the top level frame.
sorry I just saw your reply, feel free to IM me in the future if I don't respond in a few hours. On 2014/07/01 00:46:11, Daniel Nishi wrote: > On 2014/06/30 23:46:51, jam wrote: > > why do we need to add a notification in ContentBrowserClient? Can't Chrome > > already get this information through the call to RequestGeolocationPermission? > > The design doc mentions that Chrome on Android already has this UI; what does > it > > use to get this information? > > The RequestGeolocationPermission lets Chrome get the information for permission > requests, but not for actual uses of the API. This addition would be for > individual uses. > > As for Android, I think it's accessing the information through something like > this: > https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... > but I can't find the "Advanced -> Website Settings" Android code in the Chromium > code base. It looks like it might be very similar to the Browser for Android > implementation, though (an almost identical interface exists in the Browser for > Android). Regardless, I can't find that plumbing in the C++ codebase, only in > the Android Java. It would be good to figure out how the Android browser currently figures out that this geolocation access happened. I say that because as long as its not breaking the content DEPS rules (i.e. accessing code from content\browser), then the way that it's figuring out that geolocation access is happening is what we'd want here. can you look into this and figure out how Android gets this notification now?
@jam I think I may have miscommunicated. The Android browser doesn't seem to track actual uses, it only brings the origins together into a single list with their permissions. https://codereview.chromium.org/356543003/diff/200001/content/browser/geoloca... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/356543003/diff/200001/content/browser/geoloca... content/browser/geolocation/geolocation_dispatcher_host.cc:138: i->first->GetLastCommittedURL().GetOrigin(), On 2014/07/02 18:51:15, Michael van Ouwerkerk wrote: > If I understand correctly, this should be the url of the top level frame, in > case this is an iframe. If that is true, I think you'll need to traverse the > parent frames until you find the top level frame. Done. We traverse the parents now.
Thanks Dan. Getting there :-) https://codereview.chromium.org/356543003/diff/220001/content/browser/geoloca... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/356543003/diff/220001/content/browser/geoloca... content/browser/geolocation/geolocation_dispatcher_host.cc:141: topFrame->GetLastCommittedURL().GetOrigin(), Sorry to keep going on about this, but from what I understand we try to distinguish the requesting frame from the top level frame. So this line should be the requesting frame, the next line should be the top level frame.
https://codereview.chromium.org/356543003/diff/220001/content/browser/geoloca... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/356543003/diff/220001/content/browser/geoloca... content/browser/geolocation/geolocation_dispatcher_host.cc:141: topFrame->GetLastCommittedURL().GetOrigin(), On 2014/07/08 17:02:18, Michael van Ouwerkerk wrote: > Sorry to keep going on about this, but from what I understand we try to > distinguish the requesting frame from the top level frame. So this line should > be the requesting frame, the next line should be the top level frame. I think I understand now. So this change here should be appropriate?
lgtm Thanks Daniel!
@markuscheintz_: Ping, PTAL at chrome/browser/content_settings/*
+bauerb PTAL at c/b/content_settings/* when you get a moment.
https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:18: #if defined(OS_ANDROID) While you're here, this file could use some love... (yay yak-shaving!) This #ifdef seems strange and wrong. There's a scoped_ptr used below unconditionally. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:22: class Profile; This is unnecessary. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:314: const GURL& url, Needs to be indented two more spaces. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:320: void GuestPermissionRequestHelper( Two more spaces (also below) https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:354: base::WeakPtrFactory<ChromeContentBrowserClient> weak_factory_; One less space. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:356: friend class DisableWebRtcEncryptionFlagTest; friend declarations go right after private: https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider.cc (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.cc:91: clock_(scoped_ptr<base::Clock>(new base::DefaultClock())), The scoped_ptr<>() is unnecessary (a scoped_ptr can be initialized with a raw pointer). https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider.h (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.h:60: virtual void UpdateLastUsage(const ContentSettingsPattern& primary_pattern, Why is this virtual? https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.h:68: // Gains ownership of |clock|. Pass this as a scoped_ptr? https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.cc:353: content_settings_providers_[PREF_PROVIDER]) It might be worth adding a private helper method that will just return the PrefProvider. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.h:233: // Passes ownership of |clock|. Why not use a scoped_ptr then? https://codereview.chromium.org/356543003/diff/260001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:713: ->GetHostContentSettingsMap() Would it make sense to just pull this out into a short local variable like |map| and use that? https://codereview.chromium.org/356543003/diff/260001/chrome/browser/notifica... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/notifica... chrome/browser/notifications/notification_browsertest.cc:790: base::SimpleTestClock* clock_ = new base::SimpleTestClock(); Underscore at the end is for member variables. https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... content/public/browser/content_browser_client.cc:240: return; Return is unneccessary. https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... content/public/browser/content_browser_client.h:477: // Informs the content settings host map that a permission has been used. Informs whom? Content settings host map (actually host content settings map) is not a thing that content/ knows about. https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... content/public/browser/content_browser_client.h:478: virtual void UseContentSettingPermission(WebContents* web_contents, I would probably call this "DidUse..." or something. Right now it reads like an imperative. https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... content/public/browser/content_browser_client.h:481: const std::string& setting_type); This is somewhat weird as a content embedder interface. What is the setting passed here? (Looking at the whole picture, I know that it will be the string "geolocation", but that's a content setting type name defined in chrome/. The fact that you need to repeat the constant inside of GeolocationDispatcherHost should be a bit of a red flag). We also don't actually use this for arbitrary content setting types. TBH, I would just make this a specific method for geolocation. You could also move this to WebContentsObserver. The only reason why other methods around here that take a WebContents are not on WebContentsObserver is because there is one particular object that needs to handle them (to decide about a permission), whereas this is just notifying whoever is interested.
PTAL. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:18: #if defined(OS_ANDROID) On 2014/07/14 10:50:32, Bernhard Bauer wrote: > While you're here, this file could use some love... > > (yay yak-shaving!) > > This #ifdef seems strange and wrong. There's a scoped_ptr used below > unconditionally. Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:22: class Profile; On 2014/07/14 10:50:32, Bernhard Bauer wrote: > This is unnecessary. Removed. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:314: const GURL& url, On 2014/07/14 10:50:32, Bernhard Bauer wrote: > Needs to be indented two more spaces. Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:320: void GuestPermissionRequestHelper( On 2014/07/14 10:50:32, Bernhard Bauer wrote: > Two more spaces (also below) Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:354: base::WeakPtrFactory<ChromeContentBrowserClient> weak_factory_; On 2014/07/14 10:50:32, Bernhard Bauer wrote: > One less space. Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:356: friend class DisableWebRtcEncryptionFlagTest; On 2014/07/14 10:50:32, Bernhard Bauer wrote: > friend declarations go right after private: Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider.cc (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.cc:91: clock_(scoped_ptr<base::Clock>(new base::DefaultClock())), On 2014/07/14 10:50:32, Bernhard Bauer wrote: > The scoped_ptr<>() is unnecessary (a scoped_ptr can be initialized with a raw > pointer). Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider.h (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.h:60: virtual void UpdateLastUsage(const ContentSettingsPattern& primary_pattern, On 2014/07/14 10:50:32, Bernhard Bauer wrote: > Why is this virtual? Whoops. It doesn't have to be. I had it as part of the ProviderInterface implementation, but I pulled it out. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider.h:68: // Gains ownership of |clock|. On 2014/07/14 10:50:32, Bernhard Bauer wrote: > Pass this as a scoped_ptr? Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.cc:353: content_settings_providers_[PREF_PROVIDER]) On 2014/07/14 10:50:32, Bernhard Bauer wrote: > It might be worth adding a private helper method that will just return the > PrefProvider. Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map.h:233: // Passes ownership of |clock|. On 2014/07/14 10:50:32, Bernhard Bauer wrote: > Why not use a scoped_ptr then? Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:713: ->GetHostContentSettingsMap() On 2014/07/14 10:50:32, Bernhard Bauer wrote: > Would it make sense to just pull this out into a short local variable like |map| > and use that? Yep, it does. Done. https://codereview.chromium.org/356543003/diff/260001/chrome/browser/notifica... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/356543003/diff/260001/chrome/browser/notifica... chrome/browser/notifications/notification_browsertest.cc:790: base::SimpleTestClock* clock_ = new base::SimpleTestClock(); On 2014/07/14 10:50:33, Bernhard Bauer wrote: > Underscore at the end is for member variables. Done. https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... content/public/browser/content_browser_client.cc:240: return; On 2014/07/14 10:50:33, Bernhard Bauer wrote: > Return is unneccessary. Done. https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... content/public/browser/content_browser_client.h:477: // Informs the content settings host map that a permission has been used. On 2014/07/14 10:50:33, Bernhard Bauer wrote: > Informs whom? Content settings host map (actually host content settings map) is > not a thing that content/ knows about. Removed reference to chrome concept. https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... content/public/browser/content_browser_client.h:478: virtual void UseContentSettingPermission(WebContents* web_contents, On 2014/07/14 10:50:33, Bernhard Bauer wrote: > I would probably call this "DidUse..." or something. Right now it reads like an > imperative. Done. https://codereview.chromium.org/356543003/diff/260001/content/public/browser/... content/public/browser/content_browser_client.h:481: const std::string& setting_type); On 2014/07/14 10:50:33, Bernhard Bauer wrote: > This is somewhat weird as a content embedder interface. What is the setting > passed here? (Looking at the whole picture, I know that it will be the string > "geolocation", but that's a content setting type name defined in chrome/. The > fact that you need to repeat the constant inside of GeolocationDispatcherHost > should be a bit of a red flag). > > We also don't actually use this for arbitrary content setting types. TBH, I > would just make this a specific method for geolocation. > > You could also move this to WebContentsObserver. The only reason why other > methods around here that take a WebContents are not on WebContentsObserver is > because there is one particular object that needs to handle them (to decide > about a permission), whereas this is just notifying whoever is interested. I'm not sure if WebContentsObserver is the right landing place since the header implies it is for classes interested in page load events, which a permission use doesn't seem to be. It is a good point that it is only used for geolocation. I've changed the method to be specific.
lgtm with nits https://codereview.chromium.org/356543003/diff/300001/content/browser/geoloca... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/356543003/diff/300001/content/browser/geoloca... content/browser/geolocation/geolocation_dispatcher_host.cc:133: RenderFrameHost* topFrame = i->first; nit: top_frame per style guide https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... content/public/browser/content_browser_client.cc:235: void ContentBrowserClient::DidUseGeolocationPermission( nit: don't put this in the cc file, instead just have inline "{}" after the method in the header like other void methods https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... content/public/browser/content_browser_client.h:478: virtual void DidUseGeolocationPermission(WebContents* web_contents, nit: put this at line 456 so it's right after the other geolocation related method https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... content/public/browser/content_browser_client.h:479: const GURL& primary_url, nit: primary and secondary urls is ambiguous. use frame_url and main_frame_url to be clear
https://codereview.chromium.org/356543003/diff/300001/content/browser/geoloca... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/356543003/diff/300001/content/browser/geoloca... content/browser/geolocation/geolocation_dispatcher_host.cc:133: RenderFrameHost* topFrame = i->first; On 2014/07/14 18:20:53, jam wrote: > nit: top_frame per style guide Done. https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... content/public/browser/content_browser_client.cc:235: void ContentBrowserClient::DidUseGeolocationPermission( On 2014/07/14 18:20:53, jam wrote: > nit: don't put this in the cc file, instead just have inline "{}" after the > method in the header like other void methods Done. https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... content/public/browser/content_browser_client.h:478: virtual void DidUseGeolocationPermission(WebContents* web_contents, On 2014/07/14 18:20:53, jam wrote: > nit: put this at line 456 so it's right after the other geolocation related > method Done. https://codereview.chromium.org/356543003/diff/300001/content/public/browser/... content/public/browser/content_browser_client.h:479: const GURL& primary_url, On 2014/07/14 18:20:53, jam wrote: > nit: primary and secondary urls is ambiguous. use frame_url and main_frame_url > to be clear Done.
@bauerb Ping.
Sorry for the delay. LGTM with a suggestion below: https://codereview.chromium.org/356543003/diff/320001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc (right): https://codereview.chromium.org/356543003/diff/320001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:452: base::SimpleTestClock* test_clock = new base::SimpleTestClock; Alternatively, make this a scoped_ptr<base::SimpleTestClock>, then below .PassAs<base::Clock>(). https://codereview.chromium.org/356543003/diff/320001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_browsertest.cc (right): https://codereview.chromium.org/356543003/diff/320001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_browsertest.cc:777: base::SimpleTestClock* clock_ = new base::SimpleTestClock(); Same here. https://codereview.chromium.org/356543003/diff/320001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/356543003/diff/320001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:706: base::SimpleTestClock* test_clock = new base::SimpleTestClock; And here.
https://codereview.chromium.org/356543003/diff/320001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc (right): https://codereview.chromium.org/356543003/diff/320001/chrome/browser/content_... chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:452: base::SimpleTestClock* test_clock = new base::SimpleTestClock; On 2014/07/17 09:11:48, Bernhard Bauer wrote: > Alternatively, make this a scoped_ptr<base::SimpleTestClock>, then below > .PassAs<base::Clock>(). Thanks for the suggestion, but I think I'm going to leave it as is. I'd need to maintain the raw pointer to the test_clock anyway since I need to mutate the clock after Passing it to the PrefProvider.
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/356543003/320001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/356543003/320001
Message was sent while issue was closed.
Change committed as 283909 |
