Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bug 749638 - Demistify 'self' #419

Merged
merged 2 commits into from

2 participants

@Gozala
Owner

Make self independent of requirer module package.

packages/api-utils/lib/self.js
((14 lines not shown))
+
+// Utility function that synchronously reads local resource from the given
+// `uri` and returns content string.
+function readURI(uri) {
+ let request = XMLHttpRequest();
+ request.open('GET', uri, false);
+ request.overrideMimeType('text/plain');
+ request.send();
+ return request.responseText;
+}
+
+function uri(path) {
+ return addonDataURI + (path || '');
+}
+
+const addonDataURI = prefixURI + name + '/data/';
@ochameau Owner

You should move this on top, before it is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/self.js
@@ -0,0 +1,44 @@
+/* vim:st=2:sts=2:sw=2:
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const { CC } = require('chrome');
+const { jetpackID, name, manifest, metadata, prefixURI, version,
+ loadReason } = require('@packaging');
@ochameau Owner

version doesn't seem to be used.

@ochameau Owner

Nor manifest?

@Gozala Owner
Gozala added a note

Good point, although version is supposed to be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau ochameau commented on the diff
python-lib/cuddlefish/manifest.py
@@ -252,11 +247,12 @@ def get_used_files(self):
# returns all .js files that we reference, plus data/ files. You will
# need to add the loader, off-manifest files that it needs, and
# generated metadata.
+ for datamap in self.datamaps.values():
@ochameau Owner

Are we using datamaps for anything else than DataMap(self.target_cfg) ?
If not, we can do : for datamap in DataMap(self.target_cfg): instead.
(And drop self.datamaps[self.target_cfg.name] = DataMap(self.target_cfg))

@Gozala Owner
Gozala added a note

I don't really know to be honest, I just wanted to make a minimal changes, there are many places where that's being accessed, but don't know how that has being used. If you don't feel very strongly about I'd rather avoid further changes as it's going to be thrown away soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau
Owner

The patch is ok.
I'm more concerned about this first breaking change around packages.
I'm wondering if we can print some warning messages when self is required by a package that isn't the main addon package.
At least, we should open bug to document this breaking change in release note.
But it may be necessary to add some note in documentation too?
And it would be really cool to add some warning message in cfx if we can!
(it seems to be possible in manifest.py, where you removed elif reqname == "self")

@Gozala
Owner

It looks like from all 41 libraries on add-on builder site there is none that depends on self which is probably a good indication that we could get away with such a breaking change!

@Gozala Gozala merged commit 5d578ee into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 27, 2012
  1. @Gozala

    Demistify 'self'

    Gozala authored
  2. @Gozala

    Address review comments.

    Gozala authored
This page is out of date. Refresh to see the latest.
View
18 packages/api-utils/lib/cuddlefish.js
@@ -287,16 +287,6 @@ const Require = iced(function Require(loader, requirer) {
freeze(module);
}
- // "magic" modules which have contents that depend upon who imports them
- // (like "self") are expressed in the Loader's pre-populated 'modules'
- // table as callable functions, which are given the reference to this
- // Loader and a copy of the importer's URI
- //
- // TODO: Find a better way to implement `self`.
- // Maybe something like require('self!path/to/data')
- if (typeof(module) === 'function')
- module = module(loader, requirer);
-
return module.exports;
}, { main: loader.main })); // `require.main` is main `module`.
});
@@ -393,13 +383,7 @@ const Loader = iced(function Loader(options) {
// TODO: Add deprecation warnings here!
return loader.modules['@chrome'].exports;
}
- }),
- // TODO: Deprecate this `self` and switch to non-magic self.
- 'self': function self(loader, requirer) {
- let loaderURI = loader.modules['@packaging'].exports.loader;
- let require = Require(loader, { uri: loaderURI });
- return require('api-utils/self!').create(requirer);
- }
+ })
});
return loader;
View
51 packages/api-utils/lib/self!.js
@@ -1,51 +0,0 @@
-/* vim:st=2:sts=2:sw=2:
- * This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-"use strict";
-
-const { CC } = require('chrome');
-const { jetpackID, name, manifest, metadata, prefixURI,
- loadReason } = require('@packaging');
-
-const XMLHttpRequest = CC('@mozilla.org/xmlextras/xmlhttprequest;1',
- 'nsIXMLHttpRequest');
-
-// Utility function that synchronously reads local resource from the given
-// `uri` and returns content string.
-function readURI(uri) {
- let request = XMLHttpRequest();
- request.open('GET', uri, false);
- request.overrideMimeType('text/plain');
- request.send();
- return request.responseText;
-}
-
-// Some XPCOM APIs require valid URIs as an argument for certain operations (see
-// `nsILoginManager` for example). This property represents add-on associated
-// unique URI string that can be used for that.
-const uri = 'addon:' + jetpackID
-
-function url(root, path) root + (path || "")
-function read(root, path) readURI(url(root, path))
-
-exports.create = function create(module) {
- let path = module.uri.split(prefixURI).pop();
- let moduleData = manifest[path] && manifest[path].requirements['self'];
- let root = prefixURI + moduleData.dataURIPrefix;
- return Object.freeze({
- id: 'self',
- exports: Object.freeze({
- id: jetpackID,
- uri: uri,
- name: name,
- loadReason: loadReason,
- version: metadata[name].version,
- data: {
- url: url.bind(null, root),
- load: read.bind(null, root)
- }
- })
- });
-};
View
44 packages/api-utils/lib/self.js
@@ -0,0 +1,44 @@
+/* vim:st=2:sts=2:sw=2:
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const { CC } = require('chrome');
+const { jetpackID, name, prefixURI, version, loadReason } = require('@packaging');
+
+const XMLHttpRequest = CC('@mozilla.org/xmlextras/xmlhttprequest;1',
+ 'nsIXMLHttpRequest');
+
+const addonDataURI = prefixURI + name + '/data/';
+
+// Utility function that synchronously reads local resource from the given
+// `uri` and returns content string.
+function readURI(uri) {
+ let request = XMLHttpRequest();
+ request.open('GET', uri, false);
+ request.overrideMimeType('text/plain');
+ request.send();
+ return request.responseText;
+}
+
+function uri(path) {
+ return addonDataURI + (path || '');
+}
+
+
+// Some XPCOM APIs require valid URIs as an argument for certain operations
+// (see `nsILoginManager` for example). This property represents add-on
+// associated unique URI string that can be used for that.
+exports.uri = 'addon:' + jetpackID;
+exports.id = jetpackID;
+exports.name = name;
+exports.loadReason = loadReason;
+exports.version = version;
+exports.data = Object.freeze({
+ url: uri,
+ load: function read(path) {
+ return readURI(uri(path));
+ }
+});
View
1  python-lib/cuddlefish/app-extension/bootstrap.js
@@ -91,6 +91,7 @@ function startup(data, reasonCode) {
options.main = { id: options.main, uri: prefixURI + options.mainPath };
options.id = options.jetpackID;
options.loaderURI = loaderURI;
+ options.version = options.metadata[options.name].version
// Adding `uriPrefix` for backwards compatibility.
options.uriPrefix = prefixURI;
View
26 python-lib/cuddlefish/manifest.py
@@ -77,13 +77,6 @@ def get_entry_for_manifest(self):
# allowed to require() it
entry["requirements"][req] = self.requirements[req]
assert isinstance(entry["requirements"][req], dict)
- if self.datamap:
- entry["requirements"]["self"] = {
- "path": "self",
- "mapSHA256": self.datamap.data_manifest_hash,
- "mapName": self.packageName+"-data",
- "dataURIPrefix": "%s/data/" % (self.packageName),
- }
return entry
def add_js(self, js_filename):
@@ -187,8 +180,10 @@ def build(self, scan_tests, test_filter_re):
# process the top module, which recurses to process everything it
# reaches
if "main" in self.target_cfg:
- top_me = self.process_module(self.find_top(self.target_cfg))
+ top_mi = self.find_top(self.target_cfg)
+ top_me = self.process_module(top_mi)
self.top_path = top_me.get_path()
+ self.datamaps[self.target_cfg.name] = DataMap(self.target_cfg)
if scan_tests:
mi = self._find_module_in_package("test-harness", "lib", "run-tests", [])
self.process_module(mi)
@@ -252,11 +247,12 @@ def get_used_files(self):
# returns all .js files that we reference, plus data/ files. You will
# need to add the loader, off-manifest files that it needs, and
# generated metadata.
+ for datamap in self.datamaps.values():
@ochameau Owner

Are we using datamaps for anything else than DataMap(self.target_cfg) ?
If not, we can do : for datamap in DataMap(self.target_cfg): instead.
(And drop self.datamaps[self.target_cfg.name] = DataMap(self.target_cfg))

@Gozala Owner
Gozala added a note

I don't really know to be honest, I just wanted to make a minimal changes, there are many places where that's being accessed, but don't know how that has being used. If you don't feel very strongly about I'd rather avoid further changes as it's going to be thrown away soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ for (zipname, absname) in datamap.files_to_copy:
+ yield absname
+
for me in self.get_module_entries():
yield me.js_filename
- if me.datamap:
- for (zipname, absname) in me.datamap.files_to_copy:
- yield absname
def get_all_test_modules(self):
return self.test_modules
@@ -376,14 +372,6 @@ def process_module(self, mi):
for reqname in sorted(requires.keys()):
if reqname in ("chrome", "@packaging", "@loader"):
me.add_requirement(reqname, {"path": reqname})
- elif reqname == "self":
- # this might reference bundled data, so:
- # 1: hash that data, add the hash as a dependency
- # 2: arrange for the data to be copied into the XPI later
- if pkg.name not in self.datamaps:
- self.datamaps[pkg.name] = DataMap(pkg)
- dm = self.datamaps[pkg.name]
- me.add_data(dm) # 'self' is implicit
else:
# when two modules require() the same name, do they get a
# shared instance? This is a deep question. For now say yes.
View
0  ...ests/linker-files/three-deps/three-a/data/msg.txt → .../cuddlefish/tests/linker-files/three/data/msg.txt
File renamed without changes
View
0  ...r-files/three-deps/three-a/data/subdir/submsg.txt → ...h/tests/linker-files/three/data/subdir/submsg.txt
File renamed without changes
View
22 python-lib/cuddlefish/tests/test_xpi.py
@@ -164,6 +164,7 @@ def test_contents(self):
packagepath=package_path)
deps = packaging.get_deps_for_targets(pkg_cfg,
[target_cfg.name, "addon-kit"])
+ api_utils_dir = pkg_cfg.packages["api-utils"].lib[0]
m = manifest.build_manifest(target_cfg, pkg_cfg, deps, scan_tests=False)
used_files = list(m.get_used_files())
here = up(os.path.abspath(__file__))
@@ -174,15 +175,18 @@ def absify(*parts):
[("three", "lib", "main.js"),
("three-deps", "three-a", "lib", "main.js"),
("three-deps", "three-a", "lib", "subdir", "subfile.js"),
- ("three-deps", "three-a", "data", "msg.txt"),
- ("three-deps", "three-a", "data", "subdir", "submsg.txt"),
+ ("three", "data", "msg.txt"),
+ ("three", "data", "subdir", "submsg.txt"),
("three-deps", "three-b", "lib", "main.js"),
("three-deps", "three-c", "lib", "main.js"),
- ("three-deps", "three-c", "lib", "sub", "foo.js"),
+ ("three-deps", "three-c", "lib", "sub", "foo.js")
]]
+ expected.append(os.path.join(api_utils_dir, "self.js"))
+
missing = set(expected) - set(used_files)
extra = set(used_files) - set(expected)
- self.failUnlessEqual((list(missing), list(extra)), ([], []))
+ self.failUnlessEqual(list(missing), [])
+ self.failUnlessEqual(list(extra), [])
used_deps = m.get_used_packages()
build = packaging.generate_build_for_target(pkg_cfg, target_cfg.name,
@@ -210,14 +214,15 @@ def absify(*parts):
"resources/api-utils/",
"resources/api-utils/data/",
"resources/api-utils/lib/",
+ "resources/api-utils/lib/self.js",
"resources/three/",
"resources/three/lib/",
"resources/three/lib/main.js",
+ "resources/three/data/",
+ "resources/three/data/msg.txt",
+ "resources/three/data/subdir/",
+ "resources/three/data/subdir/submsg.txt",
"resources/three-a/",
- "resources/three-a/data/",
- "resources/three-a/data/msg.txt",
- "resources/three-a/data/subdir/",
- "resources/three-a/data/subdir/submsg.txt",
"resources/three-a/lib/",
"resources/three-a/lib/main.js",
"resources/three-a/lib/subdir/",
@@ -267,6 +272,7 @@ def test_scantests(self):
package_path = [self.get_linker_files_dir("three-deps")]
pkg_cfg = packaging.build_config(self.root, target_cfg,
packagepath=package_path)
+
deps = packaging.get_deps_for_targets(pkg_cfg,
[target_cfg.name, "addon-kit"])
m = manifest.build_manifest(target_cfg, pkg_cfg, deps, scan_tests=True)
Something went wrong with that request. Please try again.