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 633 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;
+        const filters = E("custom-filters-raw").value;
         sendMessageHandleErrors({
           type: "filters.importRaw",
-          text: filtersValue,
+          text: filters,
           removeExisting: true
         },
         (errors) =>
         {
           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;
-              }
-            );
+            const customFiltersError = clearAndGetCustomFiltersError();
+
+            // 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");
+              customFiltersError.appendChild(li).textContent = message;
+            }
             if (errors.length > 5)
-              editFilters.classList.add("many");
+              customFiltersError.classList.add("many");
           }
           else
           {
             setCustomFiltersView("read");
           }
         });
         break;
       case "show-more-filters-section":
         E("more-filters").setAttribute("aria-hidden", false);
         break;
@@ skipping 78 matching lines @@
           closeDialog();
         }
         else
         {
           form.querySelector(":invalid").focus();
         }
         break;
     }
   }
 
+  function clearAndGetCustomFiltersError()
+  {
+    const customFiltersError = E("custom-filters-error");
+    customFiltersError.textContent = "";
+    customFiltersError.classList.remove("many");
+    return customFiltersError;
+  }
+
   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");
+      clearAndGetCustomFiltersError();
       customFiltersElement.disabled = true;
       if (!customFiltersElement.value)
       {
         setCustomFiltersView("empty");
         return;
       }
     }
     else if (mode == "write")
     {
       customFiltersElement.disabled = false;
@@ skipping 188 matching lines @@
       type: "app.get",
       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);
+      E("link-filters-1").setAttribute("href", link);
+      E("link-filters-2").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 174 matching lines @@
       special: true
     },
     (subscriptions) =>
     {
       // Load filters
       for (let subscription of subscriptions)
       {
         browser.runtime.sendMessage({
           type: "filters.get",
           subscriptionUrl: subscription.url
-        },
-        (filters) =>
-        {
-          loadCustomFilters(filters);
-        });
+        }, loadCustomFilters);
       }
     });
     loadRecommendations();
     browser.runtime.sendMessage({
       type: "prefs.get",
       key: "subscriptions_exceptionsurl"
     },
     (url) =>
     {
       acceptableAdsUrl = url;
@@ skipping 293 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);
 }