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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.preferences;
 
 import android.content.Context;
 import android.content.Intent;
 import android.content.SharedPreferences;
 import android.content.res.Resources;
@@ skipping 22 matching lines @@
 import org.chromium.chrome.browser.preferences.website.WebsitePreferenceBridge;
 import org.chromium.chrome.browser.search_engines.TemplateUrlService;
 import org.chromium.chrome.browser.search_engines.TemplateUrlService.LoadListener;
 import org.chromium.chrome.browser.search_engines.TemplateUrlService.TemplateUrl;
 import org.chromium.components.location.LocationUtils;
 import org.chromium.ui.text.SpanApplier;
 import org.chromium.ui.text.SpanApplier.SpanInfo;
 
 import java.util.List;
 
+
 /**
 * A custom adapter for listing search engines.
 */
 public class SearchEngineAdapter extends BaseAdapter implements LoadListener, OnClickListener {
     /**
      * A callback for reporting progress to the owner.
      */
     public interface SelectSearchEngineCallback {
         /**
          * Called when the search engine data has loaded and we've determined the currently active
@@ skipping 145 matching lines @@
             public void onInitializeAccessibilityNodeInfo(View host, AccessibilityNodeInfo info) {
                 super.onInitializeAccessibilityNodeInfo(host, info);
                 info.setCheckable(true);
                 info.setChecked(selected);
             }
         });
 
         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;
     }
 
     // TemplateUrlService.LoadListener
 
     @Override
     public void onTemplateUrlServiceLoaded() {
         TemplateUrlService.getInstance().unregisterLoadListener(this);
@@ skipping 58 matching lines @@
         String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl(
                 toIndex(position));
         GeolocationInfo locationSettings = new GeolocationInfo(url, null, false);
         ContentSetting locationPermission = locationSettings.getContentSetting();
         // Handle the case where the geoHeader being sent when no permission has been specified.
         if (locationPermission == ContentSetting.ASK && checkGeoHeader) {
             return GeolocationHeader.isGeoHeaderEnabledForUrl(mContext, url, false);
         }
         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);
+    }
 }