We currently have no way to get all the generated positions for a given (source, originalLine) pair in a source map. We need this so we can properly implement breakpoints when a source map is present.

Assuming we use the same review process for patches that go outside the main source tree. I had to refactor binarySearch to return an index instead of the actual mapping so I could use it to find the index of the last mapping for a given line and then iterate downwards from that (as opposed to calling binarySearch for each mapping on the given line).

Comment on attachment 8523865 [details] [diff] [review] Implement SourceMapConsumer.allGeneratedPositionsFor Review of attachment 8523865 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall, thanks! Couple small comments below. Please update the binary search doc comments to mention that it returns an index into the array, not the actual value. Please document this new method in README.md. Finally, send this patch as a pull request to https://github.com/mozilla/source-map/ so that our test suite gets run on travis-ci, and assuming it is green and you've made the updates I've requested here, I will merge it in to the github repo, and then you can use this bug to update the tree's copy of the repo to the latest version w/ r=me. ::: lib/source-map/source-map-consumer.js @@ +471,5 @@ > + line: util.getArg(mapping, 'generatedLine', null), > + column: util.getArg(mapping, 'generatedColumn', null) > + }); > + > + mapping = this._originalMappings[--index]; Why `--index`? Wouldn't you want to search both directions, as long as the mappings are on the same target line? Ah wait, I see you're using `infinity` in the needle, and relying on the fact that our binary search return the closest mapping less than the needle when there is no exact match. A comment to this effect would be lurvely. ----- What if `mapping` gets set to `undefined` because index goes out of bounds? It seems like the while loop's condition should be checking for this. ::: test/source-map/test-source-map-consumer.js @@ +289,5 @@ > assert.equal(pos.line, 2); > assert.equal(pos.column, 2); > }; > > + exports['test allGeneratedPositionsFor'] = function (assert, util) { Please add tests for: * a line with no mappings associated with it (ie, the binary search finds a mapping, but it isn't one on our target line) and * a source map with no mappings at all (ie, the binary search fails) @@ +321,5 @@ > + }); > + map = new SourceMapConsumer(map.toString()); > + > + var mappings = map.allGeneratedPositionsFor({ > + line: 2, If you do `line: 1`, I think you'll run into the out of bounds issue I mention above.

Also, because we iterate over the mappings from highest to lowest when accumulating our result, won't the mappings be in reverse order? Either we should ensure that they are sorted (either by reversing or finding the start mapping and then iterating back to accumulate) or we should document that the order of the result is undefined and not guaranteed to be in any specific order.

This already landed in the source-map library on github. It just needs to be merged with fx-team. I have another source map related bug that will have to be merged with fx-team (bug 1126190), so the most efficient thing to do is probably to merge all the changes in the source map library in one go in that bug, and close this one.