Bug 820953 - Expose DOM window to the add-on

[Gozala]

No description provided.

[erikvold]

@Gozala I might need to change the hidden-frame module for the PWPB changes, and I doubt it, but the changes might be uplifted, so keeping those changes independent from the changes here seems like a good idea to me. So I wonder if we could revert the hidden-frame.js module's changes for now and create a separate request with those changes.

[erikvold]

This seems to load the addon/window at load time regardless if it is used or not, am I mistaken?

[Mossop]

I am concerned about this too. Do we have any information on timing and memory penalties that this incurs?

[Gozala]

We do use windows regardless of this change, in hidden-window any API doing some ui depends on implicitly. The problem is that all of that APIs need to wait on their own and deal with synchronicity & only on some platforms.

I don't think it's worth it, I'd rather delay once and make all APIs be able to do work synchronously that curry the burden of platfrom specefic inconsistencies into all modules. Not to mention that this that load is still delayed by async read of l10n data etc..

Also note that this is temporary workaround that will go away once platform fix for Bug 565388 will be implemented.

[Mossop]

Can we open the window before we wait for APP_STARTUP so add-ons start as fast as possible on startup?

[Gozala]

Can we open the window before we wait for APP_STARTUP so add-ons start as fast as possible on startup?

We probably could but intention of delaying was to avoid slowing down a startup. We could also start loading l10n stuff before startup, but we decided not to for the same reasons. We can revisit these decisions, but not sure we need to block this patch for that. We can change time we start loading when we want to.

[erikvold]

Needs tests too.

[erikvold]

I think that let options = JSON.parse(JSON.stringify(packaging)); was testing that packaging was a valid json object. I could be wrong, but this change doesn't seem necessary.

[Gozala]

No it was done just to copy options from the loader. This no longer works because with this addition:
https://github.com/mozilla/addon-sdk/pull/698/files#L3R14

it no longer JSON since window can not be serialized.

[Mossop]

This doesn't totally match what I was expecting (not necessarily a bad thing). I was anticipating that main.js would be run in a window context so it could just call anything it wanted as if it were a webpage script. I guess this way is better in that any module can use the window, but does that also introduce a risk that modules will use the window for communication or other strange things?

[jeffgca]

" I was anticipating that main.js would be run in a window context so it could just call anything it wanted as if it were a webpage script."

@Mossop I would agree, it's surprising to me that this was implemented in this way, this is not the feature as I understood it. @Gozala is there a specific reason why DOM apis are not imported into the global scope of main.js?

[Gozala]

@Mossop @canuckistani @erikvold This is not meant to be used by add-ons directly, also as comments in the module suggest this is just a temporary solution substituting platform feature that bz promised to do in Bug 565388.

This is same (but less hacky) solution enabling usage of WebSocket that is already had being used by 1Password and probably others.

I had long discussions about implementations details that can be found in the bugs linked from 820953, unfortunately even more discussions with bz happened on IRC which probably can be seen in archives. After explaining our objectives and discussing few solutions we've settled on this implementation as best possible way to go about it, until Bug 565388 is shipped. Once it is shipped we'll just change add-on/window and remove delay from runner, everything else we'll do will continue to work.

Note: Exposure of this (or maybe other) window context into main.js is a separate matter (task / discussion subject), which requires this as a base. Also note if we wanna expose window context directly into main.js we will have to delay loading of the add-on until that window is loaded or very likely add-on will misbehave (working with non-yet loaded window is error prone).

So I think we should land this and than continue discussion on how exactly DOM is exposed to the add-on.

[Gozala]

@erikvold @Mossop @canuckistani Can we please move forward with this it blocks bunch of other changes in the queue.

[erikvold]

Some screwed up stuff happened during the reverts in this pull request, I think it's needs another look @Gozala See bug 847771

[erikvold]

@Gozala @Mossop I don't understand why this landed without tests..