Issue 6420 - Fixed poping up whitelisted website notification for several custom filter lists

js/desktop-options.js

Index: js/desktop-options.js
===================================================================
--- a/js/desktop-options.js
+++ b/js/desktop-options.js
@@ -468,7 +468,6 @@
     updateFilter(filter);
 
   setCustomFiltersView("read");
-  isCustomFiltersLoaded = true;
 }
 
 function removeCustomFilter(text)
@@ -1147,18 +1146,14 @@
   },
   (subscriptions) =>
   {
-    // Load filters
-    for (let subscription of subscriptions)
+    let customFilterPromises = subscriptions.map((subscription) =>
+      getSubscriptionFilters(subscription));

[Thomas Greiner, 2018/03/05 16:43:33]

Detail: This arrow function is redundant because you can write the same code as:

let customFilterPromises = subscriptions.map(getSubscriptionFilters);

[saroyanm, 2018/03/05 16:51:13]

I like this, will change.

+
+    Promise.all(customFilterPromises).then((filters) =>
     {
-      browser.runtime.sendMessage({
-        type: "filters.get",
-        subscriptionUrl: subscription.url
-      },
-      (filters) =>
-      {
-        loadCustomFilters(filters);
-      });
-    }
+      loadCustomFilters([].concat(...filters));

[Thomas Greiner, 2018/03/05 16:43:33]

What is this concatenation for?

[saroyanm, 2018/03/05 16:51:13]

It's for flattening the array.

Promise.all returns a matrix in this case, because getSubscriptionFilters
promises an array.

[Thomas Greiner, 2018/03/06 13:08:47]

Sorry, I missed that. Maybe because I'm used to writing this as
`Array.prototype.concat.apply([], filters)`. Your approach is actually cleaner
so looks good.

+      isCustomFiltersLoaded = true;
+    });
   });
   loadRecommendations();
   browser.runtime.sendMessage({
@@ -1335,6 +1330,17 @@
   }
 }
 
+function getSubscriptionFilters(subscription)
+{
+  return new Promise((resolve, reject) =>
+  {
+    browser.runtime.sendMessage({

[Thomas Greiner, 2018/03/05 16:43:33]

`browser.runtime.sendMessage()` already returns a promise because our
abstraction layer also supports how Firefox implements it those APIs. Therefore
we don't need to wrap it inside our own promise here.

See also
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/sendMes...

[saroyanm, 2018/03/05 16:51:13]

Didn't know that we already return a promise, will update this.
THanks.

[saroyanm, 2018/03/05 17:18:39]

Hmm, Apparently the polyfill doesn't return Promise ->
https://hg.adblockplus.org/adblockplusui/file/tip/ext/content.js#l67

[Thomas Greiner, 2018/03/06 13:08:47]

It's not actually a polyfill and more of a mock so I don't know why we label it
as such in ext/content.js. But yes, we need to update that.

+      type: "filters.get",
+      subscriptionUrl: subscription.url
+    }, resolve);
+  });
+}
+
 function hidePref(key, value)
 {
   let element = document.querySelector("[data-pref='" + key + "']");