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

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

Index: chrome/browser/ui/android/page_info/page_info_popup_android.cc
diff --git a/chrome/browser/ui/android/page_info/page_info_popup_android.cc b/chrome/browser/ui/android/page_info/page_info_popup_android.cc
index b46387fbf701f6a98a58ba79cd871015741c5b36..5b25a0ec929a619a12389e119ca1eaf16fabfcb8 100644
--- a/chrome/browser/ui/android/page_info/page_info_popup_android.cc
+++ b/chrome/browser/ui/android/page_info/page_info_popup_android.cc
@@ -108,6 +108,7 @@ void PageInfoPopupAndroid::SetPermissionInfo(
 
   // 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);
@@ -116,8 +117,8 @@ void PageInfoPopupAndroid::SetPermissionInfo(
   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_AUTOPLAY);
   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.

 
   std::map<ContentSettingsType, ContentSetting>
       user_specified_settings_to_display;
@@ -136,6 +137,13 @@ void PageInfoPopupAndroid::SetPermissionInfo(
                   ? 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).

       }
     }
   }