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,35 @@
         });
         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":
@@ -755,6 +779,10 @@
     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)
       {
@@ -918,10 +946,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) =>
@@ -964,6 +992,7 @@
     getDocLink("filterdoc", (link) =>
     {
       E("link-filters").setAttribute("href", link);
+      E("link-filters-on-edit-error").setAttribute("href", link);
     });
 
     getDocLink("subscriptions", (link) =>
@@ -1295,7 +1324,7 @@
         updateSubscription(subscription);
         break;
       case "added":
-        let {url, recommended} = subscription;
+        let {url} = subscription;
         // Handle custom subscription
         if (/^~user/.test(url))
         {