Implemented ext.contextMenus for Safari

safari/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 505 matching lines @@
 
   ext.backgroundPage = {
     getWindow: function()
     {
       return safari.extension.globalPage.contentWindow;
     }
   };
 
   ext.onMessage = new MessageEventTarget(safari.application);
 
-  var contextMenu = [];
-  var isContextMenuHidden = false;
+  var contextMenuItems = [];
+  var isContextMenuHidden = true;
   ext.contextMenus = {
     addMenuItem: function(title, contexts, onclick)
     {
-      contextMenu.push({
-        id: "block-element", 

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

So all menu items have the same ID?

How about using "context-menu-item-" + contextMenu.length as identifier? I'm
assuming here that menu item IDs have to be unique across all extensions,
otherwise String(contextMenu.length) will be sufficient - and you will be able
to look up the right menu item in the array without looping.

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

Done. According to Apple "The identifier must be unique within your extension."
https://developer.apple.com/library/safari/documentation/Tools/Conceptual/Saf...

+      contextMenuItems.push({
+        id: String(contextMenuItems.length), 
         title: title,
-        item: null,
         contexts: contexts,
         onclick: onclick
       });
+      this.showMenuItems();
     },
     removeMenuItems: function()
     {
-      contextMenu = [];
-    },
-    showMenu: function()
+      contextMenuItems = [];
+      this.hideMenuItems();
+    },
+    showMenuItems: function()
     {
       isContextMenuHidden = false;
     },
-    hideMenu: function()
+    hideMenuItems: function()
     {
       isContextMenuHidden = true;
     }
   };
 
   // Create context menu items
   safari.application.addEventListener("contextmenu", function(event)
   {
+    if (isContextMenuHidden)
+      return;
+
     var context = event.userInfo.tagName.toLowerCase();

[Wladimir Palant, 2014/01/18 11:13:39]

Nit: toLowerCase() is no longer necessary now that you are using localName.

     if (context == "img")
       context = "image";

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

Seeing that srcUrl can be null, it probably makes sense to add one more check
here:

  if (!event.userInfo.srcUrl)
    context = null;

This will make sure that the context menu will only be shows if context="all" is
requested.

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

Done.

-
-    for (var i = 0; i < contextMenu.length; i++)
-    {
-      if (isContextMenuHidden)
-        return;

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

Why is this check inside the loop? This belongs to the beginning of the
function.

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

Done.

-
+    if (!event.userInfo.srcUrl)
+      context = null;
+
+    for (var i = 0; i < contextMenuItems.length; i++)
+    {
       // Supported contexts are: all, audio, image, video
-      var menuItem = contextMenu[i];
+      var menuItem = contextMenuItems[i];
       if (menuItem.contexts.indexOf("all") == -1 && menuItem.contexts.indexOf(context) == -1)
-        return;

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

This should be continue, not return - there can be multiple menu items.

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

Done.

+        continue;
       
-      menuItem.item = event.contextMenu.appendContextMenuItem(menuItem.id, menuItem.title);

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

I don't think that menuItem.item is being used, no point setting it.

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

Done.

+      event.contextMenu.appendContextMenuItem(menuItem.id, menuItem.title);
     }
   }, false);
 
   // Handle context menu item clicks
   safari.application.addEventListener("command", function(event)
   {
     for (var i = 0; i < contextMenu.length; i++)
     {
       if (contextMenu[i].id == event.command)
       {
         contextMenu[i].onclick(event.userInfo.srcUrl, new Tab(safari.application.activeBrowserWindow.activeTab));
         break;
       }
     }
   }, false);
 
   // Safari will load the bubble once, and then show it everytime the icon is
   // clicked. While Chrome loads it everytime you click the icon. So in order to
   // force the same behavior in Safari, we are going to reload the page of the
   // bubble everytime it is shown.
   if (safari.extension.globalPage.contentWindow != window)
     safari.application.addEventListener("popover", function()
     {
       document.documentElement.style.display = "none";
       document.location.reload();
     }, true);
 })();