Make new options page compatible with edge

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 515 matching lines @@
                                             type + "_title");
           }
 
           addSubscription(subscription);
         }
       });
   }
 
   function findParentData(element, dataName, returnElement)
   {
-    while (element)
+    element = element.closest(`[data-${dataName}]`);

[Thomas Greiner, 2018/01/18 16:20:46]

Nice, I wasn't even aware of this method. :)

+    if (element)

[Thomas Greiner, 2018/01/18 16:20:46]

Detail: We can save one level of indentation by returning `null` here already if
we couldn't find an element.

[a.giammarchi, 2018/01/19 17:02:28]

Done.

     {
-      if (element.hasAttribute("data-" + dataName))
-      {
-        if (returnElement)
-          return element;
-        return element.getAttribute("data-" + dataName);
-      }
-
-      element = element.parentElement;
+      if (returnElement) return element;

[Thomas Greiner, 2018/01/18 16:20:46]

Detail: We tend to avoid single-line if-statements to make it clear that, for
instance in this case, we're exiting the function.

However, to be fair I couldn't find it mentioned anywhere in our coding style.
So we may want to think about extending it since this is the way it's written
across all of our coide, as far as I'm aware of.

[a.giammarchi, 2018/01/19 11:16:48]

to be honest, there's no way I am going to remember all the rules so I have a
hook in VS Code that formats everything accordingly to the eyeo code style /
linting rules, as documented.

Do you know if there's a way to enforce this rule? Apparently ESLint does not
provide an option for this but I'd love to never care about coding style and
also, and of course, I want to preserve consistency.

If there's no way to enforce it, I might try to update the documentation
specifying that inline if statements are better off our code base.

Please let me know how to improve here, thanks.

[Thomas Greiner, 2018/01/19 13:55:16]

Our coding style wasn't written with compatibility with linters in mind so some
rules may not be enforceable in an automatic fashion. For instance, we require
usage of hexadecimal notation of CSS colors where possible since that depends on
what colors we want to define and what platforms we support.

We could investigate whether linters are willing to implement this in which case
we could provide this contribution to them. Otherwise we could either modify our
coding style or are left with keep doing it manually in which case we simply
have to accept that we'll never be able to catch all of the violations.

[a.giammarchi, 2018/01/19 14:16:28]

it's good you mentioned that ... 'cause I need a CSS linter too now :-)

However, since coding styles are always very subjective, and in my experience in
most cases a very pointless waste of everybody time when discussed, I'd love
instead to automate all rules and keep consistency.

Accordingly, I'll rather improve my automatic-fixing little snippet and maybe
share it with others at some point so that consistency and code style will be a
no-brainer for everyone, new comer or old.

Will keep this `return` rul in mind until I've done that.

[a.giammarchi, 2018/01/19 17:02:28]

Done.

[a.giammarchi, 2018/01/19 17:02:28]

Done.

[Thomas Greiner, 2018/01/22 13:06:38]

That's a very noble approach. I'd be curious to hear what other devs in the
company think about that.
I'm not too convinced of auto-fixing code personally but detecting all possible
violations would be great. Anything we can do to make development more
attractive to both internal and external people sounds like a good time
investment.

+      return element.getAttribute(`data-${dataName}`);
     }
     return null;
   }
 
   function sendMessageHandleErrors(message, onSuccess)
   {
     browser.runtime.sendMessage(message, (errors) =>
     {
       if (errors.length > 0)
         alert(errors.join("\n"));
@@ skipping 355 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 353 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);
 }