Closed Bug 908913 Opened 11 years ago Closed 11 years ago

integrate escodegen with devtools

Categories

(DevTools :: Debugger, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

escodegen will take a spidermonkey AST and pretty-ify it and make source maps too. If we can leverage this, we get 50% of the way to fixing bug 762761.
Assignee: nobody → nfitzgerald
Priority: -- → P2
Status: NEW → ASSIGNED
Attached patch wip (obsolete) — Splinter Review
Lets you require("escodegen/escodegen") and generate code. Doesn't work with source maps because I can't figure out how to load SourceMap.jsm through the loader.

In the loader's paths, if I add

  "source-map": "resource://gre/modules/devtools/SourceMap",

then it tries to load

  "resource://gre/modules/devtools/SourceMap.js" // .js, not .jsm

If I add

  "source-map": "resource://gre/modules/devtools/SourceMap.jsm",

then it tries to load

  "resource://gre/modules/devtools/SourceMap.jsm.js"

Halp!

My goal is to integrate escodegen without modifying its source at all!
Flags: needinfo?(jwalker)
Flags: needinfo?(dcamp)
Should I just add SourceMap.js which just does:

  module.exports = Cu.import("resource://gre/modules/devtools/SourceMap.jsm", {});

?
I think we can replace GCLI ` javascript beautifier (jsb)` command's backend with escodegen. This would be a appropriate way.
Blocks: 909018
No longer depends on: 909018
(In reply to Tetsuharu OHZEKI [UTC+9] from comment #3)
> I think we can replace GCLI ` javascript beautifier (jsb)` command's backend
> with escodegen. This would be a appropriate way.

Yeah, also for the scratchpad pretty printing / deobfuscation bug (which I can't seem to find at the moment) will be able to use this.
You have a few options to load the jsm from the loader:
* Rename it to .js
* Talk to irakli about how to best add support for .jsm to loader.js
* You could add a getter to Loader.jsm's "modules" thing that imports the jsm
Flags: needinfo?(dcamp)
Flags: needinfo?(jwalker)
Attached patch integrate-escodegen.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=b18fc72002c2
Attachment #794967 - Attachment is obsolete: true
Attachment #796083 - Flags: review?(rcampbell)
Comment on attachment 796083 [details] [diff] [review]
integrate-escodegen.patch

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

::: toolkit/devtools/Loader.jsm
@@ +61,5 @@
>  
> +        "escodegen/escodegen": "resource://gre/modules/devtools/escodegen/escodegen",
> +        "escodegen/package.json": "resource://gre/modules/devtools/escodegen/package.json",
> +        "estraverse": "resource://gre/modules/devtools/escodegen/estraverse",
> +

looks fine to me, but you should r? dcamp for loader changes.

::: toolkit/devtools/escodegen/UPGRADING.md
@@ +10,5 @@
> +
> +2. Make sure that all tests pass:
> +
> +       $ npm install .
> +       $ npm test

does this run the tests in Firefox or just node? I you can figure out how to update these to run in Firefox (stand-alone, not automated), that'd be sweet.
Attachment #796083 - Flags: review?(rcampbell) → review+
Attachment #796083 - Flags: review?(dcamp)
Comment on attachment 796083 [details] [diff] [review]
integrate-escodegen.patch

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

Looks good, a few changes needed below.

I think adding a directory is enough to require build peer review according to the new rules.  I'm feedback+'ing, but flag :gps after making the moz.build change below.

::: toolkit/devtools/Loader.jsm
@@ +42,5 @@
>  }
>  
>  // Used when the tools should be loaded from the Firefox package itself (the default)
>  var BuiltinProvider = {
>    load: function(done) {

You'll need to replicate the changes made to the BuiltinProvider to the srcdir provider (sorry).

::: toolkit/devtools/escodegen/moz.build
@@ +3,5 @@
> +# 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/.
> +
> +TEST_DIRS += ['tests']

You can add:
----

JS_MODULES_PATH = 'modules/devtools/escodegen'

EXTRA_JS_MODULES += [
    'escodegen.js',
    'extraverse.js',
    'package.json.js'
]
---

To your moz.build, and then since that's all Makefile.in is doing in this directory you can remove it entirely.

::: toolkit/devtools/sourcemap/Makefile.in
@@ +13,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  libs::
>  	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools
> +	$(NSINSTALL) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools

I guess this won't be needed if source-map.js goes away?

::: toolkit/devtools/sourcemap/source-map.js
@@ +1,4 @@
> +const { Cu } = require("chrome");
> +const { SourceNode, SourceMapConsumer, SourceMapGenerator } = Cu.import("resource://gre/modules/devtools/SourceMap.jsm", {});
> +exports.SourceNode = SourceNode;
> +exports.SourceMapConsumer = SourceMapConsumer;

Is this still necessary with the modules hack?
Attachment #796083 - Flags: review?(dcamp) → review+
Attachment #796083 - Flags: review+ → feedback+
Flagging gps for build system review.

Applied dcamp's feedback:

* use moz.build instead of makefiles

* remove source-map.js file which wasn't needed any longer

* add source-map to SrcdirProvider

https://tbpl.mozilla.org/?tree=Try&rev=376505973569
Attachment #796083 - Attachment is obsolete: true
Attachment #796503 - Flags: review?(gps)
(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> ::: toolkit/devtools/escodegen/UPGRADING.md
> @@ +10,5 @@
> > +
> > +2. Make sure that all tests pass:
> > +
> > +       $ npm install .
> > +       $ npm test
> 
> does this run the tests in Firefox or just node? I you can figure out how to
> update these to run in Firefox (stand-alone, not automated), that'd be sweet.

I talked to Constellation on twitter and he seemed receptice to making (most) of the tests runnable in content pages. I doubt we will be able to integrate with our test infrastructure, but at least we can do testing manually every time we update escodegen in our tree.

Will take a look into getting this going once the devtools meet up is over.

https://github.com/Constellation/escodegen/issues/124
Comment on attachment 796503 [details] [diff] [review]
integrate-escodegen.patch

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

Just the build bits.

::: toolkit/devtools/escodegen/tests/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +MODULE = 'test_escodegen'
> +
> +XPCSHELL_TESTS_MANIFESTS += ['unit/xpcshell.ini']

I'd just remove the moz.build from this directory and move the XPCSHELL_TESTS_MANIFESTS into the parent moz.build. No sense to pollute the tree with near empty moz.build files if it can be avoided.
Attachment #796503 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/daebcfc9338b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Depends on: 913301
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.