Make default options in Page Info remain there until the page is reloaded or navigated

When non-default content settings changed to default, they should be kept in
Page Info because user might need to change them again.

If the navigation or refresh happens, content settings with default value
should not be shown in Page Info anymore.

Bug: 777275
Change-Id: Ica986ee6646b833bf47da05fb39abb786ce2259a
Reviewed-on: https://chromium-review.googlesource.com/792790
Commit-Queue: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522006}

chrome/browser/content_settings/tab_specific_content_settings.cc

@@ -840,6 +840,7 @@ void TabSpecificContentSettings::DidStartNavigation(
   ClearGeolocationContentSettings();
   ClearMidiContentSettings();
   ClearPendingProtocolHandler();
+  ClearContentSettingsChangedViaPageInfo();
 }
 
 void TabSpecificContentSettings::DidFinishNavigation(
@@ -895,6 +896,10 @@ void TabSpecificContentSettings::ClearMidiContentSettings() {
   midi_usages_state_.ClearStateMap();
 }
 
+void TabSpecificContentSettings::ClearContentSettingsChangedViaPageInfo() {
+  content_settings_changed_via_page_info_.clear();
+}
+
 void TabSpecificContentSettings::GeolocationDidNavigate(
     content::NavigationHandle* navigation_handle) {
   geolocation_usages_state_.DidNavigate(navigation_handle->GetURL(),
@@ -933,3 +938,14 @@ void TabSpecificContentSettings::BlockAllContentForTesting() {
       media_blocked,
       std::string(), std::string(), std::string(), std::string());
 }
+
+void TabSpecificContentSettings::ContentSettingChangedViaPageInfo(
+    ContentSettingsType type) {
+  content_settings_changed_via_page_info_.insert(type);
+}
+
+bool TabSpecificContentSettings::HasContentSettingChangedViaPageInfo(
+    ContentSettingsType type) const {
+  return content_settings_changed_via_page_info_.find(type) !=
+         content_settings_changed_via_page_info_.end();
+}

chrome/browser/content_settings/tab_specific_content_settings.h

@@ -383,6 +383,13 @@ class TabSpecificContentSettings
   // Block all content. Used for testing content setting bubbles.
   void BlockAllContentForTesting();
 
+  // Stores content settings changed by the user via PageInfo.
+  void ContentSettingChangedViaPageInfo(ContentSettingsType type);
+
+  // Returns true if the user changed the given ContentSettingsType via PageInfo
+  // since the last navigation.
+  bool HasContentSettingChangedViaPageInfo(ContentSettingsType type) const;
+
  private:
   friend class content::WebContentsUserData<TabSpecificContentSettings>;
 
@@ -415,6 +422,9 @@ class TabSpecificContentSettings
   // Clears the MIDI settings.
   void ClearMidiContentSettings();
 
+  // Clears settings changed by the user via PageInfo since the last navigation.
+  void ClearContentSettingsChangedViaPageInfo();
+
   // Updates Geolocation settings on navigation.
   void GeolocationDidNavigate(content::NavigationHandle* navigation_handle);
 
@@ -479,6 +489,10 @@ class TabSpecificContentSettings
   // Observer to watch for content settings changed.
   ScopedObserver<HostContentSettingsMap, content_settings::Observer> observer_;
 
+  // Stores content settings changed by the user via page info since the last
+  // navigation. Used to determine whether to display the settings in page info.
+  std::set<ContentSettingsType> content_settings_changed_via_page_info_;
+
   DISALLOW_COPY_AND_ASSIGN(TabSpecificContentSettings);
 };
 

chrome/browser/ui/page_info/page_info.cc

@@ -140,10 +140,12 @@ bool IsPermissionFactoryDefault(HostContentSettingsMap* content_settings,
 
 // Determines whether to show permission |type| in the Page Info UI. Only
 // applies to permissions listed in |kPermissionType|.
-bool ShouldShowPermission(const PageInfoUI::PermissionInfo& info,
-                          const GURL& site_url,
-                          HostContentSettingsMap* content_settings,
-                          content::WebContents* web_contents) {
+bool ShouldShowPermission(
+    const PageInfoUI::PermissionInfo& info,
+    const GURL& site_url,
+    HostContentSettingsMap* content_settings,
+    content::WebContents* web_contents,
+    TabSpecificContentSettings* tab_specific_content_settings) {
   // Note |CONTENT_SETTINGS_TYPE_ADS| will show up regardless of its default
   // value when it has been activated on the current origin.
   if (info.type == CONTENT_SETTINGS_TYPE_ADS) {
@@ -181,18 +183,24 @@ bool ShouldShowPermission(const PageInfoUI::PermissionInfo& info,
     return true;
 #endif
 
-  // All other content settings only show when they are non-factory-default.
-  if (IsPermissionFactoryDefault(content_settings, info)) {
-    return false;
-  }
-
 #if !defined(OS_ANDROID)
   // Autoplay is Android-only at the moment.
   if (info.type == CONTENT_SETTINGS_TYPE_AUTOPLAY)
     return false;
 #endif
 
-  return true;
+  // Show the content setting if it has been changed by the user since the last
+  // page load.
+  if (tab_specific_content_settings->HasContentSettingChangedViaPageInfo(
+          info.type)) {
+    return true;
+  }
+
+  // Show the content setting when it has a non-default value.
+  if (!IsPermissionFactoryDefault(content_settings, info))
+    return true;
+
+  return false;
 }
 
 void CheckContentStatus(security_state::ContentStatus content_status,
@@ -399,6 +407,8 @@ void PageInfo::RecordPageInfoAction(PageInfoAction action) {
 
 void PageInfo::OnSitePermissionChanged(ContentSettingsType type,
                                        ContentSetting setting) {
+  tab_specific_content_settings()->ContentSettingChangedViaPageInfo(type);
+
   // Count how often a permission for a specific content type is changed using
   // the Page Info UI.
   size_t num_values;
@@ -840,7 +850,7 @@ void PageInfo::PresentSitePermissions() {
     }
 
     if (ShouldShowPermission(permission_info, site_url_, content_settings_,
-                             web_contents())) {
+                             web_contents(), tab_specific_content_settings())) {
       permission_info_list.push_back(permission_info);
     }
   }

chrome/browser/ui/page_info/page_info.h

@@ -185,9 +185,9 @@ class PageInfo : public TabSpecificContentSettings::SiteDataObserver,
   void OnSiteDataAccessed() override;
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(PageInfoTest, NonFactoryDefaultPermissionsShown);
+  FRIEND_TEST_ALL_PREFIXES(PageInfoTest,
+                           NonFactoryDefaultAndRecentlyChangedPermissionsShown);
   friend class PageInfoBubbleViewBrowserTest;
-
   // Initializes the |PageInfo|.
   void Init(const GURL& url, const security_state::SecurityInfo& security_info);
 

chrome/browser/ui/page_info/page_info_unittest.cc

@@ -211,7 +211,7 @@ bool PermissionInfoListContainsPermission(const PermissionInfoList& permissions,
 
 }  // namespace
 
-TEST_F(PageInfoTest, NonFactoryDefaultPermissionsShown) {
+TEST_F(PageInfoTest, NonFactoryDefaultAndRecentlyChangedPermissionsShown) {
   page_info()->PresentSitePermissions();
 // By default, the number of permissions shown should be 0, except on Android,
 // where Geolocation needs to be checked for DSE settings.
@@ -221,7 +221,7 @@ TEST_F(PageInfoTest, NonFactoryDefaultPermissionsShown) {
   EXPECT_EQ(0uL, last_permission_info_list().size());
 #endif
 
-  std::vector<ContentSettingsType> non_default_permissions = {
+  std::vector<ContentSettingsType> expected_visible_permissions = {
       CONTENT_SETTINGS_TYPE_GEOLOCATION, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
       CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC,
   };
@@ -232,46 +232,54 @@ TEST_F(PageInfoTest, NonFactoryDefaultPermissionsShown) {
                                        CONTENT_SETTING_ALLOW);
   page_info()->OnSitePermissionChanged(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC,
                                        CONTENT_SETTING_ALLOW);
-  EXPECT_EQ(non_default_permissions.size(), last_permission_info_list().size());
+  EXPECT_EQ(expected_visible_permissions.size(),
+            last_permission_info_list().size());
 
-  non_default_permissions.push_back(CONTENT_SETTINGS_TYPE_POPUPS);
+  expected_visible_permissions.push_back(CONTENT_SETTINGS_TYPE_POPUPS);
   // Change a default-block setting to a user-preference block instead.
   page_info()->OnSitePermissionChanged(CONTENT_SETTINGS_TYPE_POPUPS,
                                        CONTENT_SETTING_BLOCK);
-  EXPECT_EQ(non_default_permissions.size(), last_permission_info_list().size());
+  EXPECT_EQ(expected_visible_permissions.size(),
+            last_permission_info_list().size());
 
-  non_default_permissions.push_back(CONTENT_SETTINGS_TYPE_JAVASCRIPT);
+  expected_visible_permissions.push_back(CONTENT_SETTINGS_TYPE_JAVASCRIPT);
   // Change a default-allow setting away from the default.
   page_info()->OnSitePermissionChanged(CONTENT_SETTINGS_TYPE_JAVASCRIPT,
                                        CONTENT_SETTING_BLOCK);
-  EXPECT_EQ(non_default_permissions.size(), last_permission_info_list().size());
+  EXPECT_EQ(expected_visible_permissions.size(),
+            last_permission_info_list().size());
 
-  // Make sure setting a default setting to the default doesn't do anything.
+  // Make sure changing a setting to its default causes it to show up, since it
+  // has been recently changed.
+  expected_visible_permissions.push_back(
+      CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
   page_info()->OnSitePermissionChanged(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA,
                                        CONTENT_SETTING_DEFAULT);
-  EXPECT_EQ(non_default_permissions.size(), last_permission_info_list().size());
+  EXPECT_EQ(expected_visible_permissions.size(),
+            last_permission_info_list().size());
 
-  non_default_permissions.pop_back();
-  // Clear the Javascript setting.
+  // Set the Javascript setting to default should keep it shown.
   page_info()->OnSitePermissionChanged(CONTENT_SETTINGS_TYPE_JAVASCRIPT,
                                        CONTENT_SETTING_DEFAULT);
-  EXPECT_EQ(non_default_permissions.size(), last_permission_info_list().size());
+  EXPECT_EQ(expected_visible_permissions.size(),
+            last_permission_info_list().size());
 
-  non_default_permissions.push_back(CONTENT_SETTINGS_TYPE_JAVASCRIPT);
   // Change the default setting for Javascript away from the factory default.
   page_info()->content_settings_->SetDefaultContentSetting(
       CONTENT_SETTINGS_TYPE_JAVASCRIPT, CONTENT_SETTING_BLOCK);
   page_info()->PresentSitePermissions();
-  EXPECT_EQ(non_default_permissions.size(), last_permission_info_list().size());
+  EXPECT_EQ(expected_visible_permissions.size(),
+            last_permission_info_list().size());
 
   // Change it back to ALLOW, which is its factory default, but has a source
   // from the user preference (i.e. it counts as non-factory default).
   page_info()->OnSitePermissionChanged(CONTENT_SETTINGS_TYPE_JAVASCRIPT,
                                        CONTENT_SETTING_ALLOW);
-  EXPECT_EQ(non_default_permissions.size(), last_permission_info_list().size());
+  EXPECT_EQ(expected_visible_permissions.size(),
+            last_permission_info_list().size());
 
   // Sanity check the correct permissions are being shown.
-  for (ContentSettingsType type : non_default_permissions) {
+  for (ContentSettingsType type : expected_visible_permissions) {
     EXPECT_TRUE(PermissionInfoListContainsPermission(
         last_permission_info_list(), type));
   }