Issue 5593 - Use messaging for the popup's notification code

notification.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
  * 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/>.
  */
 
+/* global setPref */
+
 "use strict";
 
 function getDocLinks(notification)
 {
-  let docLinks = [];
-
   if (!notification.links)
-    return Promise.resolve(docLinks);
+    return Promise.resolve([]);
 
   return Promise.all(
     notification.links.map(link =>
     {
       return new Promise((resolve, reject) =>
       {
-        ext.backgroundPage.sendMessage({

[Manish Jethani, 2017/10/06 14:26:04]

We should use runtime.sendMessage here so we lose the dependence on ext.*, we're
doing the same in popup.js

[kzar, 2017/10/06 20:05:34]

Done.

+        chrome.runtime.sendMessage({
           type: "app.get",
           what: "doclink",
           link
         }, resolve);
       });
     })
   );
-}
-
-function getActiveDisplayedNotification()

[Manish Jethani, 2017/10/06 14:26:04]

Nit: I feel like "displayed" implies it's already displayed, whereas we're about
to display it here, so perhaps getActiveNotificationForDisplay.

[kzar, 2017/10/06 20:05:34]

Acknowledged.

-{
-  return new Promise((resolve, reject) =>
-  {
-    ext.backgroundPage.sendMessage({
-      type: "notifications.getActive"
-    }, notification =>

[Manish Jethani, 2017/10/06 14:26:05]

I may be wrong about this, but "notification =>" here (the second argument)
needs to start on the next line, at least according to ESLint's new indentation
rules (which we have disabled for now).

Same for "shouldDisplay =>" below.

[kzar, 2017/10/06 20:05:34]

I think this looks OK like it is. If we decide to enforce that rule with ESLint
in the future we can adjust it though.

-    {
-      if (!notification)

[Manish Jethani, 2017/10/06 14:26:05]

We would normally add braces here around the if block, if I'm not wrong.

[kzar, 2017/10/06 20:05:34]

IIRC we considered enforcing that braces for if... else... blocks should be
consistent but decided against it.

-        resolve(notification);
-      else
-      {
-        ext.backgroundPage.sendMessage({
-          type: "notifications.shouldDisplay",
-          method: "popup",
-          notificationType: notification.notificationType

[Manish Jethani, 2017/10/06 14:26:05]

It should be notification.type, not notification.notificationType? At least
that's the impression I got from the previous code.

[kzar, 2017/10/06 20:05:34]

Acknowledged.

-        }, shouldDisplay => { resolve(shouldDisplay && notification); });
-      }
-    });
-  });
 }
 
 function insertMessage(element, text, links)
 {
   let match = /^(.*?)<(a|strong)>(.*?)<\/\2>(.*)$/.exec(text);
   if (!match)
   {
     element.appendChild(document.createTextNode(text));
     return;
   }
 
   let before = match[1];
   let tagName = match[2];
   let value = match[3];
   let after = match[4];
 
   insertMessage(element, before, links);
 
   let newElement = document.createElement(tagName);
-  if (tagName === "a" && links && links.length)
+  if (tagName == "a" && links && links.length)
     newElement.href = links.shift();
   insertMessage(newElement, value, links);
   element.appendChild(newElement);
 
   insertMessage(element, after, links);
 }
 
 window.addEventListener("load", () =>
 {
-  getActiveDisplayedNotification().then(notification =>
+  chrome.runtime.sendMessage({
+    type: "notifications.get",
+    displayMethod: "popup"
+  }, notification =>
   {
     if (!notification)
       return;
 
     let titleElement = document.getElementById("notification-title");
     let messageElement = document.getElementById("notification-message");
 
-    ext.backgroundPage.sendMessage({
-      type: "notifications.getLocalizedTexts",
-      notification

[Manish Jethani, 2017/10/06 14:26:04]

I'm not sure how message.locale gets in there. Is it added by the browser
automatically?

[kzar, 2017/10/06 20:05:34]

If not specified we go with the default I think.

-    }, texts =>
+    titleElement.textContent = notification.texts.title;
+
+    getDocLinks(notification).then(docLinks =>
     {
-      titleElement.textContent = texts.title;
+      insertMessage(messageElement, notification.texts.message, docLinks);
 
-      getDocLinks(notification).then(docLinks =>
+      messageElement.addEventListener("click", event =>
       {
-        insertMessage(messageElement, texts.message, docLinks);
+        let link = event.target;
+        while (link && link != messageElement && link.localName != "a")
+          link = link.parentNode;
+        if (!link)
+          return;
+        event.preventDefault();
+        event.stopPropagation();
+        chrome.tabs.create({url: link.href});
       });
-    });
-
-    messageElement.addEventListener("click", event =>

[Manish Jethani, 2017/10/06 14:26:04]

I feel like this should be added in the getDocLinks handler, after
insertMessage.

[kzar, 2017/10/06 20:05:34]

Done.

-    {
-      let link = event.target;
-      while (link && link !== messageElement && link.localName !== "a")
-        link = link.parentNode;
-      if (!link)
-        return;
-      event.preventDefault();
-      event.stopPropagation();
-      ext.pages.open(link.href);
     });
 
     let notificationElement = document.getElementById("notification");
     notificationElement.className = notification.type;
     notificationElement.hidden = false;
     notificationElement.addEventListener("click", event =>
     {
       if (event.target.id == "notification-close")
         notificationElement.classList.add("closing");
       else if (event.target.id == "notification-optout" ||
                event.target.id == "notification-hide")
       {
         if (event.target.id == "notification-optout")
-        {
-          ext.backgroundPage.sendMessage({

[Manish Jethani, 2017/10/06 14:26:04]

You meant to use the togglePref function here, defined in popup.js?

[kzar, 2017/10/06 20:05:34]

Done.

-            type: "notifications_ignoredcategories"
-          });
-        }
+          setPref("notifications_ignoredcategories", true);
 
         notificationElement.hidden = true;
         notification.onClicked();
       }
     }, true);
   });
 }, false);