Update JS Beautify and move it into toolkit

RESOLVED FIXED in Firefox 33

Status

defect
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: miker, Assigned: miker)

Tracking

unspecified
Firefox 33
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Posted patch update-jsbeautify.patch (obsolete) — Splinter Review
JS Beautify does bad things to fat arrow functions so it needs to be updated.

I will move it into toolkit at the same time.
Attachment #8449441 - Flags: review?(jwalker)
Comment on attachment 8449441 [details] [diff] [review]
update-jsbeautify.patch

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

Could you create another patch that has "hg mv .../Jsbeautify.jsm .../jsbeautify.js" so we can see changes

::: toolkit/devtools/jsbeautify.js
@@ +1,3 @@
> +/* 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/. */

We've got 2 license headers here...
Comment on attachment 8449441 [details] [diff] [review]
update-jsbeautify.patch

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

Also - How ES6 ready is this? Does it handle let/yield/function* too?
Attachment #8449441 - Flags: review?(jwalker)
(In reply to Joe Walker [:jwalker] from comment #1)
> Comment on attachment 8449441 [details] [diff] [review]
> update-jsbeautify.patch
> 
> Review of attachment 8449441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you create another patch that has "hg mv .../Jsbeautify.jsm
> .../jsbeautify.js" so we can see changes
> 

Done.

> ::: toolkit/devtools/jsbeautify.js
> @@ +1,3 @@
> > +/* 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/. */
> 
> We've got 2 license headers here...
> 

Removed

> Also - How ES6 ready is this? Does it handle let/yield/function* too?

Yup, it does.
Attachment #8449441 - Attachment is obsolete: true
Attachment #8449505 - Flags: review?(jwalker)
Attachment #8449505 - Attachment is obsolete: true
Attachment #8449505 - Flags: review?(jwalker)
Attachment #8449506 - Flags: review?(jwalker)
IRC log:

<jwalker> fitzgen: ... might the be useful for scratchpad es6 beautification?
<jwalker> if "yes" or "yes, but not now" then we'll put the lib somewhere generic
<jwalker> if "never" then we might put it with the jsbeautify command
<fitzgen> jwalker: as far as pretty printing goes, I don't have time to talk at the moment, but I will chirp in on the bug
Joe asked on IRC if we could use this in the scratchpad/debugger.

Does it create source maps now? (I don't believe it did before). If not, then we could use it for scratchpad, but not debugger. It should also respect "devtools.editor.tabsize" as far as indentation level goes, like the current scratchpad pretty print does.
Comment on attachment 8449506 [details] [diff] [review]
update-jsbeautify-1033387.patch

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

r+ from me, but I'd like fitzgen to comment on where it should go.

::: toolkit/devtools/gcli/commands/jsb.js
@@ +8,5 @@
>  const gcli = require("gcli/index");
>  const XMLHttpRequest = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"];
>  
>  loader.lazyImporter(this, "Preferences", "resource://gre/modules/Preferences.jsm");
> +const {js_beautify} = require("devtools/toolkit/jsbeautify");

We should do this lazily, and probably use camel case?
Attachment #8449506 - Flags: review?(jwalker) → review+
Why would we use this and not pretty-fast?  Seems that pretty-fast would match the pretty printed source from the debugger (for Bug 736078)
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Why would we use this and not pretty-fast?  Seems that pretty-fast would
> match the pretty printed source from the debugger (for Bug 736078)

It supports some ES6 features, where as pretty-fast doesn't yet. Not sure what that bug has to do with this, though.
(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > Why would we use this and not pretty-fast?  Seems that pretty-fast would
> > match the pretty printed source from the debugger (for Bug 736078)
> 
> It supports some ES6 features, where as pretty-fast doesn't yet. Not sure
> what that bug has to do with this, though.

OK, got it.  That bug is the reason for updating this AFAIK.  I'm not against updating this, just wondering why we would use this instead of the debugger's pretty printing in that particular case - I'll leave a comment in 736078 about that.
Comment on attachment 8449506 [details] [diff] [review]
update-jsbeautify-1033387.patch

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

I would put it in a directory and import the 3rd party tests as well, just like we do with acorn, code mirror, pretty-fast, source-map, etc:

devtools/
  toolkit/
    jsbeautify/
      jsbeautify.js
      tests/
        ...

Probably need build team review as well.

::: toolkit/devtools/gcli/commands/jsb.js
@@ +8,5 @@
>  const gcli = require("gcli/index");
>  const XMLHttpRequest = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"];
>  
>  loader.lazyImporter(this, "Preferences", "resource://gre/modules/Preferences.jsm");
> +const {js_beautify} = require("devtools/toolkit/jsbeautify");

Can use this:

  devtools.lazyRequireGetter(this, "jsBeautify", "devtools/toolkit/jsbeautify");
Attachment #8449506 - Flags: feedback+
Assignee: nobody → mratcliffe
gps: Asking you for build team review due to extra tree stuff.

fitzgen: You had the most input about how this is to be done it probably makes sense for you to review it.

To get the test suite running we also needed the css and html components of JSBeautify. They can be accessed via:
const { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
devtools.lazyRequireGetter(this, "beautify", "devtools/jsbeautify");

beautify.css(src, [options]);
beautify.html(src, [options]);
beautify.js(src, [options]);

Obviously, beautify.html also prettifies any js or css in src.

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > Why would we use this and not pretty-fast?  Seems that pretty-fast would
> > match the pretty printed source from the debugger (for Bug 736078)
> 
> It supports some ES6 features, where as pretty-fast doesn't yet. Not sure
> what that bug has to do with this, though.

JSBeautify does not need valid JS. This means that it will work with half written code and this is necessary in tools like scratchpad.

(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> Joe asked on IRC if we could use this in the scratchpad/debugger.
> 
> Does it create source maps now? (I don't believe it did before). If not,
> then we could use it for scratchpad, but not debugger.

My use case is for visual events where we slice an event handler out of a JS file. These slices of code are not valid JS so need JSBeautify for tidying e.g.
"foo: function() { ... }"

> It should also
> respect "devtools.editor.tabsize" as far as indentation level goes, like the
> current scratchpad pretty print does.

Or the preference of whichever tool is making use of it. There are lots of options that can be passed in, indent_size being one of them.

(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> Comment on attachment 8449506 [details] [diff] [review]
> update-jsbeautify-1033387.patch
> 
> Review of attachment 8449506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would put it in a directory and import the 3rd party tests as well, just
> like we do with acorn, code mirror, pretty-fast, source-map, etc:
> 
> devtools/
>   toolkit/
>     jsbeautify/
>       jsbeautify.js
>       tests/
>         ...
> 

Done

> Probably need build team review as well.
> 

Done

> ::: toolkit/devtools/gcli/commands/jsb.js
> @@ +8,5 @@
> >  const gcli = require("gcli/index");
> >  const XMLHttpRequest = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"];
> >  
> >  loader.lazyImporter(this, "Preferences", "resource://gre/modules/Preferences.jsm");
> > +const {js_beautify} = require("devtools/toolkit/jsbeautify");
> 
> Can use this:
> 
>   devtools.lazyRequireGetter(this, "jsBeautify",
> "devtools/toolkit/jsbeautify");

Now using a lazyRequireGetter.
Attachment #8449506 - Attachment is obsolete: true
Attachment #8452968 - Flags: review?(nfitzgerald)
Attachment #8452968 - Flags: review?(gps)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #13)
> The patch looks bigger than it needs to... the larger files can be safely
> ignored.

I should rephrase that... the patch is not as big as it looks as the reviewer can ignore all of the larger files.
Comment on attachment 8452968 [details] [diff] [review]
update-jsbeautify-1033387.patch

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

So much "var" usage. Thanks for reminding me how ugly JS outside of Firefox is :)

Just a nit on the build bits. You can resubmit for review if you want. If not, I trust you.

::: toolkit/devtools/jsbeautify/lib/moz.build
@@ +9,5 @@
> +EXTRA_JS_MODULES += [
> +    'beautify-tests.js',
> +    'sanitytest.js',
> +    'urlencode_unpacker.js'
> +]

You don't need this moz.build. Please roll files into parent moz.build. Files should get installed without extra directory names in them.

::: toolkit/devtools/jsbeautify/src/moz.build
@@ +9,5 @@
> +EXTRA_JS_MODULES += [
> +    'beautify-css.js',
> +    'beautify-html.js',
> +    'beautify-js.js'
> +]

You don't need this moz.build. Please roll files into the parent moz.build.

::: toolkit/devtools/jsbeautify/tests/unit/test.js
@@ +5,5 @@
> +/*
> + * Copyright 2013 Mozilla Foundation and contributors
> + * Licensed under the New BSD license. See LICENSE.md or:
> + * http://opensource.org/licenses/BSD-2-Clause
> + */

Why are we using BSD license here? Shouldn't test files be under Public Domain? https://www.mozilla.org/MPL/license-policy.html

Is this due to using code inside JS Beautify?
Attachment #8452968 - Flags: review?(gps) → feedback+
Comment on attachment 8452968 [details] [diff] [review]
update-jsbeautify-1033387.patch

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

Looks good!
Attachment #8452968 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8455056 [details] [diff] [review]
update-jsbeautify-1033387.patch

Backed out due to xpctest failures
Attachment #8455056 - Flags: checkin+
Comment on attachment 8455056 [details] [diff] [review]
update-jsbeautify-1033387.patch

Backed out due to loader test failures:
https://hg.mozilla.org/integration/fx-team/rev/b257f50981e7
Attachment #8455056 - Flags: checkin+
Both the xpcshell and loader failures were completely unexpected... need a full try run for this now:
https://tbpl.mozilla.org/?tree=Try&rev=ef94bb0a25d8
Attachment #8455056 - Attachment is obsolete: true
Attachment #8455204 - Flags: review+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/20516685537a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.