Issue 5593 - Use messaging in popup for prefs, whitelisting, and stats

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
@@ skipping 64 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});

[Sebastian Noack, 2017/09/20 18:57:02]

I'm all for replacing usage of ext.* wit chrome.* (eventually getting rid
completely of the ext namepsace). But this change seems to be unrelated here.
Can you please submit a separate patch, that is replacing ext.pages.open with
chrome.tabs.create, not only here, but everywhere?

[Manish Jethani, 2017/09/21 06:11:16]

Actually the goal of this set of changes was to get rid of any dependence on the
background page, therefore we had to get rid of uses of ext.pages or ext.Page as
part of these changes. It is definitely not unrelated.
I can file an issue for the general case of getting rid of ext.pages and others
throughout the codebase.

[Sebastian Noack, 2017/09/21 22:57:16]

Sure, but there seems to be much more to do than done in this patch in order to
get rid of that dependency. So changing this call here seemed unrelated to the
other changes here. It's not too important, but if you split it out we could get
rid of other usage of ext.tabs.open() in the same go.

[Manish Jethani, 2017/09/24 22:37:23]

OK, I thought we were going to make all the changes to remove the dependence on
the background page in one commit, and one code review, but do it incrementally.
Is this still true? If we've decided to split it into multiple commits and code
reviews then it makes sense, in that case I'll move these changes over to
another code review.

[Sebastian Noack, 2017/09/25 17:50:51]

Well, Wladimir wasn't happy with that approach, and at the current point I don't
care if we land this effort in progressive steps. However, we should break it
down in logically contained patches at least, and the change here seems slightly
too much unrelated of the messaging changes, in particular if we could get rid
of other usage of chrome.tabs.create() in the same go.

Other than that, the changes look good to me now.

[Manish Jethani, 2017/09/26 23:13:26]

OK, backed out this change.

I like doing it in separate commits as well FWIW, makes it easier overall.

   });
 
   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")
         Notification.toggleIgnoreCategory("*", true);
 
       notificationElement.hidden = true;
       notification.onClicked();
     }
   }, true);
 }, false);