Hide location permission if a search engine is non-Google or its location permission is unset.

chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
index 792ea9b31ae8182bf99982129943194274e2f44c..611f5067a1263b80b38ea343307355fbc3eaf047 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
@@ -40,6 +40,7 @@ import org.chromium.ui.text.SpanApplier.SpanInfo;
 
 import java.util.List;
 
+
 /**
 * A custom adapter for listing search engines.
 */
@@ -205,23 +206,27 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
         TextView link = (TextView) view.findViewById(R.id.link);
         link.setVisibility(selected ? View.VISIBLE : View.GONE);
         if (selected) {
-            ForegroundColorSpan linkSpan = new ForegroundColorSpan(
-                    ApiCompatibilityUtils.getColor(resources, R.color.pref_accent_color));
-            if (LocationUtils.getInstance().isSystemLocationSettingEnabled()) {
-                String message = mContext.getString(
-                        locationEnabled(position, true)
-                        ? R.string.search_engine_location_allowed
-                        : R.string.search_engine_location_blocked);
-                SpannableString messageWithLink = new SpannableString(message);
-                messageWithLink.setSpan(linkSpan, 0, messageWithLink.length(), 0);
-                link.setText(messageWithLink);
+            if (!shouldShowLocationInfo(position)) {
+                link.setVisibility(View.GONE);
             } else {
-                link.setText(SpanApplier.applySpans(
-                        mContext.getString(R.string.android_location_off),
-                        new SpanInfo("<link>", "</link>", linkSpan)));
+                ForegroundColorSpan linkSpan = new ForegroundColorSpan(
+                        ApiCompatibilityUtils.getColor(resources, R.color.pref_accent_color));
+                if (LocationUtils.getInstance().isSystemLocationSettingEnabled()) {

[Peter Kasting, 2016/11/17 23:12:05]

Is this condition, in this nested case, still what we want?  For example, it
means that we'll show the "location is off" message instead of "location is
blocked" when the engine's setting is "block" and the system setting is "off". 
It means we'll show no setting (to prompt turning on location?) when the
engine's setting is "ask" and the system setting is off.

These might be correct behaviors, it's just not clear to me how we want these to
interact and which kind of setting we want to "override" the others.

[ltian, 2016/11/18 00:17:36]

That part is necessary. Here the "system location" means the location service
across system-wide not location permission for an single app. Just try that if
we disable the system location, it will should a clickable message "Turn on
location in Android Settings". And by clicking that, it forward to Android
location setting page.

[Peter Kasting, 2016/11/18 00:24:47]

Right, that was the UI I figured was here.

I guess my point is that I don't know whether it might be misleading or
unhelpful to do some of our current behavior.  I'm not necessarily arguing for
doing something different either -- there are arguments both ways for some of
these decisions.  But for example, if I explicitly block location data for
Google, and my system location setting also happens to be off, does showing the
equivalent of "click to turn on location" mislead or frustrate, compared to
showing "location blocked for Google"?

I want to be sure you've thought about all these different interaction cases and
are convinced the UX implemented here is the best route :)

+                    String message = mContext.getString(
+                            locationEnabled(position, true)
+                            ? R.string.search_engine_location_allowed
+                            : R.string.search_engine_location_blocked);
+                    SpannableString messageWithLink = new SpannableString(message);
+                    messageWithLink.setSpan(linkSpan, 0, messageWithLink.length(), 0);
+                    link.setText(messageWithLink);
+                } else {
+                    link.setText(SpanApplier.applySpans(
+                            mContext.getString(R.string.android_location_off),
+                            new SpanInfo("<link>", "</link>", linkSpan)));
+                }
+
+                link.setOnClickListener(this);
             }
-
-            link.setOnClickListener(this);
         }
 
         return view;
@@ -300,4 +305,15 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
         }
         return locationPermission == ContentSetting.ALLOW;
     }
+
+    private boolean shouldShowLocationInfo(int position) {
+        String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl(
+                toIndex(position));
+        GeolocationInfo locationSettings = new GeolocationInfo(url, null, false);
+        ContentSetting locationPermission = locationSettings.getContentSetting();

[Peter Kasting, 2016/11/17 23:12:05]

This code seems like it partially duplicates locationEnabled().

Is it possible to just modify that function to return an enum instead of a bool?
 You could even re-use the content setting enum for this (ALLOW, ASK, BLOCK).

[ltian, 2016/11/18 00:17:36]

I thought about it before but the two functions logically has some differences
that the locationEnabled() tries to see ContentSetting.ALLOW +
GeolocationHeader.isGeoHeaderEnabledForUrl()==true while shouldShowLocatinInfo()
checks ContentSetting.ASK + GeolocationHeader.isGeoHeaderEnabledForUrl()==true.

[Peter Kasting, 2016/11/18 00:24:47]

I think we want something like the following (oversimplified):

locationPermission == ALLOW -> ALLOW
locationPermission == ASK -> isGeoHeaderEnabled ? ALLOW : ASK
locationPermission == BLOCK -> BLOCK

Then, basically, we should show the text if the result is not ASK, and the text
should be "allowed" if the result is ALLOW.

+        // For Google search engine, if location permission is granted for the whole app,
+        // even its own location permission is unset, its location permission is still granted.
+        return locationPermission != ContentSetting.ASK
+               || GeolocationHeader.isGeoHeaderEnabledForUrl(mContext, url, false);
+    }
 }