[subresource_filter] Always display the default setting in Android Page Info

chrome/browser/ui/android/page_info/page_info_popup_android.cc

 // Copyright (c) 2013 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.
 
 #include "chrome/browser/ui/android/page_info/page_info_popup_android.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/stl_util.h"
@@ skipping 90 matching lines @@
   NOTIMPLEMENTED();
 }
 
 void PageInfoPopupAndroid::SetPermissionInfo(
     const PermissionInfoList& permission_info_list,
     ChosenObjectInfoList chosen_object_info_list) {
   JNIEnv* env = base::android::AttachCurrentThread();
 
   // On Android, we only want to display a subset of the available options in a
   // particular order, but only if their value is different from the default.
+  // This order comes from https://crbug.com/610358.
   std::vector<ContentSettingsType> permissions_to_display;
   permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_GEOLOCATION);
   permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
   permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC);
   permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_NOTIFICATIONS);
   permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_IMAGES);
   permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_JAVASCRIPT);
   permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_POPUPS);
+  permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER);
   permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_AUTOPLAY);

[raymes, 2017/05/11 00:09:42]

Hmm, not sure I understand. I don't see autoplay or subresource filter in that
bug?

[Charlie Harrison, 2017/05/11 00:15:34]

Sorry, the bug links to a document with all the relevant settings. I've just
used it as a link because it's peppered throughout the settings UI code already
when ordering comes up.

[raymes, 2017/05/11 00:32:46]

Ah I see, thanks.

-  permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER);
 
   std::map<ContentSettingsType, ContentSetting>
       user_specified_settings_to_display;
 
   for (const auto& permission : permission_info_list) {
     if (base::ContainsValue(permissions_to_display, permission.type)) {
       if (permission.setting != CONTENT_SETTING_DEFAULT) {
         user_specified_settings_to_display[permission.type] =
             permission.setting;
       } else if (permission.type == CONTENT_SETTINGS_TYPE_GEOLOCATION) {
         if (search_geolocation_service_ &&
             search_geolocation_service_->UseDSEGeolocationSetting(
                 url::Origin(url_))) {
           user_specified_settings_to_display[permission.type] =
               search_geolocation_service_->GetDSEGeolocationSetting()
                   ? CONTENT_SETTING_ALLOW
                   : CONTENT_SETTING_BLOCK;
         }
+      } else if (permission.type == CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER) {
+        // The subresource filter permission should always display the default
+        // setting, if it is showing up in Page Info. Logic for whether the
+        // setting should show up in Page Info is in ShouldShowPermission in
+        // page_info.cc.
+        user_specified_settings_to_display[permission.type] =
+            permission.default_setting;

[raymes, 2017/05/11 00:09:42]

Not quite sure I understand this either? Shouldn't it just show the actual
setting here?

[Charlie Harrison, 2017/05/11 00:15:34]

That's what I'm attempting to do :)

As far as I understand, this branch is taken if permission.setting ==
CONTENT_SETTING_DEFAULT, and permission.default_setting is the "actual" setting.

[raymes, 2017/05/11 00:32:46]

Ah I see - thanks. It's quite subtle and I'm not super familiar with this code.
Maybe we could reorder this code to make it a bit clearer?

if (permission.type == GEOLOCATION) {
  if (permission.setting == DEFAULT)
    ..
  else
    ..
} else if (permission.type == SUBRESOURCE_FILTER) {
  if (permission.setting == DEFAULT)
    ..
  else
    ..
} else {
  // Only show most permissions if they are in a non-default state.
  if (permission.setting != DEFAULT) 
    ..
}

It's more verbose but I think clearer. WDYT?

I'm assuming you've tested that the ShouldShowPermission check applies to the
list passed in here (I haven't looked closely).

       }
     }
   }
 
   for (const auto& permission : permissions_to_display) {
     if (base::ContainsKey(user_specified_settings_to_display, permission)) {
       base::string16 setting_title =
           PageInfoUI::PermissionTypeToUIString(permission);
 
       Java_PageInfoPopup_addPermissionSection(
@@ skipping 13 matching lines @@
         static_cast<jint>(CONTENT_SETTING_ALLOW));
   }
 
   Java_PageInfoPopup_updatePermissionDisplay(env, popup_jobject_);
 }
 
 // static
 bool PageInfoPopupAndroid::RegisterPageInfoPopupAndroid(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }