Site Settings Desktop: Fix bug with deleting manually added exceptions.

This is accomplished by using the embedderOrigin more
consistently throughout.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2269963003
Cr-Commit-Position: refs/heads/master@{#414698}

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

@@ -40,7 +40,7 @@ Polymer({
    * @private
    */
   validate_: function() {
-    var pattern = this.addPatternWildcard_(this.site_);
+    var pattern = this.addPatternWildcard(this.site_);
     this.browserProxy.isPatternValid(pattern).then(function(isValid) {
       this.$.add.disabled = !isValid;
     }.bind(this));
@@ -59,7 +59,7 @@ Polymer({
   onSubmit_: function() {
     if (this.$.add.disabled)
       return;  // Can happen when Enter is pressed.
-    var pattern = this.addPatternWildcard_(this.site_);
+    var pattern = this.addPatternWildcard(this.site_);
     this.setCategoryPermissionForOrigin(
         pattern, pattern, this.category, this.allowException ?
             settings.PermissionValues.ALLOW : settings.PermissionValues.BLOCK);

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

@@ -89,7 +89,7 @@ Polymer({
    */
   navigateBackIfNoData_: function() {
     if (this.storedData_ == '' && !this.permissionShowing_())
-      settings.navigateTo(settings.Route.SITE_SETTINGS_ALL);
+      settings.navigateToPreviousRoute();
   },
 
   /**

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

@@ -37,8 +37,8 @@ Polymer({
    * @private
    */
   sameOrigin_: function(left, right) {
-    return this.removePatternWildcard_(left) ==
-        this.removePatternWildcard_(right);
+    return this.removePatternWildcard(left) ==
+        this.removePatternWildcard(right);
   },
 
   /**
@@ -52,9 +52,11 @@ Polymer({
     this.browserProxy.getExceptionList(this.category).then(
         function(exceptionList) {
       for (var i = 0; i < exceptionList.length; ++i) {
-        if (this.sameOrigin_(exceptionList[i].origin, site.origin)) {
+        if (exceptionList[i].embeddingOrigin == site.embeddingOrigin &&
+            this.sameOrigin_(exceptionList[i].origin, site.origin)) {
           this.$.permission.selected = exceptionList[i].setting;
           this.$.details.hidden = false;
+          break;
         }
       }
     }.bind(this));
@@ -63,19 +65,20 @@ Polymer({
   /**
    * Called when a site within a category has been changed.
    * @param {number} category The category that changed.
-   * @param {string} site The site that changed.
+   * @param {string} origin The origin of the site that changed.
+   * @param {string} embeddingOrigin The embedding origin of the site that
+   *     changed.
    * @private
    */
-  sitePermissionChanged_: function(category, site) {
-    if (category == this.category && (site == '' || site == this.site.origin)) {
-      // TODO(finnur): Send down the full SiteException, not just a string.
-      this.siteChanged_({
-        origin: site,
-        originForDisplay: '',
-        embeddingOrigin: '',
-        setting: '',
-        source: '',
-      });
+  sitePermissionChanged_: function(category, origin, embeddingOrigin) {
+    if (this.site === undefined)
+      return;
+    if (category != this.category)
+      return;
+
+    if (origin == '' || (origin == this.site.origin &&
+                         embeddingOrigin == this.site.embeddingOrigin)) {
+      this.siteChanged_(this.site);
     }
   },
 
@@ -83,7 +86,8 @@ Polymer({
    * Resets the category permission for this origin.
    */
   resetPermission: function() {
-    this.resetCategoryPermissionForOrigin(this.site.origin, '', this.category);
+    this.resetCategoryPermissionForOrigin(
+        this.site.origin, this.site.embeddingOrigin, this.category);
     this.$.details.hidden = true;
   },
 
@@ -94,6 +98,6 @@ Polymer({
   onPermissionMenuIronActivate_: function(event) {
     var value = event.detail.item.dataset.permissionValue;
     this.setCategoryPermissionForOrigin(
-        this.site.origin, '', this.category, value);
+        this.site.origin, this.site.embeddingOrigin, this.category, value);
   },
 });

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

@@ -349,14 +349,10 @@ Polymer({
       var originForDisplay = this.sanitizePort(origin.replace('[*.]', ''));
 
       var embeddingOrigin = sites[i].embeddingOrigin;
-      if (this.category == settings.ContentSettingsTypes.GEOLOCATION) {
-        if (embeddingOrigin == '')
-          embeddingOrigin = '*';
-      }
       var embeddingOriginForDisplay = '';
-      if (embeddingOrigin != '' && origin != embeddingOrigin) {
-        embeddingOriginForDisplay = loadTimeData.getStringF(
-            'embeddedOnHost', this.sanitizePort(embeddingOrigin));
+      if (origin != embeddingOrigin) {
+        embeddingOriginForDisplay =
+            this.getEmbedderString(embeddingOrigin, this.category);
       }
 
       // The All Sites category can contain duplicates (from other categories).

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

@@ -397,7 +397,7 @@ var SiteSettingsBehaviorImpl = {
    * @return {string} The resulting pattern.
    * @private
    */
-  addPatternWildcard_: function(pattern) {
+  addPatternWildcard: function(pattern) {
     if (pattern.startsWith('http://'))
       return pattern.replace('http://', 'http://[*.]');
     else if (pattern.startsWith('https://'))
@@ -412,7 +412,7 @@ var SiteSettingsBehaviorImpl = {
    * @return {string} The resulting pattern.
    * @private
    */
-  removePatternWildcard_: function(pattern) {
+  removePatternWildcard: function(pattern) {
     if (pattern.startsWith('http://[*.]'))
       return pattern.replace('http://[*.]', 'http://');
     else if (pattern.startsWith('https://[*.]'))
@@ -422,6 +422,22 @@ var SiteSettingsBehaviorImpl = {
     return pattern;
   },
 
+  /**
+   * Looks up the human-friendly embedder string to show in the UI.
+   * @param {string} embeddingOrigin The embedding origin to show.
+   * @param {string} category The category requesting it.
+   * @return {string} The string to show.
+   */
+  getEmbedderString: function(embeddingOrigin, category) {
+    if (embeddingOrigin == '') {
+      if (category != settings.ContentSettingsTypes.GEOLOCATION)
+        return '';
+      return loadTimeData.getStringF('embeddedOnHost', '*');
+    }
+    return loadTimeData.getStringF(
+        'embeddedOnHost', this.sanitizePort(embeddingOrigin));
+  },
+
   /**
    * Returns the icon to use for a given site.
    * @param {string} site The url of the site to fetch the icon for.

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

@@ -212,7 +212,10 @@ void SiteSettingsHandler::OnContentSettingChanged(
         base::StringValue("contentSettingSitePermissionChanged"),
         base::StringValue(site_settings::ContentSettingsTypeToGroupName(
             content_type)),
-        base::StringValue(primary_pattern.ToString()));
+        base::StringValue(primary_pattern.ToString()),
+        base::StringValue(
+            secondary_pattern == ContentSettingsPattern::Wildcard() ?
+            "" : secondary_pattern.ToString()));
   }
 }
 

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

@@ -59,7 +59,7 @@ cr.define('site_details_permission', function() {
         return browserProxy.whenCalled('setCategoryPermissionForOrigin').then(
             function(arguments) {
               assertEquals(origin, arguments[0]);
-              assertEquals('', arguments[1]);
+              assertEquals(origin, arguments[1]);
               assertEquals(testElement.category, arguments[2]);
               assertEquals(allow ?
                   settings.PermissionValues.ALLOW :
@@ -86,7 +86,7 @@ cr.define('site_details_permission', function() {
         testElement.category = settings.ContentSettingsTypes.CAMERA;
         testElement.site = {
           origin: origin,
-          embeddingOrigin: '',
+          embeddingOrigin: origin,
         };
 
         return browserProxy.whenCalled('getExceptionList').then(function() {
@@ -114,7 +114,7 @@ cr.define('site_details_permission', function() {
         testElement.category = settings.ContentSettingsTypes.CAMERA;
         testElement.site = {
           origin: origin,
-          embeddingOrigin: '',
+          embeddingOrigin: origin,
         };
 
         return browserProxy.whenCalled('getExceptionList').then(function() {

content/public/test/test_web_ui.cc

@@ -62,15 +62,15 @@ void TestWebUI::CallJavascriptFunctionUnsafe(const std::string& function_name) {
 void TestWebUI::CallJavascriptFunctionUnsafe(const std::string& function_name,
                                              const base::Value& arg1) {
   call_data_.push_back(new CallData(function_name));
-  call_data_.back()->TakeAsArg1(arg1.DeepCopy());
+  call_data_.back()->TakeAsArg1(arg1.CreateDeepCopy());
 }
 
 void TestWebUI::CallJavascriptFunctionUnsafe(const std::string& function_name,
                                              const base::Value& arg1,
                                              const base::Value& arg2) {
   call_data_.push_back(new CallData(function_name));
-  call_data_.back()->TakeAsArg1(arg1.DeepCopy());
-  call_data_.back()->TakeAsArg2(arg2.DeepCopy());
+  call_data_.back()->TakeAsArg1(arg1.CreateDeepCopy());
+  call_data_.back()->TakeAsArg2(arg2.CreateDeepCopy());
 }
 
 void TestWebUI::CallJavascriptFunctionUnsafe(const std::string& function_name,
@@ -78,9 +78,9 @@ void TestWebUI::CallJavascriptFunctionUnsafe(const std::string& function_name,
                                              const base::Value& arg2,
                                              const base::Value& arg3) {
   call_data_.push_back(new CallData(function_name));
-  call_data_.back()->TakeAsArg1(arg1.DeepCopy());
-  call_data_.back()->TakeAsArg2(arg2.DeepCopy());
-  call_data_.back()->TakeAsArg3(arg3.DeepCopy());
+  call_data_.back()->TakeAsArg1(arg1.CreateDeepCopy());
+  call_data_.back()->TakeAsArg2(arg2.CreateDeepCopy());
+  call_data_.back()->TakeAsArg3(arg3.CreateDeepCopy());
 }
 
 void TestWebUI::CallJavascriptFunctionUnsafe(const std::string& function_name,
@@ -88,7 +88,11 @@ void TestWebUI::CallJavascriptFunctionUnsafe(const std::string& function_name,
                                              const base::Value& arg2,
                                              const base::Value& arg3,
                                              const base::Value& arg4) {
-  NOTREACHED();
+  call_data_.push_back(new CallData(function_name));
+  call_data_.back()->TakeAsArg1(arg1.CreateDeepCopy());
+  call_data_.back()->TakeAsArg2(arg2.CreateDeepCopy());
+  call_data_.back()->TakeAsArg3(arg3.CreateDeepCopy());
+  call_data_.back()->TakeAsArg4(arg4.CreateDeepCopy());
 }
 
 void TestWebUI::CallJavascriptFunctionUnsafe(
@@ -108,16 +112,20 @@ TestWebUI::CallData::CallData(const std::string& function_name)
 TestWebUI::CallData::~CallData() {
 }
 
-void TestWebUI::CallData::TakeAsArg1(base::Value* arg) {
-  arg1_.reset(arg);
+void TestWebUI::CallData::TakeAsArg1(std::unique_ptr<base::Value> arg) {
+  arg1_ = std::move(arg);
+}
+
+void TestWebUI::CallData::TakeAsArg2(std::unique_ptr<base::Value> arg) {
+  arg2_ = std::move(arg);
 }
 
-void TestWebUI::CallData::TakeAsArg2(base::Value* arg) {
-  arg2_.reset(arg);
+void TestWebUI::CallData::TakeAsArg3(std::unique_ptr<base::Value> arg) {
+  arg3_ = std::move(arg);
 }
 
-void TestWebUI::CallData::TakeAsArg3(base::Value* arg) {
-  arg3_.reset(arg);
+void TestWebUI::CallData::TakeAsArg4(std::unique_ptr<base::Value> arg) {
+  arg4_ = std::move(arg);
 }
 
 }  // namespace content

content/public/test/test_web_ui.h

@@ -70,20 +70,23 @@ class TestWebUI : public WebUI {
     explicit CallData(const std::string& function_name);
     ~CallData();
 
-    void TakeAsArg1(base::Value* arg);
-    void TakeAsArg2(base::Value* arg);
-    void TakeAsArg3(base::Value* arg);
+    void TakeAsArg1(std::unique_ptr<base::Value> arg);
+    void TakeAsArg2(std::unique_ptr<base::Value> arg);
+    void TakeAsArg3(std::unique_ptr<base::Value> arg);
+    void TakeAsArg4(std::unique_ptr<base::Value> arg);
 
     const std::string& function_name() const { return function_name_; }
     const base::Value* arg1() const { return arg1_.get(); }
     const base::Value* arg2() const { return arg2_.get(); }
     const base::Value* arg3() const { return arg3_.get(); }
+    const base::Value* arg4() const { return arg4_.get(); }
 
    private:
     std::string function_name_;
     std::unique_ptr<base::Value> arg1_;
     std::unique_ptr<base::Value> arg2_;
     std::unique_ptr<base::Value> arg3_;
+    std::unique_ptr<base::Value> arg4_;
   };
 
   const ScopedVector<CallData>& call_data() const { return call_data_; }