To avoid landing bug 883050 in a single megapatch, let's do this on a per-module basis. Note that all the patches will need to be flattened, eventually.

The patch attached in comment 1 cleans up osfile_shared_allthreads.jsm as follows: - the new version does not require clients to do dirty tricks with a shared |OS| object; - everything related to preferences has been removed (and moved to another module).

Same code, minimal cleanup.

- Minor changes to how modules import osfile_shared_allthreads. - Preference management has moved to osfile_async_front.jsm.

I needed to patch the module loader for two reasons: - a few typos/bugs (i.e. using |modules| instead of |module| can introduce surprising semantics); - enforcing that all modules end up with ".js" is not a good way to do refactoring on code in which all files end up with ".jsm".

The test suite explicitly used OS.Shared.{DEBUG, TEST}, which won't be possible anymore. Replacing with the appropriate preferences.

Same patch, but without reordering the contents.

As discussed over IRC, I attach a version of patch 1. ignoring whitespace.

Comment on attachment 769183 [details] [diff] [review] 0. Patching the module loader to be able to do the refactoring Review of attachment 769183 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/workerloader/require.js @@ +146,5 @@ > return function require(path) { > if (typeof path != "string" || path.indexOf("://") == -1) { > throw new TypeError("The argument to require() must be a string uri, got " + path); > } > + // For the moment, we make no difference between uri and path In fact there are lot's of SDK modules out there that I'd hope would've worked out of the box, but if that normalization was kept. @@ +194,5 @@ > let blob = new Blob([(new TextEncoder()).encode(source)]); > objectURL = URL.createObjectURL(blob); > paths.set(objectURL, path); > importScripts(objectURL); > + require._tmpModules[name].call(module, exports, require, module); In node `this` points to a `global` scope, and in SDK to a module scope, I wonder why this needs to be something else ? I understand you can't make a module scope, but in that case I'd stick to global scope to follow a node.

Is that better? I personally don't like the scope to be the global scope, but I guess we can't really do better.

Comment on attachment 769196 [details] [diff] [review] 1. Rewriting osfile_shared_allthreads.jsm for require(), v3 Review of attachment 769196 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/osfile_shared_allthreads.jsm @@ +578,5 @@ > + * A C wide char (two bytes) > + */ > +Type.jschar = > + new Type("jschar", > + ctypes.jschar); Formatting nit: might as well put this all on one line. Same for IntTypes below.

Irakli, could you please review the last patch? It's blocking me from landing this bug.

Comment on attachment 769855 [details] [diff] [review] 0. Patching the module loader to be able to do the refactoring, v2 Review of attachment 769855 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

Try: https://tbpl.mozilla.org/?tree=Try&rev=dc96a8f25d90

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc02d2288d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/552aab6137ef

https://hg.mozilla.org/mozilla-central/rev/ccc02d2288d5 https://hg.mozilla.org/mozilla-central/rev/552aab6137ef