[subresource_filter] Site Details UI for default state

chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
index 5c660be98775f72527417ead0ab6daca461a93d8..9d9722ca6ddf34a9c050ed4c3e9dbff27c49b997 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
@@ -64,6 +64,8 @@ public class SingleWebsitePreferences extends PreferenceFragment
     public static final String PREF_OS_PERMISSIONS_WARNING_EXTRA = "os_permissions_warning_extra";
     public static final String PREF_OS_PERMISSIONS_WARNING_DIVIDER =
             "os_permissions_warning_divider";
+    public static final String PREF_EXTRA_INFO = "website_settings_extra_info";
+    public static final String PREF_EXTRA_INFO_DIVIDER = "website_settings_extra_info_divider";

[Finnur, 2017/05/23 15:37:13]

These key names are a bit generic, as if to support future reuse for other info
items. I doubt that is useful, though, because you could have these two and any
future info item we might add present on screen at the same time. It would make
more sense to programmatically add them, but for now I would just rename it to
be more specific.

[Charlie Harrison, 2017/05/23 17:12:19]

Renamed to subresource_filter_permission_extra_info.

     // Actions at the top (if adding new, see hasUsagePreferences below):
     public static final String PREF_CLEAR_DATA = "clear_data";
     // Buttons:
@@ -269,6 +271,11 @@ public class SingleWebsitePreferences extends PreferenceFragment
     private void displaySitePermissions() {
         addPreferencesFromResource(R.xml.single_website_preferences);
 
+        // This boolean controls how the subresource filter list preference behaves, as well as
+        // gating the extra info header for subresource filtering.

[Finnur, 2017/05/23 15:37:13]

nit: "gating a header" is language that I (as a non-native speaker) struggle
with... :)

But taking a step back, maybe it would be better to explain what it means for a
SubresourceFilter to be activated and how these can be at odds with one another,
as it seems. At first I confused it with the SubresourceFilter content setting
value, which this UI is toggling.

[Charlie Harrison, 2017/05/23 17:12:19]

I tried to rephrase the comment.

+        boolean subresourceFilterActivated = WebsitePreferenceBridge.getSubresourceFilterActivated(
+                mSite.getAddress().getOrigin());
+
         Set<String> permissionPreferenceKeys =
                 new HashSet<>(Arrays.asList(PERMISSION_PREFERENCE_KEYS));
         int maxPermissionOrder = 0;
@@ -313,7 +320,7 @@ public class SingleWebsitePreferences extends PreferenceFragment
             } else if (PREF_PROTECTED_MEDIA_IDENTIFIER_PERMISSION.equals(preference.getKey())) {
                 setUpListPreference(preference, mSite.getProtectedMediaIdentifierPermission());
             } else if (PREF_SUBRESOURCE_FILTER_PERMISSION.equals(preference.getKey())) {
-                setUpListPreference(preference, mSite.getSubresourceFilterPermission());
+                setUpSubresourceFilterPreference(preference, subresourceFilterActivated);
             }
 
             if (permissionPreferenceKeys.contains(preference.getKey())) {
@@ -358,6 +365,18 @@ public class SingleWebsitePreferences extends PreferenceFragment
             }
         }
 
+        // The subresource filter permission includes a header with some additional information, if
+        // it is being displayed on a site that is activated.
+        Preference extraInfo = preferenceScreen.findPreference(PREF_EXTRA_INFO);
+        Preference extraInfoDivider = preferenceScreen.findPreference(PREF_EXTRA_INFO_DIVIDER);
+        if (getPreferenceScreen().findPreference(PREF_SUBRESOURCE_FILTER_PERMISSION) != null
+                && subresourceFilterActivated) {
+            extraInfo.setTitle(R.string.subresource_filter_permission_extra_info);
+        } else {
+            getPreferenceScreen().removePreference(extraInfo);
+            getPreferenceScreen().removePreference(extraInfoDivider);
+        }
+
         // Remove categories if no sub-items.
         if (!hasUsagePreferences()) {
             Preference heading = preferenceScreen.findPreference(PREF_USAGE);
@@ -496,6 +515,38 @@ public class SingleWebsitePreferences extends PreferenceFragment
         }
     }
 
+    private void setUpSubresourceFilterPreference(Preference preference, boolean activated) {

[Finnur, 2017/05/23 15:37:13]

This function has some non-obvious functionality that would benefit from a
comment, including the param |activated| and how it affects this all.

[Charlie Harrison, 2017/05/23 17:12:19]

Added a comment.

+        ContentSetting permission = mSite.getSubresourceFilterPermission();
+
+        // The subresource filter permission shows sometimes even if there is no permission set.

[Finnur, 2017/05/23 15:37:13]

I don't understand this comment, nor it's placement (above the permission ==
null block)... 

Is this a hack to work around a bug or describing how the implementation works?

[Charlie Harrison, 2017/05/23 17:12:19]

It's describing how the implementation works. I've expanded the comment.

+        if (permission == null) {
+            // If there is activation, we want the setting to show BLOCK by default. Set an explicit
+            // exception on the website that isn't backed by an explicit content setting. Note that
+            // this will never trigger an actual update to content settings, as those go through
+            // setSubresourceFilterPermission.
+            if (activated) {
+                permission = ContentSetting.BLOCK;
+                String origin = mSite.getAddress().getOrigin();
+                mSite.setSubresourceFilterException(new ContentSettingException(
+                        ContentSettingsType.CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER, origin,
+                        permission, ""));

[Finnur, 2017/05/23 15:37:13]

This feels a bit hacky... I'm a little concerned that we're modifying the mSite
object in order to, AFAICT, show some string in the UI. I don't think we do this
elsewhere. Why is it necessary to do it this way?

[Charlie Harrison, 2017/05/23 17:12:19]

Is is necessary due to how we've implemented setSubresourceFilterPermission in
Website.

I've updated the comment to explain this, but would appreciate any pointers for
a better way to do this since I agree it is kind of hacky.

[Finnur, 2017/05/24 10:37:22]

If the problem you are facing is only while changing the value then we could
modify setSubresourceFilterPermission to add the exception when it is missing
(and have a comment explaining why). Would make this code simpler, I think.

[Charlie Harrison, 2017/05/25 15:57:19]

Yeah that works for me. Done.

+            } else {
+                setUpListPreference(preference, null);
+                return;
+            }
+        }
+        assert permission != null;
+        setUpListPreference(preference, permission);
+
+        // The subresource filter permission has a custom BLOCK string.
+        ListPreference listPreference = (ListPreference) preference;
+        Resources res = getResources();
+        listPreference.setEntries(
+                new String[] {res.getString(R.string.website_settings_permissions_allow),
+                        res.getString(R.string.subresource_filter_permission_block)});
+        listPreference.setValueIndex(permission == ContentSetting.ALLOW ? 0 : 1);
+    }
+
     /**
      * Returns true if the current host matches the default search engine host and location for the
      * default search engine is being granted via x-geo.