[subresource_filter] Site Details UI for default state

chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.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.website;
 
 import android.content.Context;
 import android.content.DialogInterface;
 import android.content.res.Resources;
 import android.net.Uri;
@@ skipping 46 matching lines @@
 
     // Preference keys, see single_website_preferences.xml
     // Headings:
     public static final String PREF_SITE_TITLE = "site_title";
     public static final String PREF_USAGE = "site_usage";
     public static final String PREF_PERMISSIONS = "site_permissions";
     public static final String PREF_OS_PERMISSIONS_WARNING = "os_permissions_warning";
     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:
     public static final String PREF_RESET_SITE = "reset_site_button";
     // Website permissions (if adding new, see hasPermissionsPreferences and resetSite below):
     public static final String PREF_AUTOPLAY_PERMISSION = "autoplay_permission_list";
     public static final String PREF_BACKGROUND_SYNC_PERMISSION = "background_sync_permission_list";
     public static final String PREF_CAMERA_CAPTURE_PERMISSION = "camera_permission_list";
     public static final String PREF_COOKIES_PERMISSION = "cookies_permission_list";
     public static final String PREF_JAVASCRIPT_PERMISSION = "javascript_permission_list";
@@ skipping 185 matching lines @@
                 && (origin.equals(info.getEmbedderSafe()) || "*".equals(info.getEmbedderSafe()));
     }
 
     /**
      * Updates the permissions displayed in the UI by fetching them from mSite.
      * Must only be called once mSite is set.
      */
     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;
         ListAdapter preferences = getPreferenceScreen().getRootAdapter();
         for (int i = 0; i < preferences.getCount(); ++i) {
             Preference preference = (Preference) preferences.getItem(i);
             if (PREF_SITE_TITLE.equals(preference.getKey())) {
                 preference.setTitle(mSite.getTitle());
             } else if (PREF_CLEAR_DATA.equals(preference.getKey())) {
                 long usage = mSite.getTotalUsage();
@@ skipping 24 matching lines @@
                 setUpListPreference(preference, mSite.getMicrophonePermission());
             } else if (PREF_MIDI_SYSEX_PERMISSION.equals(preference.getKey())) {
                 setUpListPreference(preference, mSite.getMidiPermission());
             } else if (PREF_NOTIFICATIONS_PERMISSION.equals(preference.getKey())) {
                 setUpListPreference(preference, mSite.getNotificationPermission());
             } else if (PREF_POPUP_PERMISSION.equals(preference.getKey())) {
                 setUpListPreference(preference, mSite.getPopupPermission());
             } 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())) {
                 maxPermissionOrder = Math.max(maxPermissionOrder, preference.getOrder());
             }
         }
 
         for (UsbInfo info : mSite.getUsbInfo()) {
             Preference preference = new Preference(getActivity());
             preference.getExtras().putSerializable(EXTRA_USB_INFO, info);
@@ skipping 24 matching lines @@
                     osWarning, osWarningExtra, getActivity(), false);
             if (osWarning.getTitle() == null) {
                 getPreferenceScreen().removePreference(
                         preferenceScreen.findPreference(PREF_OS_PERMISSIONS_WARNING));
             } else if (osWarningExtra.getTitle() == null) {
                 getPreferenceScreen().removePreference(
                         preferenceScreen.findPreference(PREF_OS_PERMISSIONS_WARNING_EXTRA));
             }
         }
 
+        // 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);
             preferenceScreen.removePreference(heading);
         }
         if (!hasPermissionsPreferences()) {
             Preference heading = preferenceScreen.findPreference(PREF_PERMISSIONS);
             preferenceScreen.removePreference(heading);
         }
     }
@@ skipping 118 matching lines @@
         } else if (permission == null && locationAllowed != null) {
             String origin = mSite.getAddress().getOrigin();
             mSite.setGeolocationInfo(new GeolocationInfo(origin, origin, false));
             setUpListPreference(preference, (boolean) locationAllowed
                     ? ContentSetting.ALLOW : ContentSetting.BLOCK);
         } else {
             setUpListPreference(preference, permission);
         }
     }
 
+    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.
      * @param context The current context.
      */
     private boolean hasXGeoLocationPermission(Context context) {
         if (ChromeFeatureList.isEnabled(ChromeFeatureList.CONSISTENT_OMNIBOX_GEOLOCATION)) {
             return false;
         }
 
@@ skipping 214 matching lines @@
         if (mSite.getTotalUsage() > 0) {
             clearStoredData();
         } else {
             // Clearing stored data implies popping back to parent menu if there
             // is nothing left to show. Therefore, we only need to explicitly
             // close the activity if there's no stored data to begin with.
             getActivity().finish();
         }
     }
 }