1114940 - rename content/worker-parent to worker, move sync worker to deprecated/

Status: RESOLVED FIXED

Description

[Tomislav Jovanovic :zombie]

the content/worker-parent.js was never meant to ship, only to enable piecemeal switch to the new (e10s) worker. 

unfortunately, i was late in switching all the uses, so now content/worker-parent.js is in the firefox 36 (aurora) source code. 

this bug is to do the renaming, and ask for uplift.

Comment 1

[Tomislav Jovanovic :zombie]

Created attachment 8540747 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1763

hey Dave, three notes about this PR:

1) it's much easier to see what exactly is changing by looking at individual "move" and "rename" commits. i'm thinking of not squashing, and merging it like that.
https://github.com/mozilla/addon-sdk/pull/1763/commits

2) there are a few places that actually worked "as-is", without any code changes, or the need to switch the reference to the deprecated sync-worker, but i opted to still let it reference the old worker, in order to minimize risk because of the uplift.

3) it might be a good idea to roll the WorkerChild class into content/worker.js.  but since worker-child.js is loaded using another loader, avoiding loading other modules (required by worker.js, but not worker-child.js) could be better?  what do you think?

Comment 2

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 8540747 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1763

I think I'd rather Erik or Irakli make the final call here but my take is that this isn't the right thing to do. If we don't want to insta-break any extension using the old worker then we should leave it where it is and add a deprecation message whenever it is used telling them to switch to the new worker. I think it is ok for the new worker to be called worker-parent, it makes it clear that it is the parent side to worker-child.

Just adding a deprecation message should be upliftable. Changes like this that intentionally break things probably shouldn't be uplifted to what is effectively about to become beta.

Comment 3

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 8540747 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1763

I was convinced over IRC. I still think this shouldn't be uplifted to aurora/beta though.

Comment 4

[Tomislav Jovanovic :zombie]

for reference, here is the tldr of the discussion:

1) this isn't a change that intentionally, insta-breaks things. 
2) the new Worker is mostly a compatible drop-in replacement for the old one.
3) and it should probably work for 99% of addons that use it in the documented way.
4) only thing that wont is expecting effects of content script to be done before the next line executes
5) Dave's idea was to deprecate worker.js and ask all addon authors to switch to the new worker-parent.js
6) to which i disagreed, as that would require everyone to update, when in reality 99% would just work
7) and the whole situation is similar to what we did in the past with traits-worker.js.


on the other hand, i was convinced of:

> I still think this shouldn't be uplifted to aurora/beta though.

the only downside to this is that we ship worker-parent.js for just one release (fx36), but if we don't mention it anywhere, and pretend it doesn't exist, it's probably not gonna be a problem. ;)


and also, i plan to write something for the addons blog about SDK and e10s, and mention this.

Comment 5

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/759f57ff4e6442aa89861710f18f794c1b450c87
bug 1114940 - move content/worker to deprecated/sync-worker

https://github.com/mozilla/addon-sdk/commit/7a53a2d60aee37317ffc54da2f00de0988da8c2f
bug 1114940 - rename worker-parent to worker

https://github.com/mozilla/addon-sdk/commit/2eaae671b9ab0ad99a23ac6c3fad7263f9619631
Merge pull request #1763 from zombie/1114940-move-sync-worker

bug 1114940 - move sync worker to deprecated, rename worker-parent, r=@Mossop