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
+ *
+ * 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";
+

[saroyanm, 2017/07/18 12:46:28]

I'll suggest to encapsulate this file with own block scope, to ensure that
global function names will not have conflict in future with other files.

[Thomas Greiner, 2017/07/18 17:42:32]

Done.

+const {getMessage} = ext.i18n;
+
+let whitelistFilter = null;
+let promisedAcceptableAdsUrl = getAcceptableAdsUrl();
+
+/* Utility functions */
+
+function get(selector, origin)

[saroyanm, 2017/07/18 12:46:28]

Detail: Calling function "get" is too generic, as we are getting an element,
maybe we can call this "getElement", "getAllElements" ?

Anyway this shouldn't be much of a problem if we enclose the file in it's scope.

[Thomas Greiner, 2017/07/18 17:42:31]

Acknowledged.

Note that the inspiration for this function was the `E()` function in common.js
because each of those utility functions could be moved into common.js if we want
to.

For now we wanted to avoid delays due to code reusage but that'd be an easy
change to make afterwards if you agree that those functions would also be useful
to have for other pages.

[saroyanm, 2017/07/31 11:08:54]

Now I see your intention, but in that case I think maybe make sense to
completely replace E() function with current solution ?
Anyway while the file is encapsulated,current solution is fine, we can discuss
replacing E() with current solution separately, as it's not a priority right
now.

+{
+  return (origin || document).querySelector(selector);
+}
+
+function getAll(selector, origin)
+{
+  return (origin || document).querySelectorAll(selector);
+}
+
+function create(parent, tagName, content, attributes, onclick)
+{
+  let element = document.createElement(tagName);
+
+  if (typeof content == "string")
+  {
+    element.textContent = content;
+  }
+
+  if (attributes)
+  {
+    for (let name in attributes)
+    {
+      element.setAttribute(name, attributes[name]);
+    }
+  }
+
+  if (onclick)
+  {
+    element.addEventListener("click", (ev) =>
+    {
+      onclick(ev);
+      ev.stopPropagation();
+    });

[saroyanm, 2017/07/18 12:46:28]

Note: I do remember before it was required to specify useCapture. I couldn't
find that rule in Coding styles anymore. Anyway while you are using
stopPropagation, it shouldn't matter anyway.

[Thomas Greiner, 2017/07/18 17:42:32]

We dropped that requirement a while ago. From what I remember, we had to specify
it because of some Firefox issue and that we're no longer supporting said
Firefox version.

+  }
+
+  parent.appendChild(element);
+  return element;
+}
+
+/* Extension interactions */
+
+function getInstalled()
+{
+  return new Promise((resolve, reject) =>
+  {
+    ext.backgroundPage.sendMessage(
+      {type: "subscriptions.get", downloadable: true},
+      resolve
+    );
+  });
+}
+
+function getAcceptableAdsUrl()
+{
+  return new Promise((resolve, reject) =>
+  {
+    ext.backgroundPage.sendMessage(
+      {type: "prefs.get", key: "subscriptions_exceptionsurl"},
+      resolve
+    );
+  });
+}
+
+function getRecommended()

[saroyanm, 2017/07/18 12:46:28]

This function only return recommended subscriptions of type ads.

Suggestion: what about renaming this function to "getRecommendedAds".

[Thomas Greiner, 2017/07/18 17:42:33]

Good point. Done.

