Last Comment Bug 769560 - GCLI should include jsbeautifier in the set of tools it provides to Firefox
: GCLI should include jsbeautifier in the set of tools it provides to Firefox
Status: RESOLVED FIXED
[gclicommands]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 16
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on:
Blocks: 774603 GCLICMD 774238
  Show dependency treegraph
 
Reported: 2012-06-29 00:44 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-07-17 02:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch including tests v1 (53.63 KB, patch)
2012-07-12 03:25 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Patch WIP (54.89 KB, patch)
2012-07-12 06:33 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Patch 1 with broken autocomplete (54.89 KB, patch)
2012-07-13 08:18 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Patch 1 without option group (54.23 KB, patch)
2012-07-13 08:29 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: review+
Details | Diff | Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-29 00:44:55 PDT
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
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-29 00:46:12 PDT
It looks to me as though all the code here is either MIT or Mozilla, mhanson - please shout if this isn't right.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-29 00:46:43 PDT
Current best source: https://github.com/michaelrhanson/mozcmd/blob/master/jsb.mozcmd
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2012-06-29 03:10:15 PDT
Filter on teabags
Comment 4 Michael Hanson 2012-06-29 11:53:38 PDT
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.
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-29 12:48:17 PDT
(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
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-12 03:25:45 PDT
Created attachment 641417 [details] [diff] [review]
Patch including tests v1
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-12 03:51:52 PDT
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"
Comment 8 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-12 06:33:58 PDT
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.
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-12 06:36:13 PDT
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
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-13 08:18:07 PDT
Created attachment 641907 [details] [diff] [review]
Patch 1 with broken autocomplete

To be landed if we don't have time to fix autocomplete
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-13 08:29:06 PDT
Created attachment 641912 [details] [diff] [review]
Patch 1 without option group

We should land this if we can't fix autocomplete
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-14 02:38:25 PDT
http://tbpl.mozilla.org/?tree=Try&rev=9fb63e1299fc
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-14 15:19:18 PDT
https://tbpl.mozilla.org/?tree=Fx-Team&rev=6e2e2f904237
Comment 14 Ed Morley [:emorley] 2012-07-16 05:26:55 PDT
https://hg.mozilla.org/mozilla-central/rev/6e2e2f904237
Comment 15 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-16 05:31:41 PDT
Triage, filter on TEABAGS
Comment 16 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2012-07-16 22:22:20 PDT
> 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.
Comment 17 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-17 02:08:32 PDT
(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.
Comment 18 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2012-07-17 02:14:38 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.