785134 - Update jsb to fix various bugs

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

There are a number of serious jsb bugs that have been fixed recently. We need to update and optimize it for fx.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Updated jsb and worked around a btoa encoding issue

Panos implied that he was waiting for this the other day.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 662186 [details] [diff] [review]
Updated jsb and worked around a btoa encoding issue

Actually, Joe is probably better reviewing this.

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 662186 [details] [diff] [review]
Updated jsb and worked around a btoa encoding issue

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

Did we change line endings on Jsbeautify.jsm? The diff seems to think that everything has changed.

::: browser/devtools/commandline/CmdJsb.jsm
@@ +112,5 @@
>      } catch(e) {
>        return gcli.lookup('jsbInvalidURL');
>      }
>  
>      let promise = context.createPromise();

That's sooooo 2012. The NEW IMPROVED way is:

var deferred = context.defer();
setTimeout(function() {
  deferred.resolve("hello");
}, 500);
return deferred.promise;

This is the more normal way to work with jetpack/Q style promises. The old way still works, but is deprecated.

@@ +122,5 @@
>            let browserWindow = browserDoc.defaultView;
> +          let gBrowser = browserWindow.gBrowser;
> +          let result = js_beautify(xhr.responseText, opts);
> +
> +          // TODO: btoa currently only works with ISO-8859-1

Nit, since we're changing it for the above - I think we're supposed to use FIXME rather than TODO in moz code.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +811,5 @@
>  
> +# the 'jsb <doNotPreserveNewlines>' parameter. This string is designed to be
> +# shown in a menu alongside the command name, which is why it should be as short
> +# as possible.
> +jsbDoNotPreserveNewlinesDesc=Do not preserve line breaks.

Please could you remove the '.', here and in the other descriptions.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Joe Walker [:jwalker] from comment #3)
> ::: browser/devtools/commandline/CmdJsb.jsm
> @@ +112,5 @@
> >      } catch(e) {
> >        return gcli.lookup('jsbInvalidURL');
> >      }
> >  
> >      let promise = context.createPromise();
> 
> That's sooooo 2012. The NEW IMPROVED way is:
> 
> var deferred = context.defer();
> setTimeout(function() {
>   deferred.resolve("hello");
> }, 500);
> return deferred.promise;
> 

I have logged bug 792815 to update how we use promise across our commands.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed reviewers comments

(In reply to Joe Walker [:jwalker] from comment #3)
> Comment on attachment 662186 [details] [diff] [review]
> Updated jsb and worked around a btoa encoding issue
> 
> Review of attachment 662186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did we change line endings on Jsbeautify.jsm? The diff seems to think that
> everything has changed.
> 
> ::: browser/devtools/commandline/CmdJsb.jsm
> @@ +112,5 @@
> >      } catch(e) {
> >        return gcli.lookup('jsbInvalidURL');
> >      }
> >  
> >      let promise = context.createPromise();
> 
> That's sooooo 2012. The NEW IMPROVED way is:
> 
> var deferred = context.defer();
> setTimeout(function() {
>   deferred.resolve("hello");
> }, 500);
> return deferred.promise;
> 
> This is the more normal way to work with jetpack/Q style promises. The old
> way still works, but is deprecated.
> 

This has been moved out into another bug.

> @@ +122,5 @@
> >            let browserWindow = browserDoc.defaultView;
> > +          let gBrowser = browserWindow.gBrowser;
> > +          let result = js_beautify(xhr.responseText, opts);
> > +
> > +          // TODO: btoa currently only works with ISO-8859-1
> 
> Nit, since we're changing it for the above - I think we're supposed to use
> FIXME rather than TODO in moz code.
> 

Changed

> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +811,5 @@
> >  
> > +# the 'jsb <doNotPreserveNewlines>' parameter. This string is designed to be
> > +# shown in a menu alongside the command name, which is why it should be as short
> > +# as possible.
> > +jsbDoNotPreserveNewlinesDesc=Do not preserve line breaks.
> 
> Please could you remove the '.', here and in the other descriptions.

Done

Comment 6

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/48cfe823a5da

Comment 7

[Paul Rouget [:paul]]

