This is the first step for the Source Map feature: https://wiki.mozilla.org/DevTools/Features/SourceMap

Rob (cc'ing Dave), Things to consider/questions: * The 'base64-vlq' module is directly based off of Apache-licensed code from Google's Closure Compiler. Dave had told me that Apache was compat with our license, but I have found a page which says otherwise: http://www.mozilla.org/MPL/license-policy.html I am going to reach out to Google and see if they can relicense it for us, or if we can work something out. In the mean time, I would appreciate review now, but this shouldn't land yet. * Did I make/modify the Makefiles correctly? I just tried to do similar things to what other Makefiles I see in the code base do, but I would really appreciate some eyeballs on this stuff. * The file SourceMapConsumer.jsm is built in a similar way as the GCLI stuff. The original code repo is here: https://github.com/mozilla/source-map I have that as a separate repo because this same core lib is going to be integrated not only with firefox but also with CoffeeScript, UglifyJS, and anyone else who wants to support source maps (at least that is the idea). I would appreciate some feedback on that whole repo (including source-map-generator, which we aren't using in Firefox, but others will use). * Do the exposed APIs make sense? Clean? Well-named? Is this code easy to pick up? I am interested in answers to all of these questions.

(In reply to comment #2) > Dave had told me that Apache was compat with our > license, but I have found a page which says otherwise: Yeah I was just wrong, sorry about that.

Comment on attachment 547276 [details] [diff] [review] Patch update 2 w/ very small bugfix from mozilla/source-map github repo I apologize for the nits on comments coming up. I don't wanna be that guy. Makefile.in +# +# The Original Code is HUDService code. you probably meant SourceMap code? Original Code is supposed to be the name of the module or code you're describing in the license block. But I see that you get that later on. Just excessive copying? +include $(topsrcdir)/config/rules.mk \ No newline at end of file Gotta have newlines! SourceMapConsumer.jsm + * + * The Original Code is GCLI. orly? :) + * + * + * + * + *********************************** WARNING *********************************** + * + * Do not edit this file without understanding where it comes from, + * Your changes are likely to be overwritten without warning. whoh. That is a pretty heavy warning. Could maybe do away with some of the extra leading and trailing lines of *. + +/* + * This creates a console object that somewhat replicates Firebug's console + * object. It currently writes to dump(), but should write to the web + * console's chrome error section (when it has one) + */ oh, I remember this. Joe's GCLI... utils module does this as well. Is this a straight copy? Ideally, I think we'd prefer to use just one of these logging and require definitions rather than duplicate it in every jsm we include that's built from dryice. Are you even using these logging functions in your code? A quick scan doesn't seem to show any of them. If you can lose this chunk entirely, I would be happier. ... +var debugDependencies = false; + + if (debugDependencies) { + console.log("define: " + moduleName + " -> " + payload.toString() + .slice(0, 40).replace(/\n/, '\\n').replace(/\r/, '\\r') + "..."); + } would could probably remove this in final code. + * The Original Code is Mozilla Source Map. + * + * The Initial Developer of the Original Code is Mozilla. + * Portions created by the Initial Developer are Copyright (C) 2011 + * the Initial Developer. All Rights Reserved. + * + * Contributor(s): + * Nick Fitzgerald <nfitzgerald@mozilla.com> (original author) Here and elsewhere, too much indentation by 3. Should line up under the 'n' in Contributors. Extra license block within same file with the same license! + + /** + * A SourceMapConsumer instance represents a parsed source map which we can + * query for information about the original file positions by giving it a file + * position in the generated source. + */ no @param description. + function SourceMapConsumer (sourceMap) { Nit: Unusual in mozilla code to have whitespace between the function name and the (. Also inconsistent with the earlier parts in this file. Mozilla convention is to use 'a' prefix on parameter names to distinguish the arguments. E.g., aSourceMap. + var version = util.getArg(sourceMap, 'version'); any reason not to use "let" instead of "var"? Tend to prefer it in Mozilla code as it limits the scope of your variables. + + this._names = ArraySet.fromArray(names); ArraySet! (I wish we had those in JS) + // TODO: this._originalMappings? usually include bug numbers in TODOs. + SourceMapConsumer.prototype._version = 3; What's this? A comment could be helpful. + var mappingSeparator = /^[,;]/; this looks like it should be a const. + var mapping; + var temp; are these used outside of their contained blocks? They could be declared inside if not. Also, "temp" is a pretty weak name for a variable. What's in it? + while (str.length > 0) { + if ( str.charAt(0) === ';' ) { nit: not keen on the spacing here, extra whitespace after while and between the brackets after the if. Not critical though. I wouldn't mind some extra comments around the if... else if ... else clauses either to explain what's happening in this algorithm. + // TODO: insert in to this._originalMappings[mapping.source]? bug #. + var needle = { what's this for? comment, please. + function compare (a, b) { what are you comparing here? What are the arguments? + else { + return { + source: null, + line: null, + column: null, + name: null + }; wouldn't it be more succinct to just return null here if there's no mapping? Does the algorithm require the object format? + // TODO: SourceMapConsumer.prototype.generatedPositionFor using this._originalMappings? more todo... + * ***** END LICENSE BLOCK ***** */ +define('source-map/util', ['require', 'exports', 'module' ], function(require, exports, module) { are these auto-generated by something? (dryice, I guess?) Some spacing between the license block and the define would not be horrible. + function recursiveSearch (low, high, needle, haystack, compare) { this needs some documentation on it. Love the parameter names, but they still need comments. :) Recursive method should have a detailed stop condition commented inside as well. + if ( cmp === 0 ) { have we found the needle? + else if ( cmp > 0 ) { + low = mid; + if ( high - I always get a little scared when I see functions modifying their parameters. Maybe keep a local copy of low and high so as not to modify them. + } + else { + high = mid; // cmp < 0 With all these ifs and elses, it's easy to get lost in here. Some additional commenting would help. Getting rid of the extra else conditions would be even better. Since each condition returns something, you can ditch the elses entirely. All of them. ... + ArraySet.prototype.add = function ArraySet_add (str) { + var idx = this._array.length; + this._array.push(str); You don't do any checking if the str is already in the _set before pushing it? I would expect an if (this.has(str)) return; check at the top. Otherwise you'll pollute your array with duplicates. + ArraySet.prototype.toArray = function ArraySet_toArray () { + return this._array.slice(); should probably mention that you're returning a copy of the array in your comment. ... + * The contents of this file is a derivative work of + * http://code.google.com/p/closure-compiler/source/browse/trunk/src/com/google/debugging/sourcemap/Base64VLQ.java, + * its license follows: + * + * Copyright 2011 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 this is changing based on the relicensing discussion. huzzah! +});/* -*- Mode: js; js-indent-level: 2; -*- */ I've seen this on a number of lines. We should really patch up dryice so it's not quite so aggressively munging files together. + */ + exports.encode = function base64_encode (n) { + exports.decode = function base64_decode (ch) { if you could get btoa and atob in there from some window object, you wouldn't need to redefine these, though you don't want to leak anything either. Maybe it's ok. \ No newline at end of file no trailing new line. Overall, this is really nice code. I think it could do with a little extra commenting and describing of the internals. The algorithm itself is nice and simple. I'm going to r- this pass because of the relicensing block that's needed and also for some of the code cleanup around the recursive method, but this is very close, imo.

(In reply to comment #6) > Comment on attachment 547276 [details] [diff] [review] [review] > Patch update 2 w/ very small bugfix from mozilla/source-map github repo > > I apologize for the nits on comments coming up. I don't wanna be that guy. > > Makefile.in > > +# > +# The Original Code is HUDService code. > > you probably meant SourceMap code? Original Code is supposed to be the name > of the module or code you're describing in the license block. But I see that > you get that later on. Just excessive copying? I copied the hudservice makefile and then modified it to work for sourcemap because I'm not one hundred percent on how our build system works. What would be the proper attribution here? > > +include $(topsrcdir)/config/rules.mk > \ No newline at end of file > > Gotta have newlines! > > SourceMapConsumer.jsm > > + * > + * The Original Code is GCLI. > > orly? :) So I copied GCLI's build system and there are lots of little includes which are from there. What would be the proper way to give attribution? Especially because you mention that there are too many license blocks; that seems to cut down on the options for giving credit where it is due. > > + * > + * > + * > + * > + *********************************** WARNING > *********************************** > + * > + * Do not edit this file without understanding where it comes from, > + * Your changes are likely to be overwritten without warning. > > whoh. That is a pretty heavy warning. Could maybe do away with some of the > extra leading and trailing lines of *. Again, this is copied from GCLI. Can remove if you would prefer that. The idea is that this jsm should never be edited directly, you should edit the files from which this jsm is built and then rebuild the jsm. > > + > +/* > + * This creates a console object that somewhat replicates Firebug's console > + * object. It currently writes to dump(), but should write to the web > + * console's chrome error section (when it has one) > + */ > > oh, I remember this. Joe's GCLI... utils module does this as well. Is this a > straight copy? > > Ideally, I think we'd prefer to use just one of these logging and require > definitions rather than duplicate it in every jsm we include that's built > from dryice. > > Are you even using these logging functions in your code? A quick scan > doesn't seem to show any of them. If you can lose this chunk entirely, I > would be happier. Yes this is a straight copy. I believe that the Joe's mini-require system uses the console object (which is also copied and used here). I can rip that stuff out, if you would prefer. > + * The Original Code is Mozilla Source Map. > + * > + * The Initial Developer of the Original Code is Mozilla. > + * Portions created by the Initial Developer are Copyright (C) 2011 > + * the Initial Developer. All Rights Reserved. > + * > + * Contributor(s): > + * Nick Fitzgerald <nfitzgerald@mozilla.com> (original author) > > Here and elsewhere, too much indentation by 3. Should line up under the 'n' > in Contributors. > > Extra license block within same file with the same license! So what is the proper way to respect all the licenses from the various files that get combined in to this one jsm? This issue has cropped up multiple times now in different forms. > + var version = util.getArg(sourceMap, 'version'); > > any reason not to use "let" instead of "var"? Tend to prefer it in Mozilla > code as it limits the scope of your variables. Yes, the idea is that the code in https://github.com/mozilla/source-map (from which this file is generated) should be fully cross engine compatible because it is going to be used by CoffeeScript, UglifyJS, and anyone else who wants to parse or generate source maps. Because of this, I can't rely on let or const. > + var mappingSeparator = /^[,;]/; > > this looks like it should be a const. See previous comment. > + var mapping; > + var temp; > > are these used outside of their contained blocks? They could be declared > inside if not. They should be defined in their blocks even if they are vars and not lets? > Also, "temp" is a pretty weak name for a variable. What's in it? Yes, but I'm unsure what else to call it. The function base64VLQ.decode needs to return two values, and so this is just an object that wraps those values. I thought that using continuation-passing-style to pass multiple args to a continuation function might be frowned upon as overkill, even though it would make the nomenclature better. Something like: base64VLQ.decode(stringInput, function (value, rest) { ... }); versus how it currently is var temp = base64VLQ.decode(stringInput); // temp.value // temp.rest What would be your preference? Any better ideas? > + else { > + return { > + source: null, > + line: null, > + column: null, > + name: null > + }; > > wouldn't it be more succinct to just return null here if there's no mapping? > Does the algorithm require the object format? So the idea is that depending on how much or little information is encoded in the source map, some of these slots could be null anyways. Because of that, I thought it would be cleaner to always have the slots defined, but sometimes they are null, rather than sometimes null is returned *and* sometimes an object is returned which has some of the slots be null. I thought it would just be cleaner, more consistent, and make the user of the api do less work if we just said and object will always be returned, but the slots might contain null. Was this the right choice? > + } > + else { > + high = mid; > > // cmp < 0 > > With all these ifs and elses, it's easy to get lost in here. Some additional > commenting would help. Getting rid of the extra else conditions would be > even better. > > Since each condition returns something, you can ditch the elses entirely. > All of them. So just to make sure I understand what you are saying, you would prefer I change code that looks like this if (foo) { return bar; } else { return baz; } to this if (foo) { return bar; } return baz; ? > + * The contents of this file is a derivative work of > + * > http://code.google.com/p/closure-compiler/source/browse/trunk/src/com/google/ > debugging/sourcemap/Base64VLQ.java, > + * its license follows: > + * > + * Copyright 2011 The Closure Compiler Authors. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > > > this is changing based on the relicensing discussion. huzzah! I see that one of reasons for r- was this licensing stuff; should I just ask for feedback instead of review until the re-licensing stuff is finished? > > +});/* -*- Mode: js; js-indent-level: 2; -*- */ > > > I've seen this on a number of lines. We should really patch up dryice so > it's not quite so aggressively munging files together. > > + */ > + exports.encode = function base64_encode (n) { > > + exports.decode = function base64_decode (ch) { > > if you could get btoa and atob in there from some window object, you > wouldn't need to redefine these, though you don't want to leak anything > either. Maybe it's ok. So the difference is that btoa and atob do more than just a single base64 digit, and they encode multi-digit base64 values differently than how the spec defines for base64 VLQs. (There's reason for all this, but that's a different discussion). What I could do is just check that 0 <= valueBeingEncoded <= 63 before doing the encoding, but at that point what have I really gained over what is there currently? > Overall, this is really nice code. I think it could do with a little extra > commenting and describing of the internals. The algorithm itself is nice and > simple. I'm going to r- this pass because of the relicensing block that's > needed and also for some of the code cleanup around the recursive method, > but this is very close, imo. Great! Expect an updated patch soon.

Summary of changes between this patch and the last one: * Made bugs for TODOs or removed them * Removed excess whitespace in conditionals and functions parens and brackets * Newlines at EOF * Lightened up the "do not edit" warning to be less draconian * Ripped out all the console.log stuff from mini-require * Fixed indentation in contributors lists * Added @param description to the SourceMapConsumer constructor * Made all arguments be prefixed by 'a' * Documented SourceMapConsumer.prototype._version * Commented about the use of binary search in SourceMapConsumer, what needle is, what is being compared, etc * Documented the binary search implementation * Removed unnecessary else branches in the binary search implementation * Modified ArraySet#add to check if the element is already in the set * Documented that ArraySet#toArray returns a copy * Modified binary search so that arguments are not being assigned

Comment on attachment 548497 [details] [diff] [review] Patch update 3 + var mid = Math.floor(aHigh - aLow / 2) + aLow; I believe you meant to put Math.floor((aHigh - aLow) / 2) + aLow

(In reply to comment #9) > Comment on attachment 548497 [details] [diff] [review] [review] > Patch update 3 > > + var mid = Math.floor(aHigh - aLow / 2) + aLow; > > I believe you meant to put > Math.floor((aHigh - aLow) / 2) + aLow Yes! Thanks for catching that!

Comment on attachment 548497 [details] [diff] [review] Patch update 3 + +include $(topsrcdir)/config/rules.mk \ No newline at end of file Quibble: Still no newline in your makefile. + * ***** END LICENSE BLOCK ***** */ +define('source-map/source-map-consumer', ['require', 'exports', 'module' , 'source-map/util', 'source-map/binary-search', 'source-map/array-set', 'source-map/base64-vlq'], function(require, exports, module) { + Can you break this line up a bit? It's kind of massive. We tend to use 80 characters max for JS. Same elsewhere. + var mid = Math.floor(aHigh - aLow / 2) + aLow; thanks do David for catching that. :) + * Copyright 2011 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 Has the originating file been relicensed yet? We should get this updated. +/* -*- Mode: js; js-indent-level: 2; -*- */ +/////////////////////////////////////////////////////////////////////////////// + +let SourceMapConsumer = require('source-map/source-map-consumer').SourceMapConsumer; \ No newline at end of file and no new line. This is coming together. I'll wait to hear back about the file relicensing before giving r+.

(In reply to comment #11) > Comment on attachment 548497 [details] [diff] [review] [review] > Patch update 3 > > + > +include $(topsrcdir)/config/rules.mk > \ No newline at end of file > > Quibble: Still no newline in your makefile. Woops, its been taken care of now. > > + * ***** END LICENSE BLOCK ***** */ > +define('source-map/source-map-consumer', ['require', 'exports', 'module' , > 'source-map/util', 'source-map/binary-search', 'source-map/array-set', > 'source-map/base64-vlq'], function(require, exports, module) { > + > > Can you break this line up a bit? It's kind of massive. We tend to use 80 > characters max for JS. Same elsewhere. This stuff is automatically inserted by dryice, I could write some script to go over the outputted build and make these lines shorter, but I'm not sure it is worth the effort. What do you think? > > + var mid = Math.floor(aHigh - aLow / 2) + aLow; > > thanks do David for catching that. :) Thanks again, David :) > > + * Copyright 2011 The Closure Compiler Authors. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > > Has the originating file been relicensed yet? We should get this updated. > Not yet. They are being extremely understanding as is; I don't really want to pester them... > +/* -*- Mode: js; js-indent-level: 2; -*- */ > +//////////////////////////////////////////////////////////////////////////// > /// > + > +let SourceMapConsumer = > require('source-map/source-map-consumer').SourceMapConsumer; > \ No newline at end of file > > and no new line. This is coming together. Woops again! Fixed. > > I'll wait to hear back about the file relicensing before giving r+. Ok.

Comment on attachment 548582 [details] [diff] [review] Patch update 4 +Domain.prototype.require = function(deps, callback) { + if (Array.isArray(deps)) { + var params = deps.map(function(dep) { + return this.lookup(dep); + }, this); + if (callback) { + callback.apply(null, params); + } + return undefined; + } + else { + return this.lookup(deps); + } +}; still not a fan of the nested returns, but this isn't your code. + + if (mapping) { + return { + source: util.getArg(mapping, 'source', null), + line: util.getArg(mapping, 'originalLine', null), + column: util.getArg(mapping, 'originalColumn', null), + name: util.getArg(mapping, 'name', null) + }; + } + else { + return { + source: null, + line: null, + column: null, + name: null + }; another. OK, this is looking good. When the new license comes in, I'll r+.

Removing more else branches.

Tightening validation and fixing a bug in parsing the source mappings. See https://github.com/mozilla/source-map/commit/bb43e6166e6e5d9c9775d21a926d794a9d57d8d7

Bug triage, filter on PEGASUS.

Comment on attachment 551825 [details] [diff] [review] Patch update 6 oof. This request was 166 days old. Canceling review request for now. Please re-request with an updated/refreshed patch.

I have un-bitrotted this patch, and added the unit tests which previously only ran on nodejs but now will run in xpcshell.

Comment on attachment 635484 [details] [diff] [review] Patch update 7 Review of attachment 635484 [details] [diff] [review]: ----------------------------------------------------------------- I assume I'm r?ed due to the testing system we discussed? In which case, I like. 2 thoughts: - GCLI is moving to Apache2 rather than BSD. Some people care, about such things and some don't. If you care and prefer Apache2 you might consider it. - I've been trying to move the custom-Mozilla parts of GCLI into central. I'm not done with that process, but I think it's a good direction. Eventually we might be able to optionally include the node step in a standard build.

(In reply to Joe Walker from comment #20) > I assume I'm r?ed due to the testing system we discussed? Yeah, and dcamp said we needed more reviewers :P If you want to see how I sewed the build process together for the tests in more detail, here is the dryice Makefile, which should lead you to everything: https://github.com/mozilla/source-map/blob/master/Makefile.dryice.js#L93 > 2 thoughts: > - GCLI is moving to Apache2 rather than BSD. Some people care, about such > things and some don't. If you care and prefer Apache2 you might consider it. What are the advantages of making this switch? How would the source-map lib benefit? > - I've been trying to move the custom-Mozilla parts of GCLI into central. > I'm not done with that process, but I think it's a good direction. > Eventually we might be able to optionally include the node step in a > standard build. Are you moving the dryice stuff over as well? Or just having dryice reference files from moz-central?

Comment on attachment 635484 [details] [diff] [review] Patch update 7 Review of attachment 635484 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/sourcemap/SourceMap.jsm @@ +33,5 @@ > + > + // TODO: bug 673487 > + // > + // Sometime in the future, if we decide we need to be able to query where in > + // the generated source a peice of the original code came from, we may want to If you care: s/peice/piece @@ +53,5 @@ > + * - sources: An array of URLs to the original source files. > + * - names: An array of identifiers which can be referrenced by individual mappings. > + * - sourceRoot: Optional. The URL root from which all sources are relative. > + * - mappings: A string of base64 VLQs which contain the actual mappings. > + * - file: The generated file this source map is associated with. It would be super useful to have an example to work from here. I used https://gist.github.com/2553381, but I'm guessing that you have a better URL? @@ +75,5 @@ > + > + this._names = ArraySet.fromArray(names); > + this._sources = ArraySet.fromArray(sources); > + this._generatedMappings = []; > + this._parseMappings(mappings, sourceRoot); It's not obvious to me what we expect from these lines. Could you document the expected shape of this._generatedMappings? ::: toolkit/devtools/sourcemap/tests/unit/Utils.jsm @@ +31,5 @@ > + exports.init = function (throw_fn) { > + do_throw = throw_fn; > + }; > + > + exports.deepEqual = function (actual, expected, msg) { Didn't we come to the conclusion that deepEqual was a poorly defined idea that we could do without? @@ +110,5 @@ > + * Copyright 2011 Mozilla Foundation and contributors > + * Licensed under the New BSD license. See LICENSE or: > + * http://opensource.org/licenses/BSD-3-Clause > + */ > +define('test/source-map/util', ['require', 'exports', 'module' ], function(require, exports, module) { I've got a nasty feeling that some spec demands [_a-z] in module names. If that doesn't jog any memories, then I'm not sure we care? Discuss.

(In reply to Nick Fitzgerald :fitzgen from comment #21) > (In reply to Joe Walker from comment #20) > > > I assume I'm r?ed due to the testing system we discussed? > > Yeah, and dcamp said we needed more reviewers :P So I'm having a deeper look - see Comment 22. > > 2 thoughts: > > - GCLI is moving to Apache2 rather than BSD. Some people care, about such > > things and some don't. If you care and prefer Apache2 you might consider it. > > What are the advantages of making this switch? How would the source-map lib > benefit? APLv2 has patent protection, and more up to date wording, and it's preferred by people that care (from what I can see). The choice of BSD seems mostly to be about not having the license get in the way, in which case picking APLv2 seems like an upgrade on BSD, because it's what many People That Care pick. > > - I've been trying to move the custom-Mozilla parts of GCLI into central. > > I'm not done with that process, but I think it's a good direction. > > Eventually we might be able to optionally include the node step in a > > standard build. > > Are you moving the dryice stuff over as well? Or just having dryice > reference files from moz-central? I'd like to move the dryice build files across as well. One day. Maybe.

I should add that I stopped reviewing because I thought I could do a better job with the docs requested in Comment 22. I can carry on where I left off with some extra docs.

(In reply to Joe Walker from comment #22) > @@ +53,5 @@ > > + * - sources: An array of URLs to the original source files. > > + * - names: An array of identifiers which can be referrenced by individual mappings. > > + * - sourceRoot: Optional. The URL root from which all sources are relative. > > + * - mappings: A string of base64 VLQs which contain the actual mappings. > > + * - file: The generated file this source map is associated with. > > It would be super useful to have an example to work from here. > I used https://gist.github.com/2553381, but I'm guessing that you have > a better URL? What do you think would be best: an example source map, a link to the source map spec, or both? > > ::: toolkit/devtools/sourcemap/tests/unit/Utils.jsm > @@ +31,5 @@ > > + exports.init = function (throw_fn) { > > + do_throw = throw_fn; > > + }; > > + > > + exports.deepEqual = function (actual, expected, msg) { > > Didn't we come to the conclusion that deepEqual was a poorly defined idea > that we could do without? I think I do remember that conversation. I'll rewrite the tests to avoid it. > @@ +110,5 @@ > > + * Copyright 2011 Mozilla Foundation and contributors > > + * Licensed under the New BSD license. See LICENSE or: > > + * http://opensource.org/licenses/BSD-3-Clause > > + */ > > +define('test/source-map/util', ['require', 'exports', 'module' ], function(require, exports, module) { > > I've got a nasty feeling that some spec demands [_a-z] in module names. > If that doesn't jog any memories, then I'm not sure we care? > Discuss. I don't know about any spec, but I am 90% sure I have used libraries on npm that have hyphens, and everything currently works (and looks pretty IMO :P), so I'm not really worried about it. If someone else has really strong feelings on the matter, I'd be happy to change it. It would cause a backwards incompatibility for people using the source map lib now. Thanks for the feedback Joe, I'm fixing up the patch now.

(In reply to Nick Fitzgerald :fitzgen from comment #25) > (In reply to Joe Walker from comment #22) > > @@ +53,5 @@ > > > + * - sources: An array of URLs to the original source files. > > > + * - names: An array of identifiers which can be referrenced by individual mappings. > > > + * - sourceRoot: Optional. The URL root from which all sources are relative. > > > + * - mappings: A string of base64 VLQs which contain the actual mappings. > > > + * - file: The generated file this source map is associated with. > > > > It would be super useful to have an example to work from here. > > I used https://gist.github.com/2553381, but I'm guessing that you have > > a better URL? > > What do you think would be best: an example source map, a link to the source > map spec, or both? I'm greedy. Both. > > @@ +110,5 @@ > > > + * Copyright 2011 Mozilla Foundation and contributors > > > + * Licensed under the New BSD license. See LICENSE or: > > > + * http://opensource.org/licenses/BSD-3-Clause > > > + */ > > > +define('test/source-map/util', ['require', 'exports', 'module' ], function(require, exports, module) { > > > > I've got a nasty feeling that some spec demands [_a-z] in module names. > > If that doesn't jog any memories, then I'm not sure we care? > > Discuss. > > I don't know about any spec, but I am 90% sure I have used libraries on npm > that have hyphens, and everything currently works (and looks pretty IMO :P), > so I'm not really worried about it. If someone else has really strong > feelings on the matter, I'd be happy to change it. It would cause a > backwards incompatibility for people using the source map lib now. Good enough for me.

Updated the patch as per feedback from Joe.

Comment on attachment 638026 [details] [diff] [review] Patch update 8 Review of attachment 638026 [details] [diff] [review]: ----------------------------------------------------------------- +1, like, RT, etc. ::: toolkit/devtools/sourcemap/SourceMap.jsm @@ +108,5 @@ > + // > + // All properties except for `generatedLine` and `generatedColumn` can be > + // `null`. > + this._generatedMappings = []; > + this._parseMappings(mappings, sourceRoot); This is a take-it-or-leave-it type thing. I'd be tempted to return the generatedMappings from parseMappings. Something to do with pure functions, and lack of side effects. I once got coughed on by someone who knew Haskell, I think. @@ +293,5 @@ > + } else { > + throw new Error('"' + aName + '" is a required argument.'); > + } > + } > + exports.getArg = getArg; I can't say I'm a fan of getArg. It feels like the code shrinkage might not be worth the fact that everyone that reads the code now needs to lookup what getArg does. This isn't a 'please change this' (unless dcamp/robcee chime in) because it's just a style thing; more food for thought. @@ +491,5 @@ > + // Continuation > + // | Sign > + // | | > + // V V > + // 101011 https://en.wikipedia.org/wiki/Variable-length_quantity Seems to suggest that VLQ is unsigned, and that signed integers are an extension - 'In the data format for Unreal Packages used by the Unreal Engine, a variable length quantity scheme called Compact Indices is used. The only difference in this encoding is that the first VLQ has the sixth binary digit reserved to indicate whether the encoded integer is positive or negative. I note that CI uses a different sign bit. I'm not sure we value standardization between web compiler by-products and game data package formats, but I do wonder about the naming. Or maybe Wikipedia is wrong?

(In reply to Joe Walker from comment #28) > Comment on attachment 638026 [details] [diff] [review] > Patch update 8 > > Review of attachment 638026 [details] [diff] [review]: > ----------------------------------------------------------------- > > +1, like, RT, etc. > > ::: toolkit/devtools/sourcemap/SourceMap.jsm > @@ +108,5 @@ > > + // > > + // All properties except for `generatedLine` and `generatedColumn` can be > > + // `null`. > > + this._generatedMappings = []; > > + this._parseMappings(mappings, sourceRoot); > > This is a take-it-or-leave-it type thing. I'd be tempted to return the > generatedMappings from parseMappings. Something to do with pure functions, > and lack of side effects. > I once got coughed on by someone who knew Haskell, I think. Haha :P I understand, but this *is* an object oriented library. *shrugs* > @@ +293,5 @@ > > + } else { > > + throw new Error('"' + aName + '" is a required argument.'); > > + } > > + } > > + exports.getArg = getArg; > > I can't say I'm a fan of getArg. It feels like the code shrinkage might not > be worth the fact that everyone that reads the code now needs to lookup what > getArg does. > This isn't a 'please change this' (unless dcamp/robcee chime in) because > it's just a style thing; more food for thought. Tongue in cheek: "I stopped using functions in my code and just made one big 'main' function because other developers had to look up function definitions and understand abstraction." In all seriousness, sometimes I wanted default arguments and sometimes I needed to throw an error. Throwing errors in particular is very verbose because it is a statement rather than an expression so you can't just say, var foo = args.foo || throw new Error(...); The getArg function cleanly wrapped up all the use cases. I think that the lack of verbosity is worth it. It is a personal opinion, but I feel fairly strong about it, since it does get used quite a bit throughout the whole library. > @@ +491,5 @@ > > + // Continuation > > + // | Sign > > + // | | > > + // V V > > + // 101011 > > https://en.wikipedia.org/wiki/Variable-length_quantity Seems to suggest that > VLQ is unsigned, and that signed integers are an extension - 'In the data > format for Unreal Packages used by the Unreal Engine, a variable length > quantity scheme called Compact Indices is used. The only difference in this > encoding is that the first VLQ has the sixth binary digit reserved to > indicate whether the encoded integer is positive or negative. > I note that CI uses a different sign bit. > > I'm not sure we value standardization between web compiler by-products and > game data package formats, but I do wonder about the naming. > > Or maybe Wikipedia is wrong? I think wikipedia is referring to Unreal's specific implementation of a variant of VLQ. Source maps use *a* VLQ, not *the* VLQ, if that makes sense. Regardless, the whole base64-vlq module is a port of Closure Compiler's VLQ module that they use to generate source maps.

(In reply to Nick Fitzgerald :fitzgen from comment #29) > (In reply to Joe Walker from comment #28) > > Comment on attachment 638026 [details] [diff] [review] > > Patch update 8 > > > > Review of attachment 638026 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > +1, like, RT, etc. > > > > ::: toolkit/devtools/sourcemap/SourceMap.jsm > > @@ +108,5 @@ > > > + // > > > + // All properties except for `generatedLine` and `generatedColumn` can be > > > + // `null`. > > > + this._generatedMappings = []; > > > + this._parseMappings(mappings, sourceRoot); > > > > This is a take-it-or-leave-it type thing. I'd be tempted to return the > > generatedMappings from parseMappings. Something to do with pure functions, > > and lack of side effects. > > I once got coughed on by someone who knew Haskell, I think. > > Haha :P > > I understand, but this *is* an object oriented library. *shrugs* So the point where we're discussing if JS is object-oriented or functional is the point where we should move the discussion to the pub. :) > > @@ +293,5 @@ > > > + } else { > > > + throw new Error('"' + aName + '" is a required argument.'); > > > + } > > > + } > > > + exports.getArg = getArg; > > > > I can't say I'm a fan of getArg. It feels like the code shrinkage might not > > be worth the fact that everyone that reads the code now needs to lookup what > > getArg does. > > This isn't a 'please change this' (unless dcamp/robcee chime in) because > > it's just a style thing; more food for thought. > > Tongue in cheek: "I stopped using functions in my code and just made one big > 'main' function because other developers had to look up function definitions > and understand abstraction." > > In all seriousness, sometimes I wanted default arguments and sometimes I > needed to throw an error. Throwing errors in particular is very verbose > because it is a statement rather than an expression so you can't just say, > > var foo = args.foo || throw new Error(...); > > The getArg function cleanly wrapped up all the use cases. > > I think that the lack of verbosity is worth it. It is a personal opinion, > but I feel fairly strong about it, since it does get used quite a bit > throughout the whole library. That's fine. I just deleted my reply in favor of a debate in the pub. > > @@ +491,5 @@ > > > + // Continuation > > > + // | Sign > > > + // | | > > > + // V V > > > + // 101011 > > > > https://en.wikipedia.org/wiki/Variable-length_quantity Seems to suggest that > > VLQ is unsigned, and that signed integers are an extension - 'In the data > > format for Unreal Packages used by the Unreal Engine, a variable length > > quantity scheme called Compact Indices is used. The only difference in this > > encoding is that the first VLQ has the sixth binary digit reserved to > > indicate whether the encoded integer is positive or negative. > > I note that CI uses a different sign bit. > > > > I'm not sure we value standardization between web compiler by-products and > > game data package formats, but I do wonder about the naming. > > > > Or maybe Wikipedia is wrong? > > I think wikipedia is referring to Unreal's specific implementation of a > variant of VLQ. Source maps use *a* VLQ, not *the* VLQ, if that makes sense. > Regardless, the whole base64-vlq module is a port of Closure Compiler's VLQ > module that they use to generate source maps. Fine.

(In reply to Joe Walker from comment #30) > (In reply to Nick Fitzgerald :fitzgen from comment #29) > > I understand, but this *is* an object oriented library. *shrugs* > > So the point where we're discussing if JS is object-oriented or functional > is the point where we should move the discussion to the pub. :) Quickly, everyone to the pub! (simple answer: JS is neither and both. Drinkup.)

Comment on attachment 638026 [details] [diff] [review] Patch update 8 Review of attachment 638026 [details] [diff] [review]: ----------------------------------------------------------------- Not a lot's changed since the last review pass. This is great stuff. One issue is the naming of the VLQ64 library. Maybe it's ok. It says 64 right on the header, but it does make me wonder if I'm reading the spec correctly, if wikipedia's wrong and if there's something else I should be looking up. DuckDuckGo was not helpful finding other VLQ implementations. ::: toolkit/devtools/sourcemap/SourceMap.jsm @@ +108,5 @@ > + // > + // All properties except for `generatedLine` and `generatedColumn` can be > + // `null`. > + this._generatedMappings = []; > + this._parseMappings(mappings, sourceRoot); Yeah, I'm with Joe on this. No real need to separate these. But Whatevs. @@ +293,5 @@ > + } else { > + throw new Error('"' + aName + '" is a required argument.'); > + } > + } > + exports.getArg = getArg; it is a little funny, but I'll allow it if it helps fitzgen keep his moustache in order. @@ +359,5 @@ > + return aLow < 0 > + ? null > + : aHaystack[aLow]; > + } > + } I've reviewed this before! :) @@ +491,5 @@ > + // Continuation > + // | Sign > + // | | > + // V V > + // 101011 According to the above, this is a 64-bit VLQ format, and not the one(s) mentioned in the wikipedia article (they're 128-bit encodings). This does suggest that the naming is somewhat ambiguous however, especially if you're not following their MSB signing convention. Again, not sure that matters, but if people go wikipediaing VLQ, they might scratch their heads a little at this implementation. Want citation for 64-bit VLQ format.

actually, scratch that, you said it was based off of clojure's implementation. Maybe a reference in the comments to go along with it?

Added comment noting that our base 64 vlq implementation is based off of Closure Compiler's base 64 vlq implementation.

Comment on attachment 643585 [details] [diff] [review] patch update 9 looks like the VLQ reference will require the following license block to be included: /* * Copyright 2011 The Closure Compiler Authors. All rights reserved. * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are * met: * * * Redistributions of source code must retain the above copyright * notice, this list of conditions and the following disclaimer. * * Redistributions in binary form must reproduce the above * copyright notice, this list of conditions and the following * disclaimer in the documentation and/or other materials provided * with the distribution. * * Neither the name of Google Inc. nor the names of its * contributors may be used to endorse or promote products derived * from this software without specific prior written permission. * * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */

Include the license block from the Closure Compiler base 64 vlq implementation file.

waiting on a try push as the last one was nearly a year ago. :)

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