Issue 5549 - Implement missing error handlings for custom filter lists

desktop-options.js

Index: desktop-options.js
===================================================================
--- a/desktop-options.js
+++ b/desktop-options.js
@@ -547,14 +547,17 @@
     return null;
   }
 
-  function sendMessageHandleErrors(message, onSuccess)
+  function sendMessageHandleErrors(message, callback)
   {
     browser.runtime.sendMessage(message, (errors) =>
     {
-      if (errors.length > 0)
-        alert(errors.join("\n"));
-      else if (onSuccess)
-        onSuccess();
+      if (callback)
+      {
+        if (errors.length > 0)
+          callback(errors);
+        else
+          callback();
+      }
     });
   }
 
@@ -648,14 +651,38 @@
         });
         break;
       case "save-custom-filters":
+        const filters = E("custom-filters-raw").value;
         sendMessageHandleErrors({
           type: "filters.importRaw",
-          text: E("custom-filters-raw").value,
+          text: filters,
           removeExisting: true
         },
-        () =>
+        (errors) =>
         {
-          setCustomFiltersView("read");
+          if (errors)
+          {
+            E("custom-filters").classList.add("warning");
+            const editFilters = E("custom-filters-error");
+            editFilters.textContent = "";

[saroyanm, 2018/02/02 12:30:58]

I think the "many" class should also be removed here, same as it's done when we
are switching to the read mode.

In order to avoid forgetting details about resetting the "custom-filters-error"
I'll suggest to do that in a separate function.

+
+            // The current error does not contain info about the line
+            // that generated such error.
+            // Whenever the error object will pass the bad filter
+            // within its properties, this split should be removed.
+            const lines = filters.split("\n");
+            const messages = errors.map(error => lines[error.lineno - 1]);
+            for (const message of messages)
+            {
+              const li = document.createElement("li");
+              editFilters.appendChild(li).textContent = message;
+            }
+            if (errors.length > 5)
+              editFilters.classList.add("many");
+          }
+          else
+          {
+            setCustomFiltersView("read");
+          }
         });
         break;
       case "show-more-filters-section":
@@ -755,6 +782,10 @@
     updateCustomFiltersUi();
     if (mode == "read")
     {
+      E("custom-filters").classList.remove("warning");
+      const editFilters = E("custom-filters-error");
+      editFilters.textContent = "";
+      editFilters.classList.remove("many");
       customFiltersElement.disabled = true;
       if (!customFiltersElement.value)
       {
@@ -918,10 +949,10 @@
     {
       setLinks("enable-acceptable-ads-description", link);
     });
-    setElementText(E("tracking-warning-1"), "options_tracking_warning_1", 
+    setElementText(E("tracking-warning-1"), "options_tracking_warning_1",
       [getMessage("common_feature_privacy_title"),
-      getMessage("options_acceptableAds_ads_label")]);
-    setElementText(E("tracking-warning-3"), "options_tracking_warning_3", 
+       getMessage("options_acceptableAds_ads_label")]);
+    setElementText(E("tracking-warning-3"), "options_tracking_warning_3",
       [getMessage("options_acceptableAds_privacy_label")]);
 
     getDocLink("privacy_friendly_ads", (link) =>
@@ -963,7 +994,8 @@
 
     getDocLink("filterdoc", (link) =>
     {
-      E("link-filters").setAttribute("href", link);
+      E("link-filters-1").setAttribute("href", link);
+      E("link-filters-2").setAttribute("href", link);
     });
 
     getDocLink("subscriptions", (link) =>
@@ -1159,10 +1191,7 @@
           type: "filters.get",
           subscriptionUrl: subscription.url
         },
-        (filters) =>

[saroyanm, 2018/02/02 12:30:58]

Why was this change necessary ?

[a.giammarchi, 2018/02/02 13:09:53]

I guess it was just regular maintenance duty. I'll put as it was but I don't
usually wrap same signature for no reason. Curious to know why would you.

[saroyanm, 2018/02/02 13:44:25]

No no need, to put it back.
Nit: Please just don't move the callback function to the next line, please keep
it consistent with other similar implementation:
browser.runtime.sendMessage({
  type: "filters.get",
  subscriptionUrl: subscription.url
}, loadCustomFilters);

-        {
-          loadCustomFilters(filters);
-        });
+        loadCustomFilters);
       }
     });
     loadRecommendations();
@@ -1295,7 +1324,7 @@
         updateSubscription(subscription);
         break;
       case "added":
-        let {url, recommended} = subscription;
+        let {url} = subscription;
         // Handle custom subscription
         if (/^~user/.test(url))
         {