Issue 5549 - Implement missing error handlings for custom filter lists

desktop-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 529 matching lines @@
         if (returnElement)
           return element;
         return element.getAttribute("data-" + dataName);
       }
 
       element = element.parentElement;
     }
     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();
+      }
     });
   }
 
   function switchTab(id)
   {
     location.hash = id;
   }
 
   function execAction(action, element)
   {
@@ skipping 73 matching lines @@
           text: findParentData(element, "access", false)
         });
         break;
       case "remove-subscription":
         browser.runtime.sendMessage({
           type: "subscriptions.remove",
           url: findParentData(element, "access", false)
         });
         break;
       case "save-custom-filters":
+        const filters = E("custom-filters-raw");

[saroyanm, 2018/01/29 11:44:48]

Detail/Suggestion: We don't use the element anywhere else.
What about changing this to smth like:
const filters = E("custom-filters-raw").value;

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

I'll do that, but this is nitpicking ;-)

+        const filtersValue = filters.value;
         sendMessageHandleErrors({
           type: "filters.importRaw",
-          text: E("custom-filters-raw").value,
+          text: filtersValue,
           removeExisting: true
         },
-        () =>
+        (errors) =>
         {
-          setCustomFiltersView("read");
+          if (errors)
+          {
+            E("custom-filters").classList.add("warning");
+            const lines = filtersValue.split(/\n+|\n\s+\n/);

[saroyanm, 2018/01/29 11:44:48]

Splitting same as it's done in the filterValidation
(https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/filterValidation.js...)
should be enough -> split("\n");

Suggestion: I'll suggest to add a comment here mentioning that the
filterValidation doesn't return "filter" when error occurs, also I think make
sense to create a ticket for Platform to update filterValidation in order to
return filter when error occurs, so we can get rid of this duplication in future
and simplify this (let me know if you will need my help).

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

I've tested this before writing the RegExp, their logic keeps going through
empty lines so it's definitively not the same. This split works better so I
don't think I should change it.
 
I'll add a comment and also I'll probably need your help.

[saroyanm, 2018/01/29 14:30:49]

I don't understand what you mean by "Logic keeps going through the lines".
In order to match implementation in backEnd this should be consistent until we
have the logic implemented there.

Can you please let me know with which exact input have you tested ?

[a.giammarchi, 2018/01/29 15:01:09]

if you have a textarea with

a
b

c


d

the filter you pointed at me will loop through all lines. If d is an error the
lineno will be 4.
Accordingly, using just \n won't be consistent. The filter is fault tolerant
with new lines, it doesn't bother.
Neither I do, including possible spaces or multiple new lines.

As summary, splitting by \n only has possible cases not covered (multiple \n\n)
while current RegExp covers that too without any possible harm to the software,
actually granting more consistency with backend.

[saroyanm, 2018/01/30 12:03:44]

This doesn't work on my(Manvel) test enviroment, Andrea will recheck and we will
come back to this again.
My point (Manvel's) - that implementation should match the one on the backend.

+            const info = errors.map(error => lines[error.lineno - 1]);
+            const editFilters = E("custom-filters-edit-filters");
+            info.forEach(

[saroyanm, 2018/01/29 11:44:48]

Detail: We usualy use for..of when looping through arrays, see ->
https://adblockplus.org/en/coding-style#javascript-modern 

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

have we banned the forEach from the code? performance wise forEach it's much
faster than for..of but I guess we don't really have performance issues so, if
that's the case, shouldn't we write in the styleguide that forEach is not OK?

[saroyanm, 2018/01/29 14:30:49]

That is a news for me. AFAIK we used for...of because it's faster than forEach.

[a.giammarchi, 2018/01/29 15:01:09]

It's actually usually slower because it involved
Array.prototype[Symbol.iterator] [1] access and uses objects like generators to
go through each obj.next() and retrieve the value.

While just recently Chrome said that forEach and for/of are just on pair of
regular for loops [2], Firefox and Edge aren't quite there yet.

Regardless, we don't have here a performance cryptical application, choosing any
pattern for performance reasons smells a bit like premature optimisations.
Specially here where having a million filters would result in way more troubles
than a for loop boost.

However, I do care about consistency so if we want to always use for...of loops
we should state that somewhere so that next person won't waste time thinking
about which one is better/preferred ;-)

I really would like all these details to be clear and easy to point at because
to me reviews should be fast and/or focus on the right thing.

[1]
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
[2] https://v8project.blogspot.de/2017/09/

+              message =>
+              {
+                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":
         E("more-filters").setAttribute("aria-hidden", false);
         break;
       case "switch-acceptable-ads":
         let value = element.value || element.dataset.value;
         // User check the checkbox
         let shouldCheck = element.getAttribute("aria-checked") != "true";
         let installAcceptableAds = false;
@@ skipping 79 matching lines @@
         break;
     }
   }
 
   function setCustomFiltersView(mode)
   {
     let customFiltersElement = E("custom-filters-raw");
     updateCustomFiltersUi();
     if (mode == "read")
     {
+      E("custom-filters").classList.remove("warning");
+      const editFilters = E("custom-filters-edit-filters");
+      editFilters.textContent = "";
+      editFilters.classList.remove("many");
       customFiltersElement.disabled = true;
       if (!customFiltersElement.value)
       {
         setCustomFiltersView("empty");
         return;
       }
     }
     else if (mode == "write")
     {
       customFiltersElement.disabled = false;
@@ skipping 143 matching lines @@
 
     // General tab
     getDocLink("contribute", (link) =>
     {
       E("contribute").href = link;
     });
     getDocLink("acceptable_ads_criteria", (link) =>
     {
       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) =>
     {
       E("enable-acceptable-ads-privacy-description").href = link;
     });
     getDocLink("adblock_plus_{browser}_dnt", url =>
     {
       setLinks("dnt", url);
     });
@@ skipping 22 matching lines @@
       what: "features"
     },
     (features) =>
     {
       hidePref("show_devtools_panel", !features.devToolsPanel);
     });
 
     getDocLink("filterdoc", (link) =>
     {
       E("link-filters").setAttribute("href", link);
+      E("link-filters-on-edit-error").setAttribute("href", link);
     });
 
     getDocLink("subscriptions", (link) =>
     {
       E("filter-lists-learn-more").setAttribute("href", link);
     });
 
     E("custom-filters-raw").setAttribute("placeholder",
       getMessage("options_customFilters_edit_placeholder", ["/ads/track/*"]));
 
@@ skipping 311 matching lines @@
         setPrivacyConflict();
         break;
       case "downloading":
       case "downloadStatus":
       case "homepage":
       case "lastDownload":
       case "title":
         updateSubscription(subscription);
         break;
       case "added":
-        let {url, recommended} = subscription;
+        let {url} = subscription;
         // Handle custom subscription
         if (/^~user/.test(url))
         {
           loadCustomFilters(subscription.filters);
           return;
         }
         else if (url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
@@ skipping 160 matching lines @@
   });
   browser.runtime.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }