GCLI should include jsbeautifier in the set of tools it provides to Firefox

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools: Console
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwalker, Assigned: miker)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gclicommands])

Attachments

(2 attachments, 2 obsolete attachments)

Tasks:
- Is this something that is better off in GCLI rather than Firefox
  - Advantage of GCLI - Users like Orion can make use of it there too
  - Disadvantage of GCLI - The need to abstract out the window open code
  - Disadvantage of GCLI - Parallel localization although this isn't really
    our /problem/, just something that's not ideal.
  I'm leaning that we should make it part of GCLI, but not today
- Review code
  - This is existing well used code so we mostly need to check for things
    that are insane
- Create tests
  - Is there an existing test suite?
- Localize
It looks to me as though all the code here is either MIT or Mozilla, mhanson - please shout if this isn't right.
Current best source: https://github.com/michaelrhanson/mozcmd/blob/master/jsb.mozcmd
Blocks: 768998
Filter on teabags
Whiteboard: [gclicommands]

Comment 4

5 years ago
Original source of JSBeautifier code is:
https://github.com/einars/js-beautify

MIT licensed; relevant lines are:
Written by Einar Lielmanis, <einar@jsbeautifier.org>
http://jsbeautifier.org/

Originally converted to javascript by Vital, <vital76@gmail.com>
"End braces on own line" added by Chris J. Shull, <chrisjshull@gmail.com>

You are free to use this in any way you want, in case you find this useful or working for you.
(In reply to Joe Walker from comment #0)
> Tasks:
> - Is this something that is better off in GCLI rather than Firefox
>   - Advantage of GCLI - Users like Orion can make use of it there too
>   - Disadvantage of GCLI - The need to abstract out the window open code
>   - Disadvantage of GCLI - Parallel localization although this isn't really
>     our /problem/, just something that's not ideal.
>   I'm leaning that we should make it part of GCLI, but not today

Just to clear this up - I think we should begin by using this in mozilla-central alone, and maybe sharing it further later.

> - Review code
>   - This is existing well used code so we mostly need to check for things
>     that are insane
> - Create tests
>   - Is there an existing test suite?
> - Localize
Created attachment 641417 [details] [diff] [review]
Patch including tests v1
Attachment #641417 - Flags: review?(jwalker)
Comment on attachment 641417 [details] [diff] [review]
Patch including tests v1

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

::: browser/devtools/commandline/GcliCommands.jsm
@@ +1222,5 @@
> +    name: 'url',
> +    type: 'string',
> +    description: gcli.lookup('jsbUrlDesc'),
> +    manual: 'The URL of the JS to prettify'
> +  },

We really need to make from here on down into options.

The syntax for this is a bit non-obvious. See GcliCookieCommands.jsm line 124. It basically means taking all these options and grouping them:

  params: [
    {
      name: 'url',
      type: 'string',
      description: gcli.lookup('jsbUrlDesc'),
      manual: 'The URL of the JS to prettify'
    },
    {
      group: gcli.lookup("jsbOptionsDesc"),
      params: [
        {
          name: 'indentSize',
          type: 'number',
          description: gcli.lookup('jsbIndentSizeDesc'),
          manual: gcli.lookup('jsbIndentSizeManual'),
          defaultValue: 2
        },
        // ... other params
      ]
    }

This will break some of the tests, but it will make the UI much better.

@@ +1252,5 @@
> +    name: 'preserveMaxNewlines',
> +    type: 'number',
> +    description: gcli.lookup('jsbPreserveMaxNewlinesDesc'),
> +    manual: gcli.lookup('jsbPreserveMaxNewlinesManual'),
> +    defaultValue: 1000

Could we use -1? 1000 seems arbitrary

@@ +1265,5 @@
> +  {
> +    name: 'braceStyle',
> +    type: {
> +      name: 'selection',
> +      data: ['collapse', 'expand', 'end-expand', 'expand-strict']

Note to self - It would be nice to be able to do:
  data: 'collapse, expand, end-expand, expand-strict'

Hmmmm

@@ +1292,5 @@
> +    indent_size: args.indentSize,
> +    indent_char: args.indentChar,
> +    preserve_newlines: args.preserveNewlines,
> +    preserve_max_newlines: args.preserveMaxNewlines,
> +    jslint_happy: args.jslineHappy,

jslint_happy: args.jslintHappy,

s/line/lint/ ?

@@ +1322,5 @@
> +          args.url + " " + xhr.status + " " + xhr.statusText);
> +      }
> +    };
> +  }
> +  xhr.send(null);

