Refactored the abstraction layer to address prerendered pages on Safari caused by leaky abstraction

safari/ext/background.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
@@ skipping 31 matching lines @@
 
     this.browserAction = new BrowserAction(this);
   };
   Page.prototype = {
     get url()
     {
       return this._frames[0].url;
     },
     activate: function()
     {
-      pages[this._id]._tab.activate();

[Wladimir Palant, 2014/04/11 13:02:35]

Why not this._tab.activate()?

If it is because multiple Page instances can exist and only the one in pages has
all the info - you can make the Page constructor return existing object from
pages if one exists. This will make sure that only one object exists for any
page.

[Sebastian Noack, 2014/04/11 14:47:45]

Done. When I began to write this code there was no Page._tab, and I just forgot
to change this line, when I added Page._tab.

+      this._tab.activate();
     },
     sendMessage: function(message, responseCallback)
     {
       this._messageProxy.sendMessage(message, responseCallback, {pageId: this._id});
     }
   };
 
   var isPageActive = function(page)
   {
     return page._tab == page._tab.browserWindow.activeTab && !page._prerendered;
@@ skipping 42 matching lines @@
 
       for (var id in pages)
       {
         var page = pages[id];
         var win = page._tab.browserWindow;
 
         if ("active" in info && info.active != isPageActive(page))
           continue;
         if ("lastFocusedWindow" in info && info.lastFocusedWindow != (win == safari.application.activeBrowserWindow))
           continue;
-        if ("visibleWindow" in info && info.visibleWindow != win.visible)
-          continue;
 
         matchedPages.push(page);
       };
 
       callback(matchedPages);
     },
     onLoading: new ext._EventTarget()
   };
 
   safari.application.addEventListener("close", function(event)
@@ skipping 49 matching lines @@
     }
   };
 
   var BrowserAction = function(page)
   {
     this._page = page;
   };
   BrowserAction.prototype = {
     _set: function(name, value)
     {
-      var currentWindow = this._page._tab.browserWindow;
-      var toolbarItem = getToolbarItemForWindow(currentWindow);
+      var toolbarItem = getToolbarItemForWindow(this._page._tab.browserWindow);
       if (!toolbarItem)
         return;
 
       var property = toolbarItemProperties[name];
       if (!property)
         property = toolbarItemProperties[name] = {
           pages: new ext.PageMap(),
           global: toolbarItem[name]
         };
 
       property.pages.set(this._page, value);
 
-      if (this._page._tab == currentWindow.activeTab && !this._page._prerendered)

[Wladimir Palant, 2014/04/11 13:02:35]

This should use isPageActive instead of duplicating the logic.

[Sebastian Noack, 2014/04/11 14:47:45]

Done.

+      if (isPageActive(this._page))
         toolbarItem[name] = value;
     },
     setIcon: function(path)
     {
       this._set("image", safari.extension.baseURI + path.replace("$size", "16"));
     },
     setBadge: function(badge)
     {
       if (!badge)
         this._set("badge", 0);
@@ skipping 17 matching lines @@
     for (var id in pages)
     {
       var page = pages[id];
       if (page._tab == event.target && !page._prerendered)
       {
         activePage = page;
         break;
       }
     }
 
     updateToolbarItemForPage(activePage, event.target.browserWindow);

[Wladimir Palant, 2014/04/11 13:02:35]

Please add a check for activePage - it might potentially be null.

[Sebastian Noack, 2014/04/11 14:47:45]

In that case we have to reset the toolbar item properties to their default, as
done by updateToolbarItemForPage() if the given page is null.

   }, true);
 
 
   /* Web requests */
 
   ext.webRequest = {
     onBeforeRequest: new ext._EventTarget(true),
     handlerBehaviorChanged: function() {}
   };
 
@@ skipping 142 matching lines @@
           return;
 
         page._tab.page.dispatchMessage("proxyCallback",
         {
           pageId: pageId,
           frameId: frameId,
           callbackId: callbackId,
           contextId: proxy.registerObject(this, objects),
           args: proxy.serializeSequence(arguments, objects)
         });
       };

[Wladimir Palant, 2014/04/11 13:02:35]

Use .bind(this) instead of the proxy variable?

[Sebastian Noack, 2014/04/11 14:47:45]

I can't, since I also need the actual context the function is called in:

  contextId: proxy.registerObject(this, objects)

     },
     deserialize: function(spec, objects, pageId, memo)
     {
       switch (spec.type)
       {
         case "value":
           return spec.value;
         case "hosted":
           return objects[spec.objectId];
         case "callback":
@@ skipping 248 matching lines @@
         replacePage(page);
         break;
     }
   });
 
 
   /* Storage */
 
   ext.storage = safari.extension.settings;
 })();