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 togglePref */

[Sebastian Noack, 2017/10/08 19:17:22]

As I already told Manish on a different review, I'd like to see popup.js,
notifications.js and stats.js being merged into one file, rather than having
them share globals across files. Regardless, of the situation with the globals,
I think it is is rather confusing to have the code for the popup spread out that
much. But of course, this should be done with a seperate patch.

[kzar, 2017/10/09 10:33:25]

Acknowledged.

+/* global setPref */
 
 "use strict";
 
 function getDocLinks(notification)
 {
-  let docLinks = [];

[Manish Jethani, 2017/10/08 22:31:34]

The variable is unnecessary here.

[kzar, 2017/10/09 10:33:25]

Sure, we could return an empty array if notification.links is falsey and then
initialise the docLinks variable to an empty array otherwise. I preferred it
this way since the same empty array is used, but whatever. If you insist I'll
change it, I don't care too much either way.

[Manish Jethani, 2017/10/09 10:54:12]

I must be looking at the wrong diff, because to me it looks like it's only being
used in one place.

[kzar, 2017/10/09 15:10:54]

Oh yea, you are right sorry. I switched to mapping over notification.links
directly.

Done.

-
   if (!notification.links)
-    return Promise.resolve(docLinks);
+    return Promise.resolve([]);
 
   return Promise.all(
     notification.links.map(link =>
     {
       return new Promise((resolve, reject) =>

[Manish Jethani, 2017/10/08 22:31:34]

If you prefer it, you could lose the braces and the return keyword and lose one
level of indentation here.

Also, reject is not being used so it could be skipped (then you could lose
another level of indentation too).

Just a suggestion, it looks fine as it is.

[kzar, 2017/10/09 10:33:25]

Yea I considered that, but I figured it looked nicer with the braces in this
instance. I don't feel too strongly either way, I guess I'll leave it like it
is.

[Manish Jethani, 2017/10/09 10:54:12]

Acknowledged.

       {
         chrome.runtime.sendMessage({
           type: "app.get",
           what: "doclink",
           link
         }, resolve);
       });
     })
   );
 }
@@ skipping 44 matching lines @@
 
       messageElement.addEventListener("click", event =>
       {
         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);
+        chrome.tabs.create({url: 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")
-          togglePref("notifications_ignoredcategories");

[Manish Jethani, 2017/10/08 22:31:34]

The "prefs.toggle" implementation in messageResponder.js does not pass true as
the second argument here, which means it will actually get toggled:

    if (message.key == "notifications_ignoredcategories")
      return NotificationStorage.toggleIgnoreCategory("*");

Really we want to set the value to true, like so:

    if (message.key == "notifications_ignoredcategories")
      return NotificationStorage.toggleIgnoreCategory("*", true);

But it's also called from elsewhere. I think we have to pass the value of
message.forceValue here.

Either that, or perhaps have a dedicated message for this, since we aren't
really "toggling" here, we're just setting the value.

[Manish Jethani, 2017/10/08 22:40:59]

Since we're adding "notifications.get", we could add another one called
"notifications.ignoreAllCategories" or "notifications.optOut"

[kzar, 2017/10/09 10:33:25]

Well my understanding was the option would be toggled, so if it's true it would
be come false and vice versa. I just checked the implementation and it looks
about right, so I'm not sure I agree that the value won't actually get toggled.
Am I missing something?
This is true, and kind of contradicts what you said above. Anyway since ignored
notifications won't be displayed or therefore clicked, we can assume the
notification _isn't_ ignored at this point right? Therefore we can assume that
toggling it will set it to be ignored.

[Manish Jethani, 2017/10/09 10:54:12]

The reason the original code was passing true as the second argument is to force
the value to be true, regardless of the current value. I'm not sure if it
matters though, as you said, but see below.
"Relentless" notifications are always displayed, I think, regardless of the
setting. Anyway I'm not sure that passing true or not makes a difference, maybe
Thomas can throw some light on this. If you're sure though then it looks OK to
me.

[Manish Jethani, 2017/10/09 11:53:52]

Looking at this code in adblockpluscore lib/notification.js:

  if (notification.type !== "relentless" &&
      Prefs.notifications_ignoredcategories.indexOf("*") != -1)
  {
    continue;
  }

So it seems even if the user has opted out of all notifications, the
"relentless" type are still displayed, and in that case the user would have the
option of clicking on the opt out button again (I'm not sure this is good UX but
whatever). If they choose to "opt out" in this scenario, they would end up
opting in instead. So I think we need to pass true there as the second argument.

[kzar, 2017/10/09 15:10:54]

Done.

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