Backed out: https://tbpl.mozilla.org/?tree=Fx-Team&rev=4b211b444e7f

Oranges: https://tbpl.mozilla.org/?tree=Fx-Team&rev=48cfe823a5da

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This is caused by Bug 213047. It is best for us to open the result in scratchpad anyway as we don't need to worry about character encoding or base 64 encoding issues.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Now outputs prettified source to scratchpad

Comment 10

[Joe Walker [:jwalker]]

Comment on attachment 665910 [details] [diff] [review]
Now outputs prettified source to scratchpad

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

This is something of a 'while we're at it', but I think it's important ...

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +847,2 @@
>  # possible.
>  jsbBraceStyleDesc=Collapse, expand, end-expand, expand-strict

The user can see the options from the menus, so I think we should be saying what this is for rather than listing the options.

@@ +849,5 @@
>  
>  # LOCALIZATION NOTE (jsbBraceStyleManual) A fuller description of the
>  # 'jsb <braceStyle>' parameter, displayed when the user asks for help
>  # on what it does.
>  jsbBraceStyleManual=The coding style of braces. Either collapse, expand, end-expand or expand-strict

I think we really should describe the options here. We can use basic xhtml, so <br/> can be used to give examples. This is one place where help could be really useful.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed reviewers comments & fixed copy property

This pretty much does everything that we need in order to fix the copy paste issues.

Green on try.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 666580 [details]
Addressed reviewers comments & fixed copy property

Wrong comments

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Now opens in scratchpad and has better help

This works fine and is green on try.

Due to the oversized panel that this creates we are pending bug 786803.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Added missing css

Comment 16

[Joe Walker [:jwalker]]

Comment on attachment 666985 [details] [diff] [review]
Added missing css

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

::: browser/themes/gnomestripe/devtools/commandline.css
@@ +72,5 @@
> +  font-size: 80%;
> +}
> +
> +.gcli-row-out .nowrap {
> +  white-space: nowrap;

I think this should probably be content CSS.
What do you think?
https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Moved content css to content

(In reply to Joe Walker [:jwalker] from comment #16)
> Comment on attachment 666985 [details] [diff] [review]
> Added missing css
> 
> Review of attachment 666985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/gnomestripe/devtools/commandline.css
> @@ +72,5 @@
> > +  font-size: 80%;
> > +}
> > +
> > +.gcli-row-out .nowrap {
> > +  white-space: nowrap;
> 
> I think this should probably be content CSS.
> What do you think?
> https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS

Of course it should ... done.

Comment 18

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/f3af7601c307

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/f3af7601c307

Comment 20

[Francesco Lodolo [:flod]]

As usual, you can't change strings like this (jsbBraceStyleDesc, jsbBraceStyleManual) without changing the entity name.

And to be honest, I don't think all that html in one string makes much sense

jsbBraceStyleManual=<p class="nowrap">The coding style of braces. Select from one of the following:</p><ul><li>collapse<br/><pre>if (x == 1) {\n  ...\n} else {\n  ...\n}</pre></li><li>expand<br/><pre>if (x == 1)\n{\n  ...\n}\nelse\n{\n  ...\n}</pre></li><li>end-expand<br/><pre>if (x == 1) {\n  ...\n}\nelse {\n  ...\n}</pre></li><li>expand-strict<br/><pre>if (x == 1)\n{\n  return // This option can break scripts\n  {\n    a: 1\n  };\n} else {\n  ...\n}</pre></li></ul>

This landed late in the cycle, so this is already on aurora which is supposed to be string frozen.

Comment 21

[Joe Walker [:jwalker]]

(In reply to Francesco Lodolo [:flod] from comment #20)
> As usual, you can't change strings like this (jsbBraceStyleDesc,
> jsbBraceStyleManual) without changing the entity name.

Yes. I think we should probably (fairly quickly) revert to the old string, and then fix it properly as a followup.

What do you think Mike?

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I do agree, we can probably find another way to do this?

Comment 23

[Joe Walker [:jwalker]]

I opened bug 799462 and bug 799463.
Mike could you do bug 799462 fairly soon, and then bug 799463 as a lower priority (depending on how broken things look with the original strings?)