File Handling: add extensions subtext to list items in chrome://settings

Both the "file handling" entry in an app's permissions list and the
allowed exceptions in chrome://settings/content/fileHandlers get subtext
that matches the exact file handlers allowed for the given app.

Bug: 1203396, 1205103

Change-Id: Id156eeff6ddd4f17780e859ee2b6ed7c082ad09d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2848688
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Darwin Huang <huangdarwin@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#878553}

chrome/app/generated_resources_grd/IDS_WEB_APP_FILE_HANDLING_EXTENSION_LIST_SEPARATOR.png.sha1

@@ -1 +1 @@
-aa286efbaee111ce1a8e37854ff384fa397f929e
+8d581b03984765654d656d238290675b9293c282

chrome/browser/content_settings/chrome_content_settings_utils.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/location_bar/location_bar.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/web_applications/components/web_app_utils.h"
 #include "content/public/browser/web_contents.h"
 #endif
 
@@ -42,4 +43,15 @@ void UpdateLocationBarUiForWebContents(content::WebContents* web_contents) {
 #endif
 }
 
+#if !defined(OS_ANDROID)
+std::u16string GetPermissionDetailString(Profile* profile,
+                                         ContentSettingsType content_type,
+                                         const GURL& url) {
+  if (content_type != ContentSettingsType::FILE_HANDLING || !url.is_valid())
+    return {};
+
+  return web_app::GetFileExtensionsHandledByWebAppDisplayedAsList(profile, url);
+}
+#endif
+
 }  // namespace content_settings

chrome/browser/content_settings/chrome_content_settings_utils.h

@@ -6,11 +6,16 @@
 #define CHROME_BROWSER_CONTENT_SETTINGS_CHROME_CONTENT_SETTINGS_UTILS_H_
 
 #include "build/build_config.h"
+#include "components/content_settings/core/common/content_settings.h"
 
 // Put utility functions only used by //chrome code here. If a function declared
 // here would be meaningfully shared with other platforms, consider moving it to
 // components/content_settings/core/browser/content_settings_utils.h.
 
+class GURL;
+class Profile;
+enum class ContentSettingsType;
+
 namespace content {
 class WebContents;
 }  // namespace content
@@ -51,6 +56,16 @@ void RecordPopupsAction(PopupsAction action);
 // Calls UpdateContentSettingsIcons on the |LocationBar| for |web_contents|.
 void UpdateLocationBarUiForWebContents(content::WebContents* web_contents);
 
+#if !defined(OS_ANDROID)
+// Returns a string for display alongside UI that describes the given content
+// setting in `profile`. This string gives extra, pertinent details about the
+// content setting. `url` represents the site for which the given setting
+// applies.
+std::u16string GetPermissionDetailString(Profile* profile,
+                                         ContentSettingsType content_type,
+                                         const GURL& url);
+#endif
+
 }  // namespace content_settings
 
 #endif  // CHROME_BROWSER_CONTENT_SETTINGS_CHROME_CONTENT_SETTINGS_UTILS_H_

chrome/browser/resources/settings/site_settings/site_details_permission.html

@@ -1,21 +1,26 @@
     <style include="settings-shared md-select"></style>
     <div id="details" hidden$="[[shouldHideCategory_(category)]]">
       <div id="permissionItem"
-          class$="list-item [[permissionInfoStringClass_(site.source, category,
-                                                         site.setting)]]">
+          class$="list-item [[permissionInfoStringClass_(site.source,
+                                                         category,
+                                                         site.setting,
+                                                         site.settingDetail)]]">
         <div>
           <iron-icon icon="[[icon]]">
           </iron-icon>
         </div>
         <div class="middle" id="permissionHeader" aria-hidden="true">
           [[label]]
           <div class="secondary" id="permissionSecondary"
-              hidden$="[[!hasPermissionInfoString_(site.source, category,
-                                                   site.setting)]]"
+              hidden$="[[!hasPermissionInfoString_(site.source,
+                                                   category,
+                                                   site.setting,
+                                                   site.settingDetail)]]"
               inner-h-t-m-l="[[permissionInfoString_(
                 site.source,
                 category,
                 site.setting,
+                site.settingDetail,
                 '$i18nPolymer{siteSettingsAllowlisted}',
                 '$i18nPolymer{siteSettingsAdsBlockBlocklistedSingular}',
                 '$i18nPolymer{siteSettingsAdsBlockNotBlocklistedSingular}',

chrome/browser/resources/settings/site_settings/site_details_permission.js

