issue 1435 - Port popup.html from Chrome/Safari/Opera to Firefox

chrome/content/ui/popup.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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/>.
  */
 
 var backgroundPage = ext.backgroundPage.getWindow();
-window["openOptions"] = backgroundPage["openOptions"];
-
-var tab = null;
+var openOptions = backgroundPage.openOptions;
+var page = null;
 
 function init()
 {
-  // Mark page as local to hide non-relevant elements
-  ext.windows.getLastFocused(function(win)
+  ext.pages.query({active: true, lastFocusedWindow: true}, function(pages)
   {
-    win.getActiveTab(function(t)
+    page = pages[0];
+    if (page)
     {
-      tab = t;
+      document.body.classList.toggle("local", !/^https?:\/\//.test(page.url));
 
-      if (!/^https?:\/\//.exec(tab.url))

[Thomas Greiner, 2014/09/29 16:19:49]

You can replace both the check and the class assignment with the following:

`document.body.classList.toggle("local", !/^https?:\/\//.test(tab.url);`

[saroyanm, 2014/10/02 07:53:56]

Awesome! Didn't know about second attribute.

-        document.body.classList.add("local");
-    });
+      // Ask content script whether clickhide is active. If so, show cancel button.
+      page.sendMessage({type: "get-clickhide-state"}, function(response)
+      {
+        if (response && response.active)
+          document.body.classList.add("clickhide-active");
+      });
+    }
   });
-  
+
   // Initialize features
   initClickHide();
   initReportIssue();
   initBlockable();
 
   document.getElementById("options").addEventListener("click", function()
   {
     openOptions();
   }, false);
 }
 window.addEventListener("DOMContentLoaded", init, false);
 
 function initClickHide()
 {
   if (!("activateClickHide" in ext))
     return;
 
-  var menuItem1 = document.getElementById("clickhide");

[Thomas Greiner, 2014/09/29 16:19:49]

A bit more descriptive variable names would be nice (e.g. menuItem1 ->
menuItemActivate, menuItem2 -> menuItemCancel).

[saroyanm, 2014/10/02 07:53:56]

Done.

-  menuItem1.addEventListener("click", activateClickHide, false);
-  menuItem1.removeAttribute("hidden");
-  var menuItem2 = document.getElementById("clickhide-cancel");
-  menuItem2.addEventListener("click", cancelClickHide, false);
-  menuItem2.removeAttribute("hidden");
-
-  // Ask content script whether clickhide is active. If so, show cancel button.
-  // If that isn't the case, ask background.html whether it has cached filters. If so,
-  // ask the user whether she wants those filters.
-  // Otherwise, we are in default state.

[Thomas Greiner, 2014/09/29 16:19:49]

The description doesn't need to mention what's going on in the background page
so reducing it to the first two sentences should be sufficient.

[saroyanm, 2014/10/02 07:53:56]

Done.

-  ext.windows.getLastFocused(function(win)
-  {
-    win.getActiveTab(function(tab)
-    {
-      tab.sendMessage({type: "get-clickhide-state"}, function(response)
-      {
-        if (response && response.active)
-          document.body.classList.add("clickhide-active");
-      });
-    });
-  });
+  var menuItemActivate = document.getElementById("clickhide");
+  menuItemActivate.addEventListener("click", activateClickHide, false);
+  menuItemActivate.removeAttribute("hidden");
+  var menuItemCancel = document.getElementById("clickhide-cancel");
+  menuItemCancel.addEventListener("click", cancelClickHide, false);
+  menuItemCancel.removeAttribute("hidden");
 }
 
 function initReportIssue()
 {
   if (!("reportIssue" in ext) || !ext.showReportIssue())
     return;
 
   var menuItem = document.getElementById("report-issue");
   menuItem.addEventListener("click", function()
   {
     ext.reportIssue();
   }, false);
   menuItem.removeAttribute("hidden");
 }
 
 function initBlockable()
 {
-  if (!ext.showBlockable())

[Thomas Greiner, 2014/09/29 16:19:49]

In regard to my suggestion to get rid of showBlockable, change this to

`if (!("openBlockable" in ext))`

[saroyanm, 2014/10/02 07:53:56]

I guess I'm not catching on this part.
Can you please clarify why we should look for "openBlockable" parameter in ext ?

In case we would like to have similar implementation as in ui.js, then maybe we
should have something like:
var bottombarOpen = UI.isBottombarOpen(topWindow);
if (bottombarOpen)
  return;

var menuItem = document.getElementById("blockable");
menuItem.addEventListener("click", function()
{
  UI.toggleBottombar(topWindow);
  ext.closePopup();
}, false);
menuItem.removeAttribute("hidden");

But I think current implementation is more readable while we are importing UI
module in ext/popup.js.

[Thomas Greiner, 2014/10/08 10:40:43]

This menu item should always be shown on platforms that provide
ext.openBlockable. Therefore we only need to check for the existence of that
function.
In its current state this code hides that menu item when the bottom panel is
shown. However, this is inconsistent with the implementation in current Adblock
Plus versions which always show that menu items.

[saroyanm, 2014/10/10 12:05:58]

Got it, please let me know if you think we can give a go with new
implementation.

[Thomas Greiner, 2014/10/13 13:18:23]

The new implementation looks good.

+  if (!("openBlockable" in ext))
     return;
 
   var menuItem = document.getElementById("blockable");
+  menuItem.classList.toggle("open", ext.isBlockableOpen());
   menuItem.addEventListener("click", function()
   {
     ext.openBlockable();
   }, false);
   menuItem.removeAttribute("hidden");
 }
 
 function activateClickHide()
 {
   document.body.classList.add("clickhide-active");
   ext.activateClickHide(true);
 
   // Close the popup after a few seconds, so user doesn't have to
   activateClickHide.timeout = window.setTimeout(ext.closePopup, 5000);
 }
 
 function cancelClickHide()
 {
   if (activateClickHide.timeout)
   {
     window.clearTimeout(activateClickHide.timeout);
     activateClickHide.timeout = null;
   }
   document.body.classList.remove("clickhide-active");
   ext.activateClickHide(false);
 }