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

safari/ext/common.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 145 matching lines @@
           if (!placeholderDetails || !placeholderDetails.content)
             continue;
           if (placeholderDetails.content.indexOf("$") != 0)
             continue;
 
           var placeholderIdx = parseInt(placeholderDetails.content.substr(1));
           if (isNaN(placeholderIdx) || placeholderIdx < 1)
             continue;
 
           var placeholderValue;
-          if (substitutions && substitutions.constructor == Array)

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

Please remind me, why can't this be simply |substitutions instanceof Array|?

Note that this implementation is different from both Chrome and Firefox. Our
Firefox implementation always expects an array for substitutions, this is what
it should be per chrome.i18n.getMessage() documentation. The Chrome
implementation on the other hand seems to do something like this:

if (typeof substitutions == "string")
  substitutions = [substitutions];

Only one replacement is being done here and other types (e.g. numbers or even
String objects) result in an empty string.

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

When I wrote the original code I followed the top answer of a popular
StackOverflow question
(https://stackoverflow.com/questions/4775722/check-if-object-is-array).

However now where I know a lot more about JavaScript that approach seems weird
to me and I don't understand how this answer got over 600 points. So I skipped
the string conversion, and directly checked for the constructor.

The only difference between checking the constructor and using the isinstance
operator, is that the isinstance operator will also return true, if the object
is an instance of a subclass of Array. But that shouldn't be an issue here.
The purpose of this code is to emulate Chrome's behavior, so this should be the
way to go. It might be that, I didn't tested chrome.i18n.getMessage() with
numbers, but were just expecting that they behave like strings, back when I
wrote the original code.

[Wladimir Palant, 2014/04/11 18:31:19]

The only case where instanceof Array is problematic is when dealing with
different JavaScript contexts - each window and frame has its own Array
constructor and an array from another window cannot be checked against your
constructor. Not really something that is a concern here however.

+          if (typeof substitutions != "string")
             placeholderValue = substitutions[placeholderIdx - 1];
           else if (placeholderIdx == 1)
             placeholderValue = substitutions;
 
           msgstr = msgstr.replace("$" + placeholder + "$", placeholderValue || "");
         }
 
         return msgstr;
       }
 
       return "";
     }
   };
 
 
   /* Utils */
 
   ext.getURL = function(path)
   {
     return safari.extension.baseURI + path;
   };
 })();