@@ -192,15 +192,16 @@ Polymer({
    * @param {!SiteSettingSource} source The source of the permission.
    * @param {!ContentSettingsTypes} category The permission type.
    * @param {!ContentSetting} setting The permission setting.
+   * @param {?string} settingDetail A sublabel for the permission.
    * @return {boolean} Whether the permission will have a source string to
    *     display.
    * @private
    */
-  hasPermissionInfoString_(source, category, setting) {
+  hasPermissionInfoString_(source, category, setting, settingDetail) {
     // This method assumes that an empty string will be returned for categories
     // that have no permission info string.
     return this.permissionInfoString_(
-               source, category, setting,
+               source, category, setting, settingDetail,
                // Set all permission info string arguments as null. This is OK
                // because there is no need to know what the information string
                // will be, just whether there is one or not.
@@ -214,12 +215,14 @@ Polymer({
    * @param {!SiteSettingSource} source The source of the permission.
    * @param {!ContentSettingsTypes} category The permission type.
    * @param {!ContentSetting} setting The permission setting.
+   * @param {?string} settingDetail A sublabel for the permission.
    * @return {string} CSS class applied when there is an additional description
    *     string.
    * @private
    */
-  permissionInfoStringClass_(source, category, setting) {
-    return this.hasPermissionInfoString_(source, category, setting) ?
+  permissionInfoStringClass_(source, category, setting, settingDetail) {
+    return this.hasPermissionInfoString_(
+               source, category, setting, settingDetail) ?
         'two-line' :
         '';
   },
@@ -311,6 +314,10 @@ Polymer({
    * @param {!SiteSettingSource} source The source of the permission.
    * @param {!ContentSettingsTypes} category The permission type.
    * @param {!ContentSetting} setting The permission setting.
+   * @param {?string} settingDetail If non-empty, the string to display as the
+   *     permission info. This overrides other calculations made by this
+   *     function, and is used for situations where extra data about the
+   *     permission is required to compose the substring.
    * @param {?string} allowlistString The string to show if the permission is
    *     allowlisted.
    * @param {?string} adsBlacklistString The string to show if the site is
@@ -331,16 +338,24 @@ Polymer({
    * @private
    */
   permissionInfoString_(
-      source, category, setting, allowlistString, adsBlacklistString,
-      adsBlockString, embargoString, insecureOriginString, killSwitchString,
-      extensionAllowString, extensionBlockString, extensionAskString,
-      policyAllowString, policyBlockString, policyAskString,
+      source, category, setting, settingDetail, allowlistString,
+      adsBlacklistString, adsBlockString, embargoString, insecureOriginString,
+      killSwitchString, extensionAllowString, extensionBlockString,
+      extensionAskString, policyAllowString, policyBlockString, policyAskString,
       drmDisabledString) {
     if (source === undefined || category === undefined ||
         setting === undefined) {
       return null;
     }
 
+    if (settingDetail) {
+      // For now, settingDetail is only used for file extensions.
+      // TODO(estade): assert in the other direction as well: the FILE_HANDLING
+      // category should always have detail text.
+      assert(category === ContentSettingsTypes.FILE_HANDLING);
+      return settingDetail;
+    }
+
     /** @type {Object<!ContentSetting, ?string>} */
     const extensionStrings = {};
     extensionStrings[ContentSetting.ALLOW] = extensionAllowString;

chrome/browser/resources/settings/site_settings/site_list_entry.js

@@ -175,6 +175,11 @@ Polymer({
    * @return {string}
    */
   computeSiteDescription_() {
+    // If the SiteException specifies its own label, use that.
+    if (this.model.settingDetail) {
+      return this.model.settingDetail;
+    }
+
     let description = '';
 
     if (this.model.isEmbargoed) {
@@ -191,7 +196,7 @@ Polymer({
         description = loadTimeData.getStringF(
             'embeddedOnHost', this.sanitizePort(this.model.embeddingOrigin));
       }
-    } else if (this.category === ContentSettingsTypes.GEOLOCATION) {
+    } else if (this.model.category === ContentSettingsTypes.GEOLOCATION) {
       description = loadTimeData.getString('embeddedOnAnyHost');
     }
 

chrome/browser/resources/settings/site_settings/site_settings_behavior.js

@@ -172,6 +172,7 @@ const SiteSettingsBehaviorImpl = {
       origin: origin,
       displayName: exception.displayName,
       setting: exception.setting,
+      settingDetail: exception.settingDetail,
       enforcement: enforcement,
       controlledBy: controlledBy,
     };

chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js

@@ -72,6 +72,7 @@ export let SiteGroup;
  *            isEmbargoed: boolean,
  *            origin: string,
  *            displayName: string,
+ *            settingDetail: ?string,
  *            type: string,
  *            setting: !ContentSetting,
  *            source: !SiteSettingSource}}
@@ -87,6 +88,7 @@ export let RawSiteException;
  *            isEmbargoed: boolean,
  *            origin: string,
  *            displayName: string,
+ *            settingDetail: ?string,
  *            setting: !ContentSetting,
  *            enforcement: ?chrome.settingsPrivate.Enforcement,
  *            controlledBy: !chrome.settingsPrivate.ControlledBy,

chrome/browser/ui/webui/settings/site_settings_handler.cc

@@ -22,6 +22,7 @@
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/bluetooth/bluetooth_chooser_context.h"
 #include "chrome/browser/bluetooth/bluetooth_chooser_context_factory.h"
+#include "chrome/browser/content_settings/chrome_content_settings_utils.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/hid/hid_chooser_context.h"
 #include "chrome/browser/hid/hid_chooser_context_factory.h"
@@ -1020,7 +1021,11 @@ void SiteSettingsHandler::HandleGetOriginPermissions(
     raw_site_exception->SetString(site_settings::kDisplayName, display_name);
     raw_site_exception->SetString(site_settings::kSetting,
                                   content_setting_string);
+    raw_site_exception->SetString(site_settings::kSettingDetail,
+                                  content_settings::GetPermissionDetailString(
+                                      profile_, content_type, origin_url));
     raw_site_exception->SetString(site_settings::kSource, source_string);
+
     exceptions->Append(std::move(raw_site_exception));
   }
 

chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc

@@ -1396,7 +1396,8 @@ TEST_F(SiteSettingsHandlerTest, ExceptionHelpers) {
       ContentSettingsPattern::FromString("[*.]google.com");
   std::unique_ptr<base::DictionaryValue> exception =
       site_settings::GetExceptionForPage(
-          pattern, ContentSettingsPattern::Wildcard(), pattern.ToString(),
+          ContentSettingsType::NOTIFICATIONS, /*profile=*/nullptr, pattern,
+          ContentSettingsPattern::Wildcard(), pattern.ToString(),
           CONTENT_SETTING_BLOCK,
           site_settings::SiteSettingSourceToString(
               site_settings::SiteSettingSource::kPreference),

chrome/browser/ui/webui/settings/site_settings_helper.cc

@@ -15,6 +15,7 @@
 #include "base/values.h"
 #include "chrome/browser/bluetooth/bluetooth_chooser_context.h"
 #include "chrome/browser/bluetooth/bluetooth_chooser_context_factory.h"
+#include "chrome/browser/content_settings/chrome_content_settings_utils.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/hid/hid_chooser_context.h"
 #include "chrome/browser/hid/hid_chooser_context_factory.h"
@@ -459,6 +460,8 @@ void AddExceptionForHostedApp(const std::string& url_pattern,
 // Create a DictionaryValue* that will act as a data source for a single row
 // in a HostContentSettingsMap-controlled exceptions table (e.g., cookies).
 std::unique_ptr<base::DictionaryValue> GetExceptionForPage(
+    ContentSettingsType content_type,
+    Profile* profile,
     const ContentSettingsPattern& pattern,
     const ContentSettingsPattern& secondary_pattern,
     const std::string& display_name,
@@ -477,8 +480,11 @@ std::unique_ptr<base::DictionaryValue> GetExceptionForPage(
   std::string setting_string =
       content_settings::ContentSettingToString(setting);
   DCHECK(!setting_string.empty());
-
   exception->SetString(kSetting, setting_string);
+
+  exception->SetString(site_settings::kSettingDetail,
+                       content_settings::GetPermissionDetailString(
+                           profile, content_type, GURL(pattern.ToString())));
   exception->SetString(kSource, provider_name);
   exception->SetBoolean(kIncognito, incognito);
   exception->SetBoolean(kIsEmbargoed, is_embargoed);
@@ -640,8 +646,8 @@ void GetExceptionsForContentType(
     const ContentSettingsPattern& secondary_pattern =
         parent == one_settings.end() ? primary_pattern : parent->first;
     this_provider_exceptions.push_back(GetExceptionForPage(
-        primary_pattern, secondary_pattern, display_name, parent_setting,
-        source, incognito,
+        type, profile, primary_pattern, secondary_pattern, display_name,
+        parent_setting, source, incognito,
         base::Contains(origins_under_embargo, primary_pattern)));
 
     // Add the "children" for any embedded settings.
@@ -652,8 +658,9 @@ void GetExceptionsForContentType(
 
       ContentSetting content_setting = j->second;
       this_provider_exceptions.push_back(GetExceptionForPage(
-          primary_pattern, j->first, display_name, content_setting, source,
-          incognito, base::Contains(origins_under_embargo, primary_pattern)));
+          type, profile, primary_pattern, j->first, display_name,
+          content_setting, source, incognito,
+          base::Contains(origins_under_embargo, primary_pattern)));
     }
   }
 
@@ -754,7 +761,8 @@ void GetPolicyAllowedUrls(
   DCHECK(type == ContentSettingsType::MEDIASTREAM_MIC ||
          type == ContentSettingsType::MEDIASTREAM_CAMERA);
 
-  PrefService* prefs = Profile::FromWebUI(web_ui)->GetPrefs();
+  Profile* profile = Profile::FromWebUI(web_ui);
+  PrefService* prefs = profile->GetPrefs();
   const base::ListValue* policy_urls =
       prefs->GetList(type == ContentSettingsType::MEDIASTREAM_MIC
                          ? prefs::kAudioCaptureAllowedUrls
@@ -784,7 +792,8 @@ void GetPolicyAllowedUrls(
     std::string display_name =
         GetDisplayNameForPattern(pattern, extension_registry);
     exceptions->push_back(GetExceptionForPage(
-        pattern, ContentSettingsPattern(), display_name, CONTENT_SETTING_ALLOW,
+        type, profile, pattern, ContentSettingsPattern(), display_name,
+        CONTENT_SETTING_ALLOW,
         SiteSettingSourceToString(SiteSettingSource::kPolicy), incognito));
   }
 }

chrome/browser/ui/webui/settings/site_settings_helper.h

@@ -63,6 +63,7 @@ constexpr char kOrigin[] = "origin";
 constexpr char kOriginForFavicon[] = "originForFavicon";
 constexpr char kRecentPermissions[] = "recentPermissions";
 constexpr char kSetting[] = "setting";
+constexpr char kSettingDetail[] = "settingDetail";
 constexpr char kSites[] = "sites";
 constexpr char kPolicyIndicator[] = "indicator";
 constexpr char kSource[] = "source";
@@ -124,6 +125,8 @@ base::Value GetValueForManagedState(const ManagedState& state);
 
 // Helper function to construct a dictionary for an exception.
 std::unique_ptr<base::DictionaryValue> GetExceptionForPage(
+    ContentSettingsType content_type,
+    Profile* profile,
     const ContentSettingsPattern& pattern,
     const ContentSettingsPattern& secondary_pattern,
     const std::string& display_name,

chrome/browser/web_applications/components/web_app_utils.cc

@@ -142,12 +142,16 @@ bool IsChromeOs() {
 std::vector<std::string> GetFileExtensionsHandledByWebApp(Profile* profile,
                                                           const GURL& url) {
   auto* provider = WebAppProviderBase::GetProviderBase(profile);
+  if (!provider)
+    return {};
+
   const AppRegistrar& registrar = provider->registrar();
-  const apps::FileHandlers* handlers =
-      registrar.GetAppFileHandlers(*registrar.FindAppWithUrlInScope(url));
-  DCHECK(handlers);
-  std::set<std::string> extensions =
-      apps::GetFileExtensionsFromFileHandlers(*handlers);
+  base::Optional<AppId> app_id = registrar.FindAppWithUrlInScope(url);
+  if (!app_id)
+    return {};
+
+  std::set<std::string> extensions = apps::GetFileExtensionsFromFileHandlers(
+      *registrar.GetAppFileHandlers(*app_id));
   return std::vector<std::string>(extensions.begin(), extensions.end());
 }
 

chrome/test/data/webui/settings/site_details_permission_tests.js

@@ -502,4 +502,37 @@ suite('SiteDetailsPermission', function() {
         assertFalse(testElement.$.permission.disabled);
         assertFalse(testElement.$.permission.options.block.hidden);
       });
+
+  test('settingDetail string is respected', function() {
+    const origin = 'https://www.example.com';
+    browserProxy.setPrefs(prefs);
+
+    testElement.category = ContentSettingsTypes.SOUND;
+    testElement.label = 'Sound';
+    testElement.site = {
+      origin: origin,
+      embeddingOrigin: '',
+      setting: ContentSetting.ALLOW,
+      source: SiteSettingSource.PREFERENCE,
+    };
+
+    // Typically, the secondary text is hidden.
+    assertTrue(testElement.$.permissionSecondary.hidden);
+
+    testElement.category = ContentSettingsTypes.FILE_HANDLING;
+    testElement.label = 'File handlers';
+    testElement.site = {
+      origin: origin,
+      embeddingOrigin: '',
+      setting: ContentSetting.ALLOW,
+      source: SiteSettingSource.PREFERENCE,
+      settingDetail: '.txt',
+    };
+
+    // For file handlers with a `settingDetail`, the secondary text is shown.
+    assertFalse(testElement.$.permissionSecondary.hidden);
+    assertEquals(
+        '.txt', testElement.$.permissionSecondary.innerText,
+        'settingDetail should be displayed');
+  });
 });