Issue 1724 - Fixed memory leak in messaging code, when no response is sent, on Safari

Issue 1724 - Fixed memory leak in messaging code, when no response is sent, on Safari

[Wladimir Palant, 2015-01-08 14:28:18]

There we go, Safari and Firefox implementations diverge already :)

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
File safari/ext/common.js (right):

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
safari/ext/common.js:52: // is sent later asynchronously. Otherwise if no
response were sent yet,
Nit: "was sent"

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
safari/ext/common.js:54: // the other end, can remove the callback and doesn't
leak memory.
Nit: no comma here.

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
safari/ext/common.js:55: if (!sent && results.indexOf(true) != -1)
Shouldn't this be .indexOf(true) == -1?

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
safari/ext/common.js:56: this._sendResponse(request, null);
I'd rather go with undefined here, that's what the Firefox implementation does.
There might be valid reasons to send null as a response.

I assume that undefined cannot be sent as a property value. That's why the
Firefox implementation of _sendResponse() explicitly sets |responseSent: typeof
response != "undefined"| property in the response.

[Sebastian Noack, 2015-01-08 14:36:13]

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
File safari/ext/common.js (right):

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
safari/ext/common.js:52: // is sent later asynchronously. Otherwise if no
response were sent yet,
On 2015/01/08 14:28:19, Wladimir Palant wrote:
> Nit: "was sent"

Done.

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
safari/ext/common.js:54: // the other end, can remove the callback and doesn't
leak memory.
On 2015/01/08 14:28:19, Wladimir Palant wrote:
> Nit: no comma here.

Done.

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
safari/ext/common.js:55: if (!sent && results.indexOf(true) != -1)
On 2015/01/08 14:28:19, Wladimir Palant wrote:
> Shouldn't this be .indexOf(true) == -1?

Ouch, you are right.

http://codereview.adblockplus.org/6751260996796416/diff/5629499534213120/safa...
safari/ext/common.js:56: this._sendResponse(request, null);
On 2015/01/08 14:28:19, Wladimir Palant wrote:
> I'd rather go with undefined here, that's what the Firefox implementation
does.
> There might be valid reasons to send null as a response.
> 
> I assume that undefined cannot be sent as a property value. That's why the
> Firefox implementation of _sendResponse() explicitly sets |responseSent:
typeof
> response != "undefined"| property in the response.

Done.

[Wladimir Palant, 2015-01-08 14:45:25]

LGTM

Now all we need is change the Firefox implementation to match...

[Wladimir Palant, 2015-01-08 15:58:56]

LGTM again.