Issue 2816 - Restore Preview functionality

Issue 2816 - Restore Preview functionality

[Wladimir Palant, 2015-07-30 13:13:22]

https://codereview.adblockplus.org/29323107/diff/29323108/chrome/content/fram...
File chrome/content/frameScript.js (right):

https://codereview.adblockplus.org/29323107/diff/29323108/chrome/content/fram...
chrome/content/frameScript.js:31:
message.objects.callback(JSON.stringify(nodeInfo));
Strictly speaking I am fixing an issue of the previous change here - it was
passing an object to the callback here. The callback is a CPOW
(https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Proces...)
meaning that when we call it complex parameters won't be passed as structured
clones - they will rather become CPOWs themselves. This caused lots of
unnecessary cross-process traffic and console warnings, something that we can
completely avoid by passing a string.

[Thomas Greiner, 2015-07-31 12:48:03]

https://codereview.adblockplus.org/29323107/diff/29323108/chrome/content/acto...
File chrome/content/actor.jsm (right):

https://codereview.adblockplus.org/29323107/diff/29323108/chrome/content/acto...
chrome/content/actor.jsm:150: // ignore this case, we don't need to do anything.
If the context is no longer valid anyway, what about calling "forgetNode" to get
rid of the node here?

[Wladimir Palant, 2015-07-31 14:40:04]

https://codereview.adblockplus.org/29323107/diff/29323108/chrome/content/acto...
File chrome/content/actor.jsm (right):

https://codereview.adblockplus.org/29323107/diff/29323108/chrome/content/acto...
chrome/content/actor.jsm:150: // ignore this case, we don't need to do anything.
On 2015/07/31 12:48:03, Thomas Greiner wrote:
> If the context is no longer valid anyway, what about calling "forgetNode" to
get
> rid of the node here?

Done.

https://codereview.adblockplus.org/29323107/diff/29323149/chrome/content/acto...
File chrome/content/actor.jsm (right):

https://codereview.adblockplus.org/29323107/diff/29323149/chrome/content/acto...
chrome/content/actor.jsm:40: DebuggerServer.removeTabActor(actor);
This will throw in the content process, I suspect that our way to register the
actor is incorrect. I will investigate this further but in the meantime catching
this error allows the execution to continue - meaning that we won't leave
message handlers attached on shutdown.

[Wladimir Palant, 2015-07-31 15:09:36]

https://codereview.adblockplus.org/29323107/diff/29323149/chrome/content/acto...
File chrome/content/actor.jsm (right):

https://codereview.adblockplus.org/29323107/diff/29323149/chrome/content/acto...
chrome/content/actor.jsm:40: DebuggerServer.removeTabActor(actor);
On 2015/07/31 14:40:04, Wladimir Palant wrote:
> This will throw in the content process, I suspect that our way to register the
> actor is incorrect. I will investigate this further but in the meantime
catching
> this error allows the execution to continue - meaning that we won't leave
> message handlers attached on shutdown.

My investigation shows that we are doing everything correctly. This seems to be
a bug in the developer tools, they aren't really testing cleanup it seems (it's
only required for restartless extensions, not built-in actors). Filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1189780 on this, I added a
corresponding comment to the code here.

[Thomas Greiner, 2015-07-31 15:15:49]

LGTM