Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1104473002:
Make the Location is Allowed link take the Geo Header into account. (Closed)
|
Created:
5 years, 8 months ago by Finnur Modified:
5 years, 7 months ago CC:
chromium-reviews, peter+watch_chromium.org, mlamouri+watch-notifications_chromium.org, vabr (Chromium), battre Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the "Location is Allowed" link take the Geo Header into account.
BUG=479637, 467798
Committed: https://crrev.com/ca5a45d777d9f6ca14a4e41d262268cc5139aace
Cr-Commit-Position: refs/heads/master@{#328110}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Sync to latest #
Total comments: 17
Patch Set 3 : Address feedback #
Total comments: 2
Patch Set 4 : Sync to latest #
Total comments: 1
Patch Set 5 : Address feedback #Patch Set 6 : Sync to latest #
Total comments: 1
Patch Set 7 : Address feedback #
Messages
Total messages: 22 (5 generated)
finnur@chromium.org changed reviewers: + newt@chromium.org
finnur@chromium.org changed reviewers: + mariakhomenko@chromium.org, miguelg@chromium.org
Miguel, can you review: NotificationUIManager.java? Maria, can you review: template_url_service_android.cc? And Newton, can you review the rest? https://codereview.chromium.org/1104473002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1104473002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:229: } These two functions are from the GeoHeader class. Once this is checked in, I intend for the GeoHeader class to use these instead. I chose this file for lack of a better place. Let me know if you have a better idea. https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.cc:256: base::ASCIIToUTF16("query")), SearchTermsData(), nullptr)); This might seem counter-intuitive, but is needed because the functions that verify if a string is a Google search query don't work unless there's an actual query in there. None of the callers care about the precise value returned, so this should have no other effect.
https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.cc:256: base::ASCIIToUTF16("query")), SearchTermsData(), nullptr)); On 2015/04/22 11:12:59, Finnur wrote: > This might seem counter-intuitive, but is needed because the functions that > verify if a string is a Google search query don't work unless there's an actual > query in there. None of the callers care about the precise value returned, so > this should have no other effect. I do think that this behaviour is unexpected. There seems to be a mismatch between what the GeoHeader function is expecting (a fully formed Google search URL) and what we were providing to preferences (a "canonical" form of the URL). It's also not clear to me that the change won't have any effect. It seems like the native code in website_preference_bridge::SetSettingForOrigin may be affected (it's a bit difficult for me to determine). IMO, we should instead fix the check in geo settings to accept this format rather than make this change here.
https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.cc:256: base::ASCIIToUTF16("query")), SearchTermsData(), nullptr)); Hmm... Here's how I see it... I created this function and my code is the only consumer of it. Side note: I don't see how website_preference_bridge::SetSettingForOrigin is related, but if I'm mistaken I'd be interested in your findings there. Anway, the whole purpose of this function was to supply an example search URL for a given template URL but today it provides an *invalid* search URL (because the query part is missing). I am fixing it to provide a valid example URL. Keep in mind that neither version would be used for actual queries. > IMO, we should instead fix the check in geo settings I don't see why/how the geo settings code should change to accommodate an invalid search URL. One could make the argument that the check itself (IsGoogleSearchURL) would have to change. Two problems there: 1) We'd be changing it to say that a malformed search url is valid. 2) this check is implemented in google_util.cc, and that function is used throughout both C++ and Java code, including: voice_search_tab_helper.cc google_search_counter.cc safe_search_util.cc rlz.cc TemplateUrlServiceAndroid::GetUrlForVoiceSearchQuery TemplateUrlServiceAndroid::GetUrlForContextualSearchQuery ... to name a few (probably not an exhaustive list). I don't know if any of these functions *rely* on the check returning false for invalid search urls (that are missing the query part). But given the fact that I'm the only consumer of this function, I'd rather change it and live with breaking my code than change the google_util check and create a much worse problem in some other people's code.
Friendly ping for all three reviewers. I think it is important to fix this for two reasons: 1) When permissions for a site have been reset there is a chance of a miscommunication because the location permission is not specified. We're going to show "Location is blocked" (because there's no permission) but x-geo treats the lack of a permission preference as a reason to send location with Omnibox searches. 2) Clicking on the "Location is allowed/blocked" link today cements a permission decision for the user and that's both non-intuitive and has potentially unwanted and easy to overlook consequences. This CL provides a way to let the user toggle the permission without first creating the permission, meaning that just clicking the "Location is allowed/blocked" link doesn't change any state. Which is how it should be.
On 2015/04/24 15:49:19, Finnur wrote: > https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... > File chrome/browser/search_engines/template_url_service_android.cc (right): > > https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service_android.cc:256: > base::ASCIIToUTF16("query")), SearchTermsData(), nullptr)); > Hmm... Here's how I see it... > > I created this function and my code is the only consumer of it. Side note: I > don't see how website_preference_bridge::SetSettingForOrigin is related, but if > I'm mistaken I'd be interested in your findings there. > > Anway, the whole purpose of this function was to supply an example search URL > for a given template URL but today it provides an *invalid* search URL (because > the query part is missing). I am fixing it to provide a valid example URL. Keep > in mind that neither version would be used for actual queries. > > > IMO, we should instead fix the check in geo settings > > I don't see why/how the geo settings code should change to accommodate an > invalid search URL. One could make the argument that the check itself > (IsGoogleSearchURL) would have to change. Two problems there: > > 1) We'd be changing it to say that a malformed search url is valid. > 2) this check is implemented in google_util.cc, and that function is used > throughout both C++ and Java code, including: > > voice_search_tab_helper.cc > google_search_counter.cc > safe_search_util.cc > rlz.cc > TemplateUrlServiceAndroid::GetUrlForVoiceSearchQuery > TemplateUrlServiceAndroid::GetUrlForContextualSearchQuery > > ... to name a few (probably not an exhaustive list). I don't know if any of > these functions *rely* on the check returning false for invalid search urls > (that are missing the query part). > > But given the fact that I'm the only consumer of this function, I'd rather > change it and live with breaking my code than change the google_util check and > create a much worse problem in some other people's code. So, I was looking at how GetSearchEngineUrlFromTemplateUrl is used. I saw two caller classes: PrefServiceBridge.java and SearchEngineAdapter.java. In both cases the URL is used to a) construct GeolocationInfo object, where url doesn't seem to be really used, but can be accessed via a public accessor, which I haven't been able to trace to uses effectively b) Passed as an argument into WebsitePreferenceBridge.nativeSetGeolocationSettingForOrigin, which in turn calls WebsitePreferenceBridge::SetSettingForOrigin(), which does some URL manipulation with ContentSettingsPattern::FromURLNoWildcard on the URL and saves it into some map. In this code path, it's not clear to me that changing the URL won't result in problems. If you believe it won't, I would appreciate an explanation as to why.
https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:186: * Wheter to send a geo header for the current URL. "Whether the geo header is allowed to be sent for the current URL." https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:191: public static boolean sendGeoHeader(Context context, String url, boolean isIncognito) { sendGeoHeader() is a wildly misleading method name. How about "isAllowedToReceiveGeoHeader()" or "mayReceiveGeoHeader()" or "isGeoHeaderEnabledForUrl()" https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:212: public static boolean isLocationDisabledForUrl(Uri uri) { Add a TODO to delete these methods after GeolocationHeader has been upstreamed. https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:267: ? "allow" : "block")); Use ContentSetting.ALLOW and ContentSetting.BLOCK instead of "allow" and "block" string literals https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:273: private boolean locationEnabled(int position, boolean checkGeoHeader) { suggestion: extract a method getLocationContentSetting() like so: private int getLocationContentSetting(int position) { if (position == -1) return ContentSetting.BLOCK; String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl( toIndex(position)); GeolocationInfo locationSettings = new GeolocationInfo(url, null); return locationSettings.getContentSetting(); } private boolean locationEnabled(int position) { return getLocationContentSetting(position) == ContentSetting.ALLOW; } then when you're creating the Intent to open SingleWebsitePreferences, use getLocationContentSetting() and call PrefServiceBridge.sendGeoHeader() if needed: Bundle fragmentArgs = SingleWebsitePreferences.createFragmentArgsForSite(url); int locationSetting = getLocationContentSetting(mSelectedSearchEnginePosition); if (locationSetting = ContentSetting.ASK) { int locationSettingFallback = PrefServiceBridge.sendGeoHeader(mContext, url, false) ? ContentSetting.ALLOW : ContentSetting.BLOCK); fragmentArgs.putBoolean(SingleWebsitePreferences.EXTRA_LOCATION_SETTING_FALLBACK, locationSettingFallback); } https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:280: // Catch a corner case with the geoHeader being sent when no permission has been specified. This is actually a common case. How about "Handle the case where..." https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:124: public static Bundle createFragmentArgsForSite(String url, String locationValueIfMissing) { Instead of adding a new argument to this method, I'd just add the EXTRA_LOCATION value to the returned Bundle in SearchEngineAdapter.java. https://codereview.chromium.org/1104473002/diff/20001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:256: base::ASCIIToUTF16("query")), SearchTermsData(), nullptr)); why/is this needed?
Addressed comments from both. PTAL. https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/1104473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.cc:256: base::ASCIIToUTF16("query")), SearchTermsData(), nullptr)); > So, I was looking at how GetSearchEngineUrlFromTemplateUrl is used. I saw two > caller classes: > PrefServiceBridge.java and SearchEngineAdapter.java. > In both cases the URL is used to > a) construct GeolocationInfo object, where url doesn't seem to be really used, > but can be accessed via a public accessor, which I haven't been able to trace to > uses effectively > b) Passed as an argument into > WebsitePreferenceBridge.nativeSetGeolocationSettingForOrigin, which in turn > calls WebsitePreferenceBridge::SetSettingForOrigin(), which does some URL > manipulation with ContentSettingsPattern::FromURLNoWildcard on the URL and saves > won't result in problems. If you believe it won't, I would appreciate an > explanation as to why. I've changed it so that the only time we use the full URL is when we pass it to the x-geo check. This should eliminate some of the concerns and demonstrate that the change in query string isn't material except when dealing with x-geo. https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:186: * Wheter to send a geo header for the current URL. On 2015/04/28 06:44:01, newt wrote: > "Whether the geo header is allowed to be sent for the current URL." Done. https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:191: public static boolean sendGeoHeader(Context context, String url, boolean isIncognito) { On 2015/04/28 06:44:01, newt wrote: > sendGeoHeader() is a wildly misleading method name. How about > "isAllowedToReceiveGeoHeader()" or "mayReceiveGeoHeader()" or > "isGeoHeaderEnabledForUrl()" Done. https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:212: public static boolean isLocationDisabledForUrl(Uri uri) { On 2015/04/28 06:44:01, newt wrote: > Add a TODO to delete these methods after GeolocationHeader has been upstreamed. Done. https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:267: ? "allow" : "block")); On 2015/04/28 06:44:01, newt wrote: > Use ContentSetting.ALLOW and ContentSetting.BLOCK instead of "allow" and "block" > string literals Done. https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:273: private boolean locationEnabled(int position, boolean checkGeoHeader) { We must not call getLocationContentSetting directly when creating the fragment args because we need to take x-geo into account. Otherwise, we'll show Location as Allowed in the Search Engine picker, but when you click on the link we show Location as Blocked in the details page. I thought about still creating getLocationContentSetting, but decided against it to avoid falling into the pit of forgetting to take x-geo into account. https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:280: // Catch a corner case with the geoHeader being sent when no permission has been specified. On 2015/04/28 06:44:01, newt wrote: > This is actually a common case. How about "Handle the case where..." Done. https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:124: public static Bundle createFragmentArgsForSite(String url, String locationValueIfMissing) { Oooh, I like that idea! :) https://codereview.chromium.org/1104473002/diff/20001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:256: base::ASCIIToUTF16("query")), SearchTermsData(), nullptr)); Yes, it is needed. Maria and I have been discussing this back and forth. The net of it is: This function's purpose is to return a URL that is representative of a search query. An example if you will. Before this CL, we only used the URL to get the scheme and host name for search queries and we didn't care about the rest. Now, however, we also use the URL to see if a query URL will have location info sent via x-geo. So the URL passes through the normal x-geo checks, but one of the checks it uses is to see if the url is a valid search url (see google_util's IsGoogleSearchUrl). That function returns false for search URLs that are missing the query part, as it considers it invalid. The end result of that would be my code thinking x-geo will not be used, when in fact it will be. The fix is to make the example URL returned be a valid search URL (with the query part) and then we can pass it through the x-geo checks and get an answer that matches the answer that the Omnibox gets when asking if x-geo should be used.
https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/1104473002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:273: private boolean locationEnabled(int position, boolean checkGeoHeader) { On 2015/04/29 14:54:09, Finnur wrote: > We must not call getLocationContentSetting directly when creating the fragment > args because we need to take x-geo into account. Otherwise, we'll show Location > as Allowed in the Search Engine picker, but when you click on the link we show > Location as Blocked in the details page. I thought about still creating > getLocationContentSetting, but decided against it to avoid falling into the pit > of forgetting to take x-geo into account. Ah, I didn't realize that you were calling locationEnabled(..., true) in two different places. Given that, keeping the checkGeoHeader argument is quite reasonable. https://codereview.chromium.org/1104473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1104473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:170: Uri uri = Uri.parse( I wouldn't bother with this extra URL parsing. In fact, it could break in subtle ways, e.g. if a search engine uses https but with a non-standard port number. Same goes for the newly created method SearchEngineAdapter.getHostOfSearchEngineUrl()
https://codereview.chromium.org/1104473002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1104473002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:196: // Only send the Geo header in normal mode. Why can't we just upstream GeolocationHeader for this change? I took a quick look and it doesn't seem like GeolocationHeader has any downstream dependencies. It could easily be just copied up and then you wouldn't have to remember to come back and remove this. And nothing would get out of sync meanwhile.
On 2015/04/29 19:14:10, Maria wrote: > https://codereview.chromium.org/1104473002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java > (right): > > https://codereview.chromium.org/1104473002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:196: > // Only send the Geo header in normal mode. > Why can't we just upstream GeolocationHeader for this change? I took a quick > look and it doesn't seem like GeolocationHeader has any downstream dependencies. > It could easily be just copied up and then you wouldn't have to remember to come > back and remove this. And nothing would get out of sync meanwhile. Yeah, that's a reasonable option. Though this CL needs to be cherry-picked to the branch, and cherry-picking an upstreaming CL is messy.
On 2015/04/29 19:17:55, newt wrote: > On 2015/04/29 19:14:10, Maria wrote: > > > https://codereview.chromium.org/1104473002/diff/60001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java > > (right): > > > > > https://codereview.chromium.org/1104473002/diff/60001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:196: > > // Only send the Geo header in normal mode. > > Why can't we just upstream GeolocationHeader for this change? I took a quick > > look and it doesn't seem like GeolocationHeader has any downstream > dependencies. > > It could easily be just copied up and then you wouldn't have to remember to > come > > back and remove this. And nothing would get out of sync meanwhile. > > Yeah, that's a reasonable option. Though this CL needs to be cherry-picked to > the branch, and cherry-picking an upstreaming CL is messy. I didn't realize this was going to be cherry-picked. I am fine with upstreaming as a follow up to this CL on master instead. Just have a preference for minimizing the amount of time we have copied code living around the codebase.
finnur@chromium.org changed reviewers: - miguelg@chromium.org
Are there remaining concerns here? I think I've addressed all. PTAL. I removed Miguel from reviewers list since the file he was supposed to review is no longer being changed. https://codereview.chromium.org/1104473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1104473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:170: Uri uri = Uri.parse( Okely dokely, mister.
yes, lgtm :) Sorry this took a while...
lgtm https://codereview.chromium.org/1104473002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/1104473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:279: mContext, TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl( optional nit: reuse the url value from the call right above to avoid extra JNI call in this scenario
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1104473002/#ps120001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1104473002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ca5a45d777d9f6ca14a4e41d262268cc5139aace Cr-Commit-Position: refs/heads/master@{#328110} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
