Applied changes from emailed code review

lib/appIntegration.js

 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-// Extensions like Tab Mix Plus will try to recompile our handlers and fail
-// badly - they manage to replace our handler but the recompiled version
-// won't work because the closure is missing its variables. We replace
-// toString() function to prevent this kind of recompiling on earlier stages
-// (produce a syntax error but still leave the source code viewable).
-function doNotRecompile()
+let {hook} = require("hooks");
+let functionHooks = new WeakMap();
+
+exports.removeFromWindow = function(window)
 {
-  let result = Function.prototype.toString.apply(this);
-  return result + "\n$%&!/DO_NOT_RECOMPILE"
-}
+  if (functionHooks.has(window))
+  {
+    let unhook = functionHooks.get(window);
+    unhook();
+    functionHooks.delete(window);
+  }
+};
 
 let {application} = require("info");
 switch (application)
 {
   case "firefox":
   {
     // Firefox
     exports.isKnownWindow = function(window) window.document.documentElement.getAttribute("windowtype") == "navigator:browser";
 
     exports.getURLBar = function(window) "gURLBar" in window ? window.gURLBar : null;
 
     exports.getBrowser = function(window) "gBrowser" in window ? window.gBrowser : null;
 
     exports.applyToWindow = function(window, corrector)
     {
       let urlbar = exports.getURLBar(window);
-      if (urlbar && urlbar.handleCommand && !("urlfixerOldHandler" in urlbar.handleCommand))
+      if (urlbar && urlbar.handleCommand && !functionHooks.has(window))
       {
         // Handle new URLs being entered
-        let oldHandler = urlbar.handleCommand;
-        urlbar.handleCommand = function()
-        {
-          try
-          {
-            let correction = corrector(window, urlbar.value);
-            if (correction)
-              urlbar.value = correction;
-          }
-          catch(e)
-          {
-            if (e == Cr.NS_BINDING_ABORTED)
-              return;
-            else
-              Cu.reportError(e);
-          }
-          oldHandler.apply(this, arguments);
-        }
-        urlbar.handleCommand.urlfixerOldHandler = oldHandler;
-        urlbar.handleCommand.toString = doNotRecompile;
-      }
-    };
-
-    exports.removeFromWindow = function(window)
-    {
-      let urlbar = exports.getURLBar(window);
-      if (urlbar && urlbar.handleCommand && "urlfixerOldHandler" in urlbar.handleCommand)
-        urlbar.handleCommand = urlbar.handleCommand.urlfixerOldHandler;
-    };
-    
+        let unhook = hook(urlbar, "handleCommand", function()
+        {
+          let correction = corrector(window, urlbar.value);
+          if (correction)
+            urlbar.value = correction;
+        });
+        functionHooks.set(window, unhook);
+      }
+    };
+
     exports.openInfobar = function(window, id, message, buttons, persistence)
     {
       let browser = exports.getBrowser(window);
       let infobar = browser.getNotificationBox();
-      let notif = infobar.getNotificationWithValue(id);

[Wladimir Palant, 2012/09/28 09:29:34]

As mentioned before - you don't need to save characters in an extension and your
editor has autocomplete. So nothing wrong with writing out "notification".

-      
-      if (notif)
-      {
-        infobar.removeNotification(notif);
-      }
-      
-      notif = infobar.appendNotification(
+      let notification = infobar.getNotificationWithValue(id);
+
+      if (notification)
+      {
+        infobar.removeNotification(notification);
+      }
+
+      notification = infobar.appendNotification(
         message,
         id,
         require("info").addonRoot + "icon64.png",
         infobar.PRIORITY_INFO_HIGH,
         buttons
       );
-      notif.persistence = persistence;
-    };
-    
-    exports.loadURI = function(uri)
-    {
-      exports.getBrowser(Services.wm.getMostRecentWindow("navigator:browser")).loadURI(uri);

[Wladimir Palant, 2012/09/28 09:29:34]

This will load the address in *some* browser window which is not necessarily the
one we want. It's bad enough that we don't specify the tab - but assuming that
the right tab is selected when the user clicks a notification button isn't
unreasonable. We should pass the window as a parameter explicitly however.

(Note that this issue was addressed in one of my changes)

+      notification.persistence = persistence;
+    };
+
+    exports.loadURI = function(window, uri)
+    {
+      exports.getBrowser(window).loadURI(uri);
     };
 
     break;
   }
   case "seamonkey":
   {
+    let eventListeners = new WeakMap();
+
     // SeaMonkey
     exports.isKnownWindow = function(window) window.document.documentElement.getAttribute("windowtype") == "navigator:browser";
 
     exports.getURLBar = function(window) "gURLBar" in window ? window.gURLBar : null;
 
     exports.getBrowser = function(window) "gBrowser" in window ? window.gBrowser : null;
 
     exports.applyToWindow = function(window, corrector)
     {
       let urlbar = exports.getURLBar(window);
-      if (urlbar && window.handleURLBarCommand && !("urlfixerOldHandler" in window.handleURLBarCommand))
-      {
-        // Handle new URLs being entered
-        let oldHandler = window.handleURLBarCommand;
-        window.handleURLBarCommand = function()
-        {
-          try
+      let goButton = window.document.getElementById("go-button-container");
+
+      if (urlbar && urlbar._fireEvent && !functionHooks.has(window))
+      {
+        function correctURL()
+        {
+          let correction = corrector(window, urlbar.value);
+          if (correction)
+            urlbar.value = correction;
+        }
+
+        let unhook = hook(urlbar, "_fireEvent", function(eventType)
+        {
+          if (eventType == "textentered")
           {
-            let correction = corrector(window, urlbar.value);
-            if (correction)
-              urlbar.value = correction;
+            correctURL();
           }
-          catch(e)
-          {
-            if (e == Cr.NS_BINDING_ABORTED)
-              return;
-            else
-              Cu.reportError(e);
-          }
-          oldHandler.apply(this, arguments);
-        }
-        window.handleURLBarCommand.urlfixerOldHandler = oldHandler;
-        window.handleURLBarCommand.toString = doNotRecompile;
-      }
-    };
-
+        });
+        functionHooks.set(window, unhook);
+
+        if (goButton)
+        {
+          goButton.addEventListener("command", correctURL, true);
+          eventListeners.set(window, {
+            "listener": correctURL,
+            "element": goButton
+          });
+        }
+      }
+    };
+
+    let basicRemove = exports.removeFromWindow;
     exports.removeFromWindow = function(window)
     {
-      if (window.handleURLBarCommand && "urlfixerOldHandler" in window.handleURLBarCommand)
-        window.handleURLBarCommand = window.handleURLBarCommand.urlfixerOldHandler;
-    };
-    
+      basicRemove(window);
+
+      if (eventListeners.has(window))
+      {
+        let eventListener = eventListeners.get(window);
+        eventListener.element.removeEventListener("command", eventListener.listener, true);
+        eventListeners.delete(window);
+      }
+    };
+
     exports.openInfobar = function(window, id, message, buttons, persistence)
     {
       let browser = exports.getBrowser(window);
       let infobar = browser.getNotificationBox();
-      let notif = infobar.getNotificationWithValue(id);
-      
-      if (notif)
-      {
-        infobar.removeNotification(notif);
-      }
-      
-      notif = infobar.appendNotification(
+      let notification = infobar.getNotificationWithValue(id);
+
+      if (notification)
+      {
+        infobar.removeNotification(notification);
+      }
+
+      notification = infobar.appendNotification(
         message,
         id,
         require("info").addonRoot + "icon64.png",
         infobar.PRIORITY_INFO_HIGH,
         buttons
       );
-      notif.persistence = persistence;
-    };
-    
-    exports.loadURI = function(uri)
-    {
-      exports.getBrowser(Services.wm.getMostRecentWindow("navigator:browser")).loadURI(uri);
+      notification.persistence = persistence;
+    };
+
+    exports.loadURI = function(window, uri)
+    {
+      exports.getBrowser(window).loadURI(uri);
     };
 
     break;
   }
   case "fennec":
   {
     // XUL Fennec
     exports.isKnownWindow = function(window) window.document.documentElement.getAttribute("windowtype") == "navigator:browser";
 
     exports.getURLBar = function(window) null;
 
     exports.getBrowser = function(window) null;
 
     exports.applyToWindow = function(window, corrector)
     {
-      if ("BrowserUI" in window && window.BrowserUI.goToURI && !("urlfixerOldHandler" in window.BrowserUI.goToURI))
+      if ("BrowserUI" in window && window.BrowserUI.goToURI && !functionHooks.has(window))
       {
         // Handle new URLs being entered
-        let oldHandler = window.BrowserUI.goToURI;
-        window.BrowserUI.goToURI = function(url)
+        let unhook = hook(window.BrowserUI, "goToURI", function(url)
         {
           url = url || this._edit.value;
-          try
-          {
-            let correction = corrector(window, url);
-            if (correction)
-              url = correction;
-          }
-          catch(e)
-          {
-            if (e == Cr.NS_BINDING_ABORTED)
-              return;
-            else
-              Cu.reportError(e);
-          }
-          oldHandler.call(this, url);
-        }
-        window.BrowserUI.goToURI.urlfixerOldHandler = oldHandler;
-        window.BrowserUI.goToURI.toString = doNotRecompile;
-      }
-    };
-
-    exports.removeFromWindow = function(window)
-    {
-      if ("BrowserUI" in window && window.BrowserUI.goToURI && "urlfixerOldHandler" in window.BrowserUI.goToURI)
-        window.BrowserUI.goToURI = window.BrowserUI.goToURI.urlfixerOldHandler;
-    };
-    
-    exports.openInfobar = function()
-    {
-      // TODO: Implement infobar
-    };
-    
-    exports.loadURI = function(window, uri)
-    {
-      // TODO: Implement infobar
+
+          let correction = corrector(window, url);
+          if (correction)
+            url = correction;
+
+          return [url];
+        });
+        functionHooks.set(window, unhook);
+      }
+    };
+
+    exports.openInfobar = function(window, id, message, buttons, persistence)
+    {
+      if ("getNotificationBox" in window)
+      {
+        let infobar = window.getNotificationBox();
+        let notification = infobar.getNotificationWithValue(id);
+
+        if (notification)
+        {
+          infobar.removeNotification(notification);
+        }
+
+        notification = infobar.appendNotification(
+          message,
+          id,
+          require("info").addonRoot + "icon64.png",
+          infobar.PRIORITY_INFO_HIGH,
+          buttons
+        );
+        notification.persistence = persistence;
+      }
+    };
+
+    exports.loadURI = function(window, uri)
+    {
+      if ("BrowserUI" in window && "goToURI" in window.BrowserUI)
+      {
+        window.BrowserUI.goToURI(uri);
+      }
     };
 
     break;
   }
   case "fennec2":
   {
     // Native Fennec
     exports.isKnownWindow = function(window) window.document.documentElement.getAttribute("windowtype") == "navigator:browser";
 
     exports.getURLBar = function(window) null;
 
     exports.getBrowser = function(window) null;
 
     exports.applyToWindow = function(window, corrector)
     {
-      if ("BrowserApp" in window && window.BrowserApp.observe && !("urlfixerOldHandler" in window.BrowserApp.observe))
-      {
-        let oldHandler = window.BrowserApp.observe;
-        let oldFunc = null;
-        let handler = function()
-        {
-          let params = Array.prototype.slice.apply(arguments);
-          try
-          {
-            let correction = corrector(window, params[0]);
-            if (correction)
-              params[0] = correction;
-          }
-          catch(e)
-          {
-            if (e == Cr.NS_BINDING_ABORTED)
-              return null;
-            else
-              Cu.reportError(e);
-          }
-          return oldFunc.apply(this, params);
-        };
-
-        window.BrowserApp.observe = function(subject, topic, data)
+      if ("BrowserApp" in window && window.BrowserApp.observe && !functionHooks.has(window))
+      {
+        let innerUnhook = null;
+        function cleanup()
+        {
+          if (innerUnhook)
+            innerUnhook();
+
+          innerUnhook = null;
+        }
+
+        let unhook = hook(window.BrowserApp, "observe", function(subject, topic, data)
         {
           // Huge hack: we replace addTab/loadURI when the observer is
           // triggered. This seems to be the only way to know that the calls
           // originate from user input.
           let method = null;
           if (topic == "Tab:Add")
             method = "addTab";
           else if (topic == "Tab:Load")
             method = "loadURI";
 
           if (method)
           {
-            oldFunc = this[method];
-            this[method] = handler;
+            innerUnhook = hook(this, method, function()
+            {
+              let params = Array.prototype.slice.apply(arguments);
+              let correction = corrector(window, params[0]);
+              if (correction)
+                params[0] = correction;
+              return params;
+            });
           }
-
-          try
-          {
-            oldHandler.apply(this, arguments);
-          }
-          finally
-          {
-            if (method)
-              this[method] = oldFunc;
-          }
-        };
-        window.BrowserApp.observe.urlfixerOldHandler = oldHandler;
-        window.BrowserApp.observe.toString = doNotRecompile;
-      }
-    };
-
-    exports.removeFromWindow = function(window)
-    {
-      if ("BrowserApp" in window && window.BrowserApp.observe && "urlfixerOldHandler" in window.BrowserApp.observe)
-        window.BrowserApp.observe = window.BrowserApp.observe.urlfixerOldHandler;
-    };
-    
+        }, cleanup);
+        functionHooks.set(window, unhook);
+      }
+    };
+
     exports.openInfobar = function(window, id, message, buttons, persistence)
     {
       if ("BrowserApp" in window && "selectedTab" in window.BrowserApp)
       {
         window.NativeWindow.doorhanger.show(message, id, buttons, window.BrowserApp.selectedTab.id,
           {
             persistence: persistence
           }
         );
       }
     };
-    
-    exports.loadURI = function(uri)
-    {
-      let window = Services.wm.getMostRecentWindow("navigator:browser");
+
+    exports.loadURI = function(window, uri)
+    {
       if ("BrowserApp" in window && "loadURI" in window.BrowserApp)
         window.BrowserApp.loadURI(uri);
     };
 
     break;
   }
   default:
   {
     exports.isKnownWindow = function(window) false;
     break;
   }
 }