Fixed properties defined on prototype in Safari background page proxy

Fixed properties defined on prototype in Safari background page proxy

[Sebastian Noack, 2013-11-17 17:34:14]

In order to access the background page from within the options and first run
page, I had to implement an object proxy on top of Safari's synchronous
messaging API.

Whenever a property of an object from the background page is read, the
background page is asked to read that property from the object that defined the
property.

However this doesn't work when the property is defined in the object's prototype
and is using a setter which relies on "this". So instead, the background page
must be asked to read the property from the object, that was used to access the
property.

This bug prevented subscription details on the options page from updating
correctly.

[Felix Dahlke, 2013-11-18 16:39:07]

LGTM

[Wladimir Palant, 2013-11-21 08:15:58]

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:31: evt.initEvent("beforeload");
Can we reuse the same event instead of creating a new one every time?

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:46: var objectId = this.objects.indexOf(obj);
this.getObjectId() should be used here.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:52: var callbackId = this.callbacks.indexOf(obj);
As with objects, it is better to put the callback ID into an expando property of
the callback rather than going through the array every time.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:61: if (event.message.callbackId == callbackId)
You should use && here and put brackets around the body of the block.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:66: }.bind(this));
Why add a new listener for each single callback? There should be only one
listener:

function(event)
{
  if (event.name == "proxyCallback")
  {
    var callback = this.callbacks[event.message.callbackId];
    callback.apply(
      this.getObject(event.message.contextId),
      this.deserializeSequence(event.message.args)
    );
  }
}

This listener can be added when the first callback is created.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:148: return this.objects.indexOf(obj);
Why iterate through the entire array when we can cache the object ID as an
object property (e.g. __objectID, not enumerable) when we are creating the
object?

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:204: getObject: function(objectId) {
Style nit: this bracket should be on the next line.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:211: if (obj)
Nit: I generally dislike blindly requesting properties that might not exist.
This should really be |if (objectId in this.objects)|.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:229: ignored.splice(ignored.indexOf("constructor"), 1);
I think there is something conceptually wrong here. Why is the sender sending
all properties rather than own properties only? ignored shouldn't normally be
necessary.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:243: obj.__proto__ = null;
Note that __proto__ is deprecated. It would be better to use Object.create()
here to create the object with a specific prototype. Object.getPrototypeOf() can
be used on the sender's side.

We use __proto__ in our Firefox code because that code is older than the two
functions mentioned above.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:250: ));
Style nit: please always put brackets around multi-line blocks, even if
syntactically they aren't required.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:253: obj.prototype = this.getProperty(objectId, "prototype");
I'm not sure whether this special case is the best solution. The loop above
could instead check whether the property already exists and is non-configurable
- the property value should be set then rather than defining the property.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:266: switch(event.target.nodeName)
Please use localName and check for lower-case tag names below - then this will
work properly with XHTML documents as well.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:283: if (/(^|\s)stylesheet($|\s)/i.test(event.target.rel))
/\bstylesheet\b/i should do here.

[Sebastian Noack, 2013-11-21 21:17:08]

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:52: var callbackId = this.callbacks.indexOf(obj);
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> As with objects, it is better to put the callback ID into an expando property
of
> the callback rather than going through the array every time.

But this would make the ID visible externally and might theoretically clash with
other properties. I would prefer to keep it hidden from the calling code.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:211: if (obj)
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> Nit: I generally dislike blindly requesting properties that might not exist.
> This should really be |if (objectId in this.objects)|.

First using the in opreator and then getting the actual property from the
object, duplicates the logic, and since the lookup is done twice it adds an
unneeded overhead.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:229: ignored.splice(ignored.indexOf("constructor"), 1);
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> I think there is something conceptually wrong here. Why is the sender sending
> all properties rather than own properties only? ignored shouldn't normally be
> necessary.

Only own properties are sent. This code only handles the special case of
Object.prototype and Function.prototype. The proxy objects for Object.prototype
and Function.prototype are going to inherit from the builtin Object.prototype
and Function.prototype and we must not override functions like toString(),
__lookupGetter__(), etc with proxy functions. However we want to proxy the
constructor (in order that "bg.someObj instanceof bg.Object == true"), and we
also want to proxy properties that were monkey-patched to Object.prototype or
Function.prototype in the bg page, but not in our current execution environment.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:243: obj.__proto__ = null;
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> Note that __proto__ is deprecated. It would be better to use Object.create()
> here to create the object with a specific prototype. Object.getPrototypeOf()
can
> be used on the sender's side.
> 
> We use __proto__ in our Firefox code because that code is older than the two
> functions mentioned above.

I know and I have actually used Object.create() in the initial version. However
__proto__ is the only way to prepare a function with a custom prototype and its
the only way to change the prototype of an existing object.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:283: if (/(^|\s)stylesheet($|\s)/i.test(event.target.rel))
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> /\bstylesheet\b/i should do here.

There is no \b in regular expressions in JS (at least in Webkit).

[Sebastian Noack, 2013-11-27 17:43:16]

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:283: if (/(^|\s)stylesheet($|\s)/i.test(event.target.rel))
On 2013/11/21 21:17:08, sebastian wrote:
> On 2013/11/21 08:15:58, Wladimir Palant wrote:
> > /\bstylesheet\b/i should do here.
> 
> There is no \b in regular expressions in JS (at least in Webkit).

I'm wrong. Nevermind.

[Sebastian Noack, 2013-11-29 10:32:20]

I addressed all comments, except those I have replied to. However the initial
patch set has already been merged, so the new patch set only contains the new
changes.

[Wladimir Palant, 2014-01-15 16:21:01]

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:52: var callbackId = this.callbacks.indexOf(obj);
On 2013/11/21 21:17:08, sebastian wrote:
> But this would make the ID visible externally and might theoretically clash
with
> other properties. I would prefer to keep it hidden from the calling code.