+{
+  return fetch("subscriptions.xml")
+    .then((resp) => resp.text())

[saroyanm, 2017/07/18 12:46:27]

Detail: we usually are not using short form of the words. ex. "response" instead
"resp". (exception "ev", for event ?)

Same applies to other occurrences ex. "doc".

[saroyanm, 2017/07/18 12:46:28]

Shouldn't this be returned (return response.text();) ? In order to be used in
next chain ?
(Maybe I'm missing something here)

[Thomas Greiner, 2017/07/18 17:42:31]

No, this is one of the features of arrow functions.

"In a concise body, only an expression is needed, and an implicit return is
attached. In a block body, you must use an explicit return statement."
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/A...

[Thomas Greiner, 2017/07/18 17:42:31]

I agree that it makes sense for "response", so I'll change that, but with others
it helps us avoid confusion and issues due to existing variables. For instance,
the "document" variable already exists and same applies for the "event" variable
in the scope of an event listener.

+    .then((text) =>
+    {
+      let doc = new DOMParser().parseFromString(text, "application/xml");
+      let elements = Array.from(doc.getElementsByTagName("subscription"));
+
+      return elements
+        .filter((element) => element.getAttribute("type") == "ads")
+        .map((element) =>
+        {
+          return {

[saroyanm, 2017/07/18 12:46:27]

Detail: the brace should go to the next row for consistency and readability I
assume.

[Thomas Greiner, 2017/07/18 17:42:31]

Moving this brace to the next row would be interpreted by the JavaScript engine
as:

return undefined;
{
  title: element.getAttribute("title"),
  url: element.getAttribute("url")
};

Secondly, this is not an opening brace but an object literal so the rule doesn't
apply here.

+            title: element.getAttribute("title"),

[saroyanm, 2017/07/18 12:46:28]

Shouldn't this titles be translated ?

I think we should somehow provide translations, or include the name of the
language in the native language (ex. next to the language or below). 

[Thomas Greiner, 2017/07/18 17:42:31]

We don't translate filter list titles (e.g. "EasyList") in any of our UIs.

+            url: element.getAttribute("url")
+          };
+        });
+    });
+}
+
+function installSubscription(url, title)
+{
+  ext.backgroundPage.sendMessage({type: "subscriptions.add", url, title});
+}
+
+function uninstallSubscription(url)
+{
+  ext.backgroundPage.sendMessage({type: "subscriptions.remove", url});
+}
+
+/* Actions */
+
+function setSubscription({disabled, title, url}, shouldAdd)
+{
+  if (disabled)
+    return;
+
+  promisedAcceptableAdsUrl.then((acceptableAdsUrl) =>
+  {
+    if (url == acceptableAdsUrl)
+    {
+      get("#acceptableAds").checked = true;
+      return;
+    }
+
+    let listInstalled = get("#subscriptions-installed");
+    let installed = get(`[data-url="${url}"]`, listInstalled);

[saroyanm, 2017/07/18 12:46:28]

Detail: I think we are using single quotes "'", rather than back-quotes "`" for
strings.

(Same applies to other occurrences)

[Thomas Greiner, 2017/07/18 17:42:32]

This is a template literal, not a string literal. It allows us to dynamically
create a string without concatenation.

+
+    if (installed)
+    {
+      let titleElement = get("span", installed);
+      titleElement.textContent = title || url;
+    }
+    else if (shouldAdd)

[saroyanm, 2017/07/18 12:46:28]

Why are you using "shouldAdd" parameter ?

If the subscription is not installed and is not disabled, then it should be
added, if I'm not missing something here.

[Thomas Greiner, 2017/07/18 17:42:31]

Why should we show subscriptions that are not installed?

[saroyanm, 2017/07/31 11:08:54]

I think I meant "installed", nevermind, I don't remember why I made the comment.
I think implementation is fine.

+    {
+      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}"]`);
+      if (recommended)
+      {
+        recommended.classList.add("installed");
+      }
+    }
+  });
+}
+
+function removeSubscription(url)
+{
+  promisedAcceptableAdsUrl.then((acceptableAdsUrl) =>
+  {
+    if (url == acceptableAdsUrl)
+    {
+      get("#acceptable-ads").checked = false;

[saroyanm, 2017/07/18 12:46:27]

I think you forget to change all camelCase IDs.

[Thomas Greiner, 2017/07/18 17:42:32]

Yep, seems so. Done.

+      return;
+    }
+
+    let installed = get(`#subscriptions-installed [data-url="${url}"]`);
+    if (installed)
+    {
+      installed.parentNode.removeChild(installed);
+    }
+
+    let recommended = get(`#subscriptions-recommended [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] : "";
+  }
+  setError(id, null);
+
+  document.body.dataset.dialog = id;
+}
+
+function setError(dialogId, message)

[saroyanm, 2017/07/18 12:46:28]

Detail: Parameter name "message" is misleading, in this case I think you are
passing "fieldName" rather than "message", which feels to be error message,
while it's not.

[Thomas Greiner, 2017/07/18 17:42:33]

Looks like I forgot to adapt that. Done.

+{
+  let dialog = get(`#dialog-${dialogId}`);
+  if (message)
+  {
+    dialog.dataset.error = message;
+  }
+  else
+  {
+    delete dialog.dataset.error;
+  }
+}
+
+function populateLists()
+{
+  Promise.all([getInstalled(), getRecommended()])
+    .then(([installed, recommended]) =>
+    {
+      let listRecommended = get("#subscriptions-recommended");
+      for (let {title, url} of recommended)
+      {
+        create(listRecommended, "li", title, {"data-url": url},
+          (ev) =>
+          {
+            if (ev.target.classList.contains("installed"))
+              return;
+
+            setDialog("subscribe", {title, url});

[saroyanm, 2017/07/18 12:46:27]

Detail: reference to "subscribe" dialog is used quite often, might make sense to
use a separate variable for that. Also the variable can be used to generate the
actual id. ex.: "dialog-" + dialogName.

[Thomas Greiner, 2017/07/18 17:42:32]

Done.
Yes, that's how it's currently being used (see `setDialog()` and `setError()`)
so not sure what you mean.

+          }
+        );
+      }
+
+      for (let subscription of installed)
+      {
+        if (subscription.disabled)
+          continue;
+
+        setSubscription(subscription, true);
+      }
+    })
+    .catch((err) => console.error(err));

[saroyanm, 2017/07/18 12:46:28]

I think you have mentioned this, so I'll ignore that this error is not visible
in the UI as it's also not specified in the specifications.

[Thomas Greiner, 2017/07/18 17:42:32]

Acknowledged.

+}
+
+/* Listeners */
+
+function onChange(ev)
+{
+  if (ev.target.id != "acceptable-ads")

[saroyanm, 2017/07/18 12:46:27]

Same as mentioned above, there is no element with "acceptable-ads" ID.

[Thomas Greiner, 2017/07/18 17:42:33]

Done.

+    return;
+
+  promisedAcceptableAdsUrl.then((acceptableAdsUrl) =>
+  {
+    if (ev.target.checked)
+    {
+      installSubscription(acceptableAdsUrl, null);
+    }
+    else
+    {
+      uninstallSubscription(acceptableAdsUrl);
+    }
+  });
+}
+document.addEventListener("change", onChange);
+
+function onClick(ev)
+{
+  switch (ev.target.dataset.action)
+  {
+    case "close-dialog":

[saroyanm, 2017/07/18 12:46:27]

Note: Why not to handle all other click events here as well ? Ex. Remove
subscription, add subscription and etc.

(If it's appears to be non trivial, this solution is still fine for the release)

[Thomas Greiner, 2017/07/18 17:42:32]

Done. I'm also using it now for the "enabled" toggle but it's simpler to do it
this way for dynamically generated items because for those we'd need to
determine which element was clicked and which element contains the information
the action requires (e.g. title and URL). Therefore I left it that way for those
actions.

+      setDialog(null);
+      break;
+    case "open-dialog":
+      setDialog(ev.target.dataset.dialog);
+      break;
+  }
+}
+document.addEventListener("click", onClick);
+
+function onSubmit(ev)
+{
+  let fields = ev.target.elements;
+  let title = fields.title.value;
+  let url = fields.url.value;
+
+  if (!title)
+  {
+    setError("subscribe", "title");
+  }
+  else if (!url)
+  {
+    setError("subscribe", "url");
+  }
+  else
+  {
+    installSubscription(url, title);
+    setDialog(null);
+  }
+
+  ev.preventDefault();
+}
+document.addEventListener("submit", onSubmit);
+
+function onToggleWhitelistFilter(ev)
+{
+  let checkbox = ev.target;

[saroyanm, 2017/07/18 12:46:28]

Suggestion: We can avoid using element name as a variable, as the element itself
can change (I don't have strong opinion regarding this, but I do remember you
mentioned similar case in another review)

[Thomas Greiner, 2017/07/18 17:42:32]

In this case the implementation depends on it being a checkbox. Of course we
could still implement it using different HTML elements but we always expect it
to behave like a checkbox.

However, I guess "toggle" should be just as descriptive so I changed it to that
one. Done.

+  ext.backgroundPage.sendMessage(
+    {
+      type: (checkbox.checked) ? "filters.remove" : "filters.add",
+      text: whitelistFilter
+    }, (errors) =>
+    {
+      if (errors.length < 1)
+        return;
+
+      console.error(errors);
+      checkbox.checked = !checkbox.checked;
+    }
+  );
+  ev.preventDefault();
+}
+
+function onMessage(msg)
+{
+  switch (msg.type)
+  {
+    case "app.respond": {
+      switch (msg.action)
+      {
+        case "addSubscription":
+          let [subscription] = msg.args;
+          setDialog("subscribe", {
+            title: subscription.title,
+            url: subscription.url
+          });
+          break;
+        case "showPageOptions":
+          let [{host, whitelisted}] = msg.args;
+          whitelistFilter = `@@||${host}^$document`;

[saroyanm, 2017/07/18 12:46:28]

Shouldn't this be promise as well, similar to the "promisedAcceptableAdsUrl" ?
(If non trivial, this solution still looks fine to me, I don't have strong
opinion about this)

[Thomas Greiner, 2017/07/18 17:42:32]

Even though it's not necessary because the UI using this value will only be
initialized after the value has been set, a promise would indeed be the more
appropriate solution.

In this case, however, we'd need a deferred promise because we can only resolve
the promise after we get the `host` value. Unfortunately, deferred promises have
not been standardized yet so we'd have to implement that logic ourselves (see
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Prom...).

Additionally, we don't even know whether this promise will ever resolve because
the options page could be opened without referring to a domain. Therefore
waiting for the value of the promise doesn't seem to make sense.

Nevertheless, I added a check to `toggleWhitelistFilter()` now to ensure that
dependencies are met.

+
+          ext.i18n.setElementText(
+            get("#enabled-label"),
+            "mops_enabled_label",
+            [host]
+          );
+
+          let checkbox = get("#enabled");
+          checkbox.checked = !whitelisted;
+          checkbox.addEventListener("click", onToggleWhitelistFilter);
+
+          get("#enabled-container").hidden = false;
+          break;
+      }
+      break;
+    }
+    case "filters.respond": {
+      let [filter] = msg.args;
+      if (!whitelistFilter || filter.text != whitelistFilter)

[saroyanm, 2017/07/18 12:46:28]

First check for "!whitelistFilter" seems to redundant until you don't expect
"filter.text" to be undefined or null, in case of undefined hardequation should
be enough "filter.text !== whitelistFilter" I guess.

[Thomas Greiner, 2017/07/18 17:42:31]

While I don't expect `filter.text` to be `null`, it is theoretically possible so
I'd rather avoid undefined behavior where possible. On that note, `null ===
null` is `true` so a strict comparison would not be sufficient to catch this
case.

+        break;
+
+      get("#enabled").checked = (msg.action == "removed");
+      break;
+    }
+    case "subscriptions.respond": {
+      let [subscription] = msg.args;
+      switch (msg.action)
+      {
+        case "added":
+          setSubscription(subscription, true);
+          break;
+        case "disabled":
+          if (subscription.disabled)
+          {
+            removeSubscription(subscription.url);
+          }
+          else
+          {
+            setSubscription(subscription, true);
+          }
+          break;
+        case "removed":
+          removeSubscription(subscription.url);
+          break;
+        case "title":
+          // 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({
+  type: "app.listen",
+  filter: ["addSubscription", "showPageOptions"]
+});
+
+ext.backgroundPage.sendMessage({
+  type: "filters.listen",
+  filter: ["added", "removed"]
+});
+
+ext.backgroundPage.sendMessage({
+  type: "subscriptions.listen",
+  filter: ["added", "disabled", "removed", "title"]
+});
+
+/* Initialization */
+
+populateLists();
+
+getDocLink("acceptable_ads", (link) =>
+{
+  get("#acceptableAds-more").href = link;
+});
+
+get("#dialog-subscribe [name='title']").setAttribute(
+  "placeholder",
+  getMessage("mops_subscribe_title")
+);
+
+get("#dialog-subscribe [name='url']").setAttribute(
+  "placeholder",
+  getMessage("mops_subscribe_url")
+);