Issue 5384 - Introduced dedicated mobile options page

mobile-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
- * Copyright (C) 2006-2017 eyeo GmbH
+ * 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* globals getDocLink */
 
 "use strict";
 
 {
   const {getMessage} = ext.i18n;

[saroyanm, 2017/08/27 17:44:01]

Shouldn't the "ext", be declared as global ?
The eslint doesn't fire an error, but I did noticed that on regular options page
as well. 

Am I missing something here ?

[Thomas Greiner, 2017/08/28 12:04:21]

We define `ext` as a global in .eslintrc.json so no need to define it here.

Note that I did get some ESLint errors about "max-len" here and "indent" in
background.js as well as messageResponder.js so I fixed those since I'm already
touching those as part of this change.

[saroyanm, 2017/08/28 12:27:20]

Acknowledged.

 
   const dialogSubscribe = "subscribe";
+  const idAcceptableAds = "acceptableAds";
+  const idRecommended = "subscriptions-recommended";
   let whitelistFilter = null;
   let promisedAcceptableAdsUrl = getAcceptableAdsUrl();
 
   /* Utility functions */
 
   function get(selector, origin)
   {
     return (origin || document).querySelector(selector);
   }
 
@@ skipping 12 matching lines @@
     }
 
     if (attributes)
     {
       for (let name in attributes)
       {
         element.setAttribute(name, attributes[name]);
       }
     }
 
     if (onclick)

[saroyanm, 2017/08/27 17:44:02]

Considering the fact that we have "onclick" function specified in file scope
this will always be true.

[saroyanm, 2017/08/28 08:15:28]

Nevermind it will not, local scope will override the value anyway..

     {
       element.addEventListener("click", (ev) =>
       {
         onclick(ev);
         ev.stopPropagation();
       });
     }
 
     parent.appendChild(element);
     return element;
