Implemented ext.contextMenus for Safari

chrome/background.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 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 352 matching lines @@
     onCompleted: new CompletedTabEventTarget(),
     onActivated: new ActivatedTabEventTarget(),
     onRemoved: new RemovedTabEventTarget()
   };
 
   ext.webRequest = {
     onBeforeRequest: new BeforeRequestEventTarget(),
     handlerBehaviorChanged: chrome.webRequest.handlerBehaviorChanged
   };
 
-  var contextMenu = [];

[Wladimir Palant, 2014/01/17 15:36:55]

Nit: Would contextMenuItems be a better name?

[Thomas Greiner, 2014/01/18 10:32:05]

Done.

+  var contextMenuItems = [];
+  var isContextMenuHidden = true;
   ext.contextMenus = {
     addMenuItem: function(title, contexts, onclick)
     {
-      contextMenu.push({
+      contextMenuItems.push({
         title: title,
         contexts: contexts,
         onclick: function(info, tab)
         {
           onclick(info.srcUrl, new Tab(tab));
         }
       });
+      this.showMenuItems();
     },
     removeMenuItems: function()
     {
-      contextMenu = [];
-    },
-    showMenu: function()
-    {
+      contextMenuItems = [];
+      this.hideMenuItems();
+    },
+    showMenuItems: function()
+    {
+      if (!isContextMenuHidden)
+        return;
+
       chrome.contextMenus.removeAll(function()
       {
-        for (var i = 0; i < contextMenu.length; i++)
-        {
-          var item = contextMenu[i];
+        for (var i = 0; i < contextMenuItems.length; i++)
+        {
+          var item = contextMenuItems[i];
           chrome.contextMenus.create({
             title: item.title,
             contexts: item.contexts,
             onclick: item.onclick
           });
         }
       });
-    },
-    hideMenu: function()

[Wladimir Palant, 2014/01/17 15:36:55]

Nit: These methods should be called showMenuItems/hideMenuItems to indicate that
they affect all menu items.

[Thomas Greiner, 2014/01/18 10:32:05]

Done.

-    {
+      isContextMenuHidden = false;
+    },
+    hideMenuItems: function()
+    {
+      if (isContextMenuHidden)
+        return;
+
       chrome.contextMenus.removeAll();
-    }
-  };
-
+      isContextMenuHidden = true;
+    }
+  };
 })();

[Wladimir Palant, 2014/01/17 15:36:55]

The API here is somewhat unexpected - addMenuItem and removeMenuItems have no
immediate effect. I would expect an internal flag variable isContextMenuHidden -
false by default, set in showMenu and hideMenu, same as for Safari. If
isContextMenuHidden is true then addMenuItem calls showMenu and removeMenuItems
calls hideMenu automatically. Also, showMenu can check that flag and return
early if shown is already true - no point wasting effort here. Note that this is
already how it works in Safari.

[Thomas Greiner, 2014/01/18 10:32:05]

Done. However, I decided to go with true as the default value for
isContextMenuHidden because it makes more sense to have it hidden by default and
only show it when something has been added to it.
I also changed this in the Safari implementation to make it consistent.