Closed Bug 924466 Opened 7 years ago Closed 7 years ago

integrate acorn with the devtools

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: bbenvie, Assigned: bbenvie)

References

Details

(Whiteboard: [sec-review][qa-])

Attachments

(1 file, 4 obsolete files)

We'd like to add a third party JavaScript parser to the tree because Reflect.parse isn't adequate for our needs [1]. The two options we have that produce compatible AST are acorn [2] and esprima [3].

acorn pros:
* faster
* succinct code (~40% the code to do the same thing)
* better error-recovering parser

esprima pros:
* better tokenizer exposed
* has support for most ES6 features (in its harmony branch)


[1] https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/cWANPm0FWVM
[2] https://github.com/marijnh/acorn
[3] https://github.com/ariya/esprima
The decision here also would have an impact on which tool we use for code completion. Tern [1] depends on acorn, while aulx [2] depends on esprima. These tools rely on the lenient parsing modes and tokenizers that each of the parsers provide, which don't have compatible APIs.
Blocks: 762164
Priority: -- → P3
Assignee: nobody → bbenvie
After discussing it with a number of people, the consensus is to use acorn and to address the things it is missing via either contributing changes back to acorn or to ask acorn's owner (Marijn Haverbeke) about getting them implemented.
Be vary that this means that we will have to put some (maybe a lot of ? ) extra effort in:
- Making the Acorn's lexar better (as compared to esprima)
- Having the support of ES6 up to the level so that ACorn is actually useful in Chrome tools like browser debugger, browser level scratchpad, etc.

[This is not a discussion, I am okay with any option. Just wanted put it out here amidst the fact that we are already getting bitten by the lack of ES6 support in other external tools like CM, escodegen etc.]
Attached patch acorn.patch (obsolete) — Splinter Review
Most of the work is done here. I'd probably like to add some more tests though.
Bug 932416 is the legal review submission.
Whiteboard: [sec-review]
Blocks: 932456
Attached patch acorn.patch (obsolete) — Splinter Review
Fixes test_same_ast.js, adds test_parse_dammit.js.
Attachment #824158 - Attachment is obsolete: true
Summary: integrate a third party JavaScript parser with the devtools → integrate acorn with the devtools
Attached patch acorn.patch (obsolete) — Splinter Review
Adds acorn to license.html.
Attachment #824255 - Attachment is obsolete: true
Attached patch acorn.patch (obsolete) — Splinter Review
Clean up the tests a bit. https://tbpl.mozilla.org/?tree=Try&rev=1c3c634bf814
Attachment #824265 - Attachment is obsolete: true
Comment on attachment 824287 [details] [diff] [review]
acorn.patch

robcee for the general review
dcamp for loader changes
gps for adding a new folder to the build system
Attachment #824287 - Flags: review?(gps)
Attachment #824287 - Flags: review?(dcamp)
Attachment #824287 - Flags: review?(rcampbell)
Comment on attachment 824287 [details] [diff] [review]
acorn.patch

Review of attachment 824287 [details] [diff] [review]:
-----------------------------------------------------------------

Covers just the build bits.
Attachment #824287 - Flags: review?(gps) → review+
Comment on attachment 824287 [details] [diff] [review]
acorn.patch

Review of attachment 824287 [details] [diff] [review]:
-----------------------------------------------------------------

I hate to be that guy, but could we name the test test_lenient_parser.js?
(In reply to Dave Camp (:dcamp) from comment #12)
> I hate to be that guy, but could we name the test test_lenient_parser.js?

Hah, of course.
Comment on attachment 824287 [details] [diff] [review]
acorn.patch

Review of attachment 824287 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me, assuming that file is renamed.
Attachment #824287 - Flags: review?(dcamp) → review+
Attached patch acorn.patchSplinter Review
Rebase and a few changes:

* Rename test_parse_dammit.js to test_lenient_parser.js
* Remove package.json.js since it's not actually needed
Attachment #824287 - Attachment is obsolete: true
Attachment #824287 - Flags: review?(rcampbell)
Attachment #825584 - Flags: review?(rcampbell)
Comment on attachment 825584 [details] [diff] [review]
acorn.patch

Review of attachment 825584 [details] [diff] [review]:
-----------------------------------------------------------------

looks ok to me too.
Attachment #825584 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/integration/fx-team/rev/7e81c446b6ca
Whiteboard: [sec-review] → [sec-review][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7e81c446b6ca
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [sec-review][fixed-in-fx-team] → [sec-review]
Target Milestone: --- → Firefox 28
Whiteboard: [sec-review] → [sec-review][qa-]
Sorry for picking up this old bug, but do you have a tracker for porting ES6 features to acorn?
I don't think so. Bug 938203 also depends on acorn supporting ES6.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.