@@ skipping 58 matching lines @@
 
   function setSubscription({disabled, title, url}, shouldAdd)
   {
     if (disabled)
       return;
 
     promisedAcceptableAdsUrl.then((acceptableAdsUrl) =>
     {
       if (url == acceptableAdsUrl)
       {
-        get("#acceptableAds").checked = true;

[saroyanm, 2017/08/27 17:44:01]

Detail: this ID is used couple of times, if you will decide to fix the "detail"
of "#subscriptions-recommended" I think make sense to fix this as well.

[Thomas Greiner, 2017/08/28 12:04:20]

See other comment for the reply.

+        get(`#${idAcceptableAds}`).checked = true;
         return;
       }
 
       let listInstalled = get("#subscriptions-installed");
       let installed = get(`[data-url="${url}"]`, listInstalled);
 
       if (installed)
       {
         let titleElement = get("span", installed);
         titleElement.textContent = title || url;
       }
       else if (shouldAdd)
       {
         let element = create(listInstalled, "li", null, {"data-url": url});
         create(element, "span", title || url);
         create(element, "button", null, {class: "remove"},
           () => uninstallSubscription(url)
         );
 
-        let recommended = get(`#subscriptions-recommended [data-url="${url}"]`);

[saroyanm, 2017/08/27 17:44:01]

Detail: the "#subscriptions-recommended" ID is used in multiple places, I think
you can use that in the file scope and use "Origin" to select recommended using
the "get()" method.

[Thomas Greiner, 2017/08/28 12:04:20]

Not sure what the underlying argument is.
- If the issue is the duplication of the ID name, I could make it a constant for
this file.
- If the issue is the duplication of calls to retrieve the DOM element, we
usually look at that on a function-level so that we don't keep references to DOM
nodes around for too long.
- If the issue is the performance impact on retrieving the DOM element each
time, I'd consider this to be premature optimization.
- If the issue is none of the above, please let me know.

[saroyanm, 2017/08/28 12:27:19]

Yes I was referring to this .

[Thomas Greiner, 2017/08/28 13:14:05]

Acknowledged.

[saroyanm, 2017/08/28 13:52:05]

Detail: this change didn't land, but it's trivial.

[Thomas Greiner, 2017/08/28 14:00:12]

As I mentioned, throughout all of our code "we don't keep references to DOM
nodes around for too long" so it sounded like you didn't have anything to add to
that.

[saroyanm, 2017/08/28 14:05:24]

I did commented under the line:
"If the issue is the duplication of the ID name, I could make it a constant for
this file."

But as I mentioned it's trivial. So feel free either creating constants for
duplicated IDs, or push as it is right now.

[Thomas Greiner, 2017/08/28 15:01:28]

Ok, I only looked at the text below your message. I uploaded the patch set with
the change but I'm not sure whether it's an improvement.

I don't mind it either way so feel free to decide yourself whether or not you
want to keep this change or whether you think it's better not to make this
change.

+        let recommended = get(`#${idRecommended} [data-url="${url}"]`);
         if (recommended)
         {
           recommended.classList.add("installed");
         }
       }
     });
   }
 
   function removeSubscription(url)
   {
     promisedAcceptableAdsUrl.then((acceptableAdsUrl) =>
     {
       if (url == acceptableAdsUrl)
       {
-        get("#acceptableAds").checked = false;
+        get(`#${idAcceptableAds}`).checked = false;
         return;
       }
 
       let installed = get(`#subscriptions-installed [data-url="${url}"]`);
       if (installed)
       {
         installed.parentNode.removeChild(installed);
       }
 
-      let recommended = get(`#subscriptions-recommended [data-url="${url}"]`);
+      let recommended = get(`#${idRecommended} [data-url="${url}"]`);
       if (recommended)
       {
         recommended.classList.remove("installed");
       }
     });
   }
 
   function setDialog(id, options)
   {
     if (!id)
     {
       delete document.body.dataset.dialog;
       return;
     }
 
     let fields = getAll(`#dialog-${id} input`);
     for (let field of fields)
     {
-      field.value = (options && field.name in options) ? options[field.name] : "";

[saroyanm, 2017/08/27 17:44:01]

Detail: this exceeds the characters limit.

[Thomas Greiner, 2017/08/28 12:04:22]

Done. As mentioned, I also fixed linter errors in the other files.

+      let {name} = field;
+      field.value = (options && name in options) ? options[name] : "";
     }
     setError(id, null);
 
     document.body.dataset.dialog = id;
   }
 
   function setError(dialogId, fieldName)
   {
     let dialog = get(`#dialog-${dialogId}`);
     if (fieldName)
     {
       dialog.dataset.error = fieldName;
     }
     else
     {
       delete dialog.dataset.error;
     }
   }
 
   function populateLists()
   {
     Promise.all([getInstalled(), getRecommendedAds()])
       .then(([installed, recommended]) =>
       {
-        let listRecommended = get("#subscriptions-recommended");
+        let listRecommended = get(`#${idRecommended}`);
         for (let {title, url} of recommended)
         {
           create(listRecommended, "li", title, {"data-url": url},

[saroyanm, 2017/08/27 17:44:00]

Detail: I think we also should specify role="button" for this list items

[Thomas Greiner, 2017/08/28 12:04:22]

We don't include Accessibility in the mobile options page spec which is why no
work has been done in that regard.

Due to that but also the fact that this page is going to be replaced with the
regular options page which will introduced Accessibility support, I'd recommend
not to spend time on such changes.

[saroyanm, 2017/08/28 12:27:20]

Acknowledged.

             (ev) =>
             {
               if (ev.target.classList.contains("installed"))
                 return;
 
               setDialog(dialogSubscribe, {title, url});
             }
           );
         }
 
         for (let subscription of installed)
         {
           if (subscription.disabled)
             continue;
 
           setSubscription(subscription, true);
         }
       })
       .catch((err) => console.error(err));
   }
 
   /* Listeners */
 
   function onChange(ev)
   {
-    if (ev.target.id != "acceptableAds")
+    if (ev.target.id != idAcceptableAds)
       return;
 
     promisedAcceptableAdsUrl.then((acceptableAdsUrl) =>
     {
       if (ev.target.checked)
       {
         installSubscription(acceptableAdsUrl, null);
       }
       else
       {
         uninstallSubscription(acceptableAdsUrl);
       }
     });
   }
   document.addEventListener("change", onChange);
 
   function toggleWhitelistFilter(toggle)
   {
     if (whitelistFilter)
     {
       ext.backgroundPage.sendMessage(
         {
           type: (toggle.checked) ? "filters.remove" : "filters.add",
           text: whitelistFilter
-        }, (errors) =>
+        },
+        (errors) =>
         {
           if (errors.length < 1)

[saroyanm, 2017/08/27 17:44:02]

This doesn't pass ESlint indentation chekck for some reason.
There are some more errors similar to this, but this error doesn't make sense
for me.

[Thomas Greiner, 2017/08/28 12:04:21]

I'm not getting an error here. What rule does it mention?

[saroyanm, 2017/08/28 12:27:19]

Indent: "282:11  error  Expected indentation of 8 spaces but found 10   indent"

[Thomas Greiner, 2017/08/28 13:14:04]

Done. Seems like we updated eslint-config-eyeo. At least I'm seeing it as
"indent-legacy" (see https://hg.adblockplus.org/codingtools/rev/916665587cad)
after upgrading to the latest version.

             return;
 
           console.error(errors);
           toggle.checked = !toggle.checked;
         }
       );
     }
     else
     {
       console.error("Whitelist filter hasn't been initialized yet");
@@ skipping 96 matching lines @@
             }
             else
             {
               setSubscription(subscription, true);
             }
             break;
           case "removed":
             removeSubscription(subscription.url);
             break;
           case "title":
-            // We're also receiving these messages for subscriptions that are not

[saroyanm, 2017/08/27 17:44:01]

Detail: exceeding 80 chars.

[Thomas Greiner, 2017/08/28 12:04:20]

Done.

-            // installed so we shouldn't add those by accident
+            // We're also receiving these messages for subscriptions that are
+            // not installed so we shouldn't add those by accident
             setSubscription(subscription, false);
             break;
         }
         break;
       }
     }
   }
   ext.onMessage.addListener(onMessage);
 
   ext.backgroundPage.sendMessage({
@@ skipping 23 matching lines @@
   get("#dialog-subscribe [name='title']").setAttribute(
     "placeholder",
     getMessage("mops_subscribe_title")
   );
 
   get("#dialog-subscribe [name='url']").setAttribute(
     "placeholder",
     getMessage("mops_subscribe_url")
   );
 }