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

notification.js

Index: notification.js
diff --git a/notification.js b/notification.js
index 18eff540b3dddc0664594f2fce064a2e3f742183..14b21733a2dac5fe7a058ad62cfd87aebac6c886 100644
--- a/notification.js
+++ b/notification.js
@@ -17,23 +17,48 @@
 
 "use strict";
 
-const {require} = ext.backgroundPage.getWindow();
-
-const {Utils} = require("utils");
-const {Notification} = require("notification");
-const {getActiveNotification, shouldDisplay} = require("notificationHelper");
-
 function getDocLinks(notification)
 {
+  let docLinks = [];
+
   if (!notification.links)
-    return [];
+    return Promise.resolve(docLinks);
 
-  let docLinks = [];
-  notification.links.forEach(link =>
+  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.

+          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) =>
   {
-    docLinks.push(Utils.getDocLink(link));
+    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); });
+      }
+    });
   });
-  return docLinks;
 }
 
 function insertMessage(element, text, links)
@@ -63,45 +88,59 @@ function insertMessage(element, text, links)
 
 window.addEventListener("load", () =>
 {
-  let notification = getActiveNotification();
-  if (!notification || !shouldDisplay("popup", notification.type))
-    return;
-
-  let texts = Notification.getLocalizedTexts(notification);
-  let titleElement = document.getElementById("notification-title");
-  titleElement.textContent = texts.title;
-
-  let docLinks = getDocLinks(notification);
-  let messageElement = document.getElementById("notification-message");
-  insertMessage(messageElement, texts.message, docLinks);
-
-  messageElement.addEventListener("click", event =>
+  getActiveDisplayedNotification().then(notification =>
   {
-    let link = event.target;
-    while (link && link !== messageElement && link.localName !== "a")
-      link = link.parentNode;
-    if (!link)
+    if (!notification)
       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")
+    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 =>
     {
-      if (event.target.id == "notification-optout")
-        Notification.toggleIgnoreCategory("*", true);
+      titleElement.textContent = texts.title;
+
+      getDocLinks(notification).then(docLinks =>
+      {
+        insertMessage(messageElement, texts.message, docLinks);
+      });
+    });
 
-      notificationElement.hidden = true;
-      notification.onClicked();
-    }
-  }, true);
+    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"
+          });
+        }
+
+        notificationElement.hidden = true;
+        notification.onClicked();
+      }
+    }, true);
+  });
 }, false);