708855 - refactor sandbox functions into a module

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P4)

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/295

Pointer to Github pull-request

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 580236 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/295

We are creating sandboxes only from worker.js and cuddlefish.js. 
As we are not going to make a dependency for cuddlefish, 
I don't see the point of such genericity. 
Do you have some upcoming changes in branches or future landing that may use Sandboxes ?

I know e10s project is on hold, but I'd like to avoid dependencies around worker.js and content-proxy.js. It doesn't make much sense to instanciate a module loader in all remote tabs so that we will have to load a part of worker.js with all dependencies pulled into it. And even if we finally decide to instanciate a module loader, we should limit number of files to load for performances reasons.

(httpd.js creates some sanboxes but as we pulled this code, it doesn't makes sense to modify it)

I'm r- this, but I obviously lack your picture for this patch, as this bug, nor the pull request contains any description. So feel free to give me more information and r? again!

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> Comment on attachment 580236 [details]
> Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/295
> 
> We are creating sandboxes only from worker.js and cuddlefish.js. 
> As we are not going to make a dependency for cuddlefish, 
> I don't see the point of such genericity. 
> Do you have some upcoming changes in branches or future landing that may use
> Sandboxes ?
> 
> I know e10s project is on hold, but I'd like to avoid dependencies around
> worker.js and content-proxy.js. It doesn't make much sense to instanciate a
> module loader in all remote tabs so that we will have to load a part of
> worker.js with all dependencies pulled into it. And even if we finally
> decide to instanciate a module loader, we should limit number of files to
> load for performances reasons.
> 
> (httpd.js creates some sanboxes but as we pulled this code, it doesn't makes
> sense to modify it)
> 
> I'm r- this, but I obviously lack your picture for this patch, as this bug,
> nor the pull request contains any description. So feel free to give me more
> information and r? again!

In fact I did not intend to add generic sandbox API. I end up doing this as I was trying to refactor / cleanup Worker API to use namespaces API as we plan across all of the SDK code (See https://github.com/Gozala/addon-sdk/tree/cleanup/worker-704357 for details). I don't know about you, but I find it extremely hard to follow functions that are 160 lines of code (https://github.com/Gozala/addon-sdk/tree/cleanup/worker-704357), usually smaller composable components help looking / talking at the level of your problems. That being said this is not the only reason for this change, I have being talking to ZER0 about the changes he needs to move forward with mobile, and if I understood correctly current plan is to implement message manager like abstraction using sandboxes, so this change will help. (Please correct me ZER0 if I'm wrong).

ochameau please let me know if this is a reasonable explanation to you, if so I'll make another review request.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Oh and I just recalled that @marijnjh explains my point regarding composable parts, very well in his book "Eloquent JavaScrip" see thttp://eloquentjavascript.net/chapter6.html section before ".....".

Comment 5

[Alexandre Poirot [:ochameau]]

Sure, I agree with the idea. Small components is a good thing, but we should not be too extreme. And in this particular case, worker.js, dependencies can end up being an issue.
I wasn't aware of ZER0 needs. Having two codes sharing these module makes a big difference ! ZER0, how did you handled remote processes for tabs ? Do you think adding these dependencies will work for e10s ?

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

I'm using sandboxes, so if we want refactoring them in a module I'm not against it.

However, the only thing I will save from that is just the `loadFrameScript` method of the Message Manager APIs. I don't need that change to moving forward with mobile, but I guess it won't hurt as well. So, keep in mind that from this change the Mobile code won't get any benefits, but also that now we have three places where we're using sandboxes (worker, cuddlefish, and message-manager).

Alex, I do not handle the remote process for tabs at the moment. What Irakli was talking about was related to create sandbox to use them as remote frame for the message manager API, exactly as you did in your prototype that you shown to me.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 580236 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/295

Reopen based on conversation here and IRC.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 580236 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/295

One encoding issue has to be fixed. 
Some doubts around some property freeze.
After a deeper review, I finally decided to stick with my negative judgment about `sandbox` module. Otherwise, others cleanup are usefull.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

P4 to get out of the triage list.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 580236 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/295

Some nits in tests mentioned in pull request. One dead link in md file.
There is some `catch` statements on the same line than closing brace in test-sandbox.js.
Otherwise looks good, tests passed!

Comment 11

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/5329b772c43d5d43c5908c238270a7c8e80aee0a
Merge pull request #295 from Gozala/bug/sandbox-708855

fix Bug 708855 - refactor sandbox functions into it's own module r=ochameau