It's not like we are messing with some website here - this is our own code, our
own properties. If you are afraid of naming conflicts you can use
$safariCompat$callbackId as property name. However, looping through arrays to
find object/callback IDs isn't really an option as it will degrade performance
severely. This will become especially visible if we expose individual filters in
the preferences UI - we will be dealing with thousands of objects then.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:211: if (obj)
On 2013/11/21 21:17:08, Sebastian Noack wrote:
> First using the in opreator and then getting the actual property from the
> object, duplicates the logic, and since the lookup is done twice it adds an
> unneeded overhead.

Very Python-like logic. Except that property lookups are generally cached so the
performance difference should be negligible even if this were a hot spot.

if (foo.bar) is not quite the same as if ("bar" in foo) as you certainly know. I
prefer to show clearly that we are checking existence here, not the value - and
this doesn't really count as "duplicating logic". But I will accept this as is
if you insist.

http://codereview.adblockplus.org/5670052883857408/diff/5639274879778816/safa...
File safari/ext/content.js (right):

http://codereview.adblockplus.org/5670052883857408/diff/5639274879778816/safa...
safari/ext/content.js:60: if (callbackId == 0)
This is non-obvious and needs a short explanation in a comment, something like
"Only add a listener for the first callback, after that we can simply assume
that it is already there."

http://codereview.adblockplus.org/5670052883857408/diff/5639274879778816/safa...
safari/ext/content.js:236: ignored.splice(ignored.indexOf("constructor"), 1);
The assumption here is that ignored.indexOf() will never return -1. I'd rather
drop that assumption and check explicitly.

[Sebastian Noack, 2014-02-26 11:27:19]

Most of the comments were already addressed by other patches. However here is a
new patch set, that addresses the remaining comments.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:31: evt.initEvent("beforeload");
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> Can we reuse the same event instead of creating a new one every time?

This was addressed by http://codereview.adblockplus.org/5464830253203456/

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:46: var objectId = this.objects.indexOf(obj);
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> this.getObjectId() should be used here.

getObjectId() is gone now.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:52: var callbackId = this.callbacks.indexOf(obj);
On 2014/01/15 16:21:01, Wladimir Palant wrote:
> On 2013/11/21 21:17:08, sebastian wrote:
> > But this would make the ID visible externally and might theoretically clash
> with
> > other properties. I would prefer to keep it hidden from the calling code.
> 
> It's not like we are messing with some website here - this is our own code,
our
> own properties. If you are afraid of naming conflicts you can use
> $safariCompat$callbackId as property name. However, looping through arrays to
> find object/callback IDs isn't really an option as it will degrade performance
> severely. This will become especially visible if we expose individual filters
in
> the preferences UI - we will be dealing with thousands of objects then.

Done.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:61: if (event.message.callbackId == callbackId)
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> You should use && here and put brackets around the body of the block.

This was addressed by http://codereview.adblockplus.org/5464830253203456/

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:66: }.bind(this));
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> Why add a new listener for each single callback? There should be only one
> listener:
> 
> function(event)
> {
>   if (event.name == "proxyCallback")
>   {
>     var callback = this.callbacks[event.message.callbackId];
>     callback.apply(
>       this.getObject(event.message.contextId),
>       this.deserializeSequence(event.message.args)
>     );
>   }
> }
> 
> This listener can be added when the first callback is created.

This was addressed by http://codereview.adblockplus.org/5464830253203456/

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:148: return this.objects.indexOf(obj);
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> Why iterate through the entire array when we can cache the object ID as an
> object property (e.g. __objectID, not enumerable) when we are creating the
> object?

Done.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:204: getObject: function(objectId) {
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> Style nit: this bracket should be on the next line.

This was addressed by http://codereview.adblockplus.org/5464830253203456/

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:211: if (obj)
On 2014/01/15 16:21:01, Wladimir Palant wrote:
> On 2013/11/21 21:17:08, Sebastian Noack wrote:
> > First using the in opreator and then getting the actual property from the
> > object, duplicates the logic, and since the lookup is done twice it adds an
> > unneeded overhead.
> 
> Very Python-like logic. Except that property lookups are generally cached so
the
> performance difference should be negligible even if this were a hot spot.

I've benchmarked this, and on Firefox using the in operator before the actual
lookup seems to be even faster. But on Chrome and Safari (where this code is
used) it is faster if you skip the in operator check and do only one lookup.
http://jsperf.com/in-operator-performance

> if (foo.bar) is not quite the same as if ("bar" in foo) as you certainly know.

Yes, I know but since we have only objects here, which always evaluate to true,
it doesn't make a difference.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:250: ));
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> Style nit: please always put brackets around multi-line blocks, even if
> syntactically they aren't required.

Done.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:253: obj.prototype = this.getProperty(objectId, "prototype");
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> I'm not sure whether this special case is the best solution. The loop above
> could instead check whether the property already exists and is
non-configurable
> - the property value should be set then rather than defining the property.

Done.

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:266: switch(event.target.nodeName)
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> Please use localName and check for lower-case tag names below - then this will
> work properly with XHTML documents as well.

This was addressed by http://codereview.adblockplus.org/4526994515558400/

http://codereview.adblockplus.org/5670052883857408/diff/5629499534213120/safa...
safari/content.js:283: if (/(^|\s)stylesheet($|\s)/i.test(event.target.rel))
On 2013/11/21 08:15:58, Wladimir Palant wrote:
> /\bstylesheet\b/i should do here.

This was addressed by http://codereview.adblockplus.org/4526994515558400/

[Wladimir Palant, 2014-03-21 14:27:28]

LGTM