Asynchronous commands should create a promise and return it, and then resolve() so we know there is work in progress. There isn't a spinner today, but there will be soon.

This also enables is to promise.reject() rather than Cu.reportError() which no-one will ever spot.

::: browser/devtools/commandline/test/Makefile.in
@@ +35,5 @@
>    resources_inpage.js \
>    resources_inpage1.css \
>    resources_inpage2.css \
>    resources.html \
> +  browser_gcli_jsb_script.js \

Please could you put this in 'resources_jsb_script.js' so it's clear this isn't a test.
And while we're at it, please could you move 'resource.html' to the top of the list to keep them in alpha order.

::: browser/devtools/commandline/test/browser_gcli_jsb.js
@@ +1,5 @@
> +function test() {
> +  const TEST_URI = "http://example.com/browser/browser/devtools/commandline/" +
> +                   "test/browser_gcli_jsb_script.js";
> +
> +  DeveloperToolbarTest.test("about:blank", function GAT_test() {

Is GAT_test a cut and paste thing? Did you mean GJT_test or something? I'm not sure what the system is, but I think the idea behind the names is that their vaguely unique, and we use the same thing in the addons test

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +709,5 @@
> +
> +# LOCALIZATION NOTE (jsbIndentSizeManual) A fuller description of the
> +# 'jsb <indentSize>' parameter, displayed when the user asks for help on what it
> +# does.
> +jsbIndentSizeManual=Indentation size in characters

GCLI automatically uses the description if there is no manual, so there is no point in providing both with the same text.

I think we could s/characters/char in the descriptions, and perhaps make the manuals into sentances. e.g. "The indentation size in characters" or even "The extra amount nested lines are indented in characters"
Created attachment 641444 [details] [diff] [review]
Patch WIP

(In reply to Joe Walker from comment #7)
> Comment on attachment 641417 [details] [diff] [review]
> Patch including tests v1
> 
> Review of attachment 641417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/GcliCommands.jsm
> @@ +1222,5 @@
> > +    name: 'url',
> > +    type: 'string',
> > +    description: gcli.lookup('jsbUrlDesc'),
> > +    manual: 'The URL of the JS to prettify'
> > +  },
> 
> We really need to make from here on down into options.
> 
> The syntax for this is a bit non-obvious. See GcliCookieCommands.jsm line
> 124. It basically means taking all these options and grouping them:
> 
>   params: [
>     {
>       name: 'url',
>       type: 'string',
>       description: gcli.lookup('jsbUrlDesc'),
>       manual: 'The URL of the JS to prettify'
>     },
>     {
>       group: gcli.lookup("jsbOptionsDesc"),
>       params: [
>         {
>           name: 'indentSize',
>           type: 'number',
>           description: gcli.lookup('jsbIndentSizeDesc'),
>           manual: gcli.lookup('jsbIndentSizeManual'),
>           defaultValue: 2
>         },
>         // ... other params
>       ]
>     }
> 
> This will break some of the tests, but it will make the UI much better.
> 

Done

> @@ +1252,5 @@
> > +    name: 'preserveMaxNewlines',
> > +    type: 'number',
> > +    description: gcli.lookup('jsbPreserveMaxNewlinesDesc'),
> > +    manual: gcli.lookup('jsbPreserveMaxNewlinesManual'),
> > +    defaultValue: 1000
> 
> Could we use -1? 1000 seems arbitrary
> 

Done

> @@ +1265,5 @@
> > +  {
> > +    name: 'braceStyle',
> > +    type: {
> > +      name: 'selection',
> > +      data: ['collapse', 'expand', 'end-expand', 'expand-strict']
> 
> Note to self - It would be nice to be able to do:
>   data: 'collapse, expand, end-expand, expand-strict'
> 

Agreed

> Hmmmm
> 
> @@ +1292,5 @@
> > +    indent_size: args.indentSize,
> > +    indent_char: args.indentChar,
> > +    preserve_newlines: args.preserveNewlines,
> > +    preserve_max_newlines: args.preserveMaxNewlines,
> > +    jslint_happy: args.jslineHappy,
> 
> jslint_happy: args.jslintHappy,
> 
> s/line/lint/ ?
> 

No idea how that was missed, fixed.

> @@ +1322,5 @@
> > +          args.url + " " + xhr.status + " " + xhr.statusText);
> > +      }
> > +    };
> > +  }
> > +  xhr.send(null);
> 
> Asynchronous commands should create a promise and return it, and then
> resolve() so we know there is work in progress. There isn't a spinner today,
> but there will be soon.
> 
> This also enables is to promise.reject() rather than Cu.reportError() which
> no-one will ever spot.
> 

Makes sense, done.

> ::: browser/devtools/commandline/test/Makefile.in
> @@ +35,5 @@
> >    resources_inpage.js \
> >    resources_inpage1.css \
> >    resources_inpage2.css \
> >    resources.html \
> > +  browser_gcli_jsb_script.js \
> 
> Please could you put this in 'resources_jsb_script.js' so it's clear this
> isn't a test.
> And while we're at it, please could you move 'resource.html' to the top of
> the list to keep them in alpha order.
> 

Done

> ::: browser/devtools/commandline/test/browser_gcli_jsb.js
> @@ +1,5 @@
> > +function test() {
> > +  const TEST_URI = "http://example.com/browser/browser/devtools/commandline/" +
> > +                   "test/browser_gcli_jsb_script.js";
> > +
> > +  DeveloperToolbarTest.test("about:blank", function GAT_test() {
> 
> Is GAT_test a cut and paste thing? Did you mean GJT_test or something? I'm
> not sure what the system is, but I think the idea behind the names is that
> their vaguely unique, and we use the same thing in the addons test
> 

Copy paste error ... fixed.

> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +709,5 @@
> > +
> > +# LOCALIZATION NOTE (jsbIndentSizeManual) A fuller description of the
> > +# 'jsb <indentSize>' parameter, displayed when the user asks for help on what it
> > +# does.
> > +jsbIndentSizeManual=Indentation size in characters
> 
> GCLI automatically uses the description if there is no manual, so there is
> no point in providing both with the same text.
> 
> I think we could s/characters/char in the descriptions, and perhaps make the
> manuals into sentances. e.g. "The indentation size in characters" or even
> "The extra amount nested lines are indented in characters"

Reviewed and updated strings.
Attachment #641417 - Attachment is obsolete: true
Attachment #641417 - Flags: review?(jwalker)
Autocomplete has gone wild, type the following:
jsb http://code.jquery.com/jquery-1.7.2.min.js --jslintHappy

Press space and you get the completion for braceStyle ... add --braceStyle and choose collapse from the popup and it overwrites braceStyle
Depends on: 773565
Created attachment 641907 [details] [diff] [review]
Patch 1 with broken autocomplete

To be landed if we don't have time to fix autocomplete
Attachment #641444 - Attachment is obsolete: true
Created attachment 641912 [details] [diff] [review]
Patch 1 without option group

We should land this if we can't fix autocomplete
Attachment #641912 - Flags: review+
http://tbpl.mozilla.org/?tree=Try&rev=9fb63e1299fc
Whiteboard: [gclicommands] → [gclicommands][land-in-fx-team]
https://tbpl.mozilla.org/?tree=Fx-Team&rev=6e2e2f904237
Whiteboard: [gclicommands][land-in-fx-team] → [gclicommands][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6e2e2f904237
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [gclicommands][fixed-in-fx-team] → [gclicommands]
Blocks: 774238
Triage, filter on TEABAGS
No longer depends on: 773565
> The skipped statements are executed, but not stepped through
Could you explain the meaning of this string?

> jsbBraceStyleDesc=Collapse, expand, end-expand, expand-strict
Is this supposed to stay non localized? They look like parameter values to me

Side note: copy&paste of l10n comments (clearly wrong, e.g. "A very short string used to describe thefunction of the dbg step over command.") is kind of worse than having no comments at all.
(In reply to Francesco Lodolo [:flod] from comment #16)
> > The skipped statements are executed, but not stepped through
> Could you explain the meaning of this string?
> 

No idea, it is not part of this patch

> > jsbBraceStyleDesc=Collapse, expand, end-expand, expand-strict
> Is this supposed to stay non localized? They look like parameter values to me
> 

They should not be localised, the string should be removed.

> Side note: copy&paste of l10n comments (clearly wrong, e.g. "A very short
> string used to describe thefunction of the dbg step over command.") is kind
> of worse than having no comments at all.

The word dbg is not contained in this patch.

Bug 774603 has been created in relation to your comments.
Blocks: 774603
Sorry Michael, I guess I shouldn't be doing these thing at 7 in the morning.

Most of those comments were related to bug 733783, I'll report theme there.
You need to log in before you can comment on or make changes to this bug.