Update jsb to fix various bugs

RESOLVED FIXED in Firefox 18

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: miker, Assigned: miker)

Tracking

Trunk
Firefox 18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

There are a number of serious jsb bugs that have been fixed recently. We need to update and optimize it for fx.
Created attachment 662186 [details] [diff] [review]
Updated jsb and worked around a btoa encoding issue

Panos implied that he was waiting for this the other day.
Attachment #662186 - Flags: review?(past)
Comment on attachment 662186 [details] [diff] [review]
Updated jsb and worked around a btoa encoding issue

Actually, Joe is probably better reviewing this.
Attachment #662186 - Flags: review?(past) → review?(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.
Attachment #662186 - Flags: review?(jwalker) → review+
(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.
Created attachment 664044 [details] [diff] [review]
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
Attachment #662186 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]

Comment 6

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/48cfe823a5da
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]

Comment 7

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

Oranges: https://tbpl.mozilla.org/?tree=Fx-Team&rev=48cfe823a5da
Whiteboard: [fixed-in-fx-team]
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.
Created attachment 665910 [details] [diff] [review]
Now outputs prettified source to scratchpad
Attachment #664044 - Attachment is obsolete: true
Attachment #665910 - Flags: review?(dcamp)
Attachment #665910 - Flags: review?(dcamp) → review?(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.
Attachment #665910 - Flags: review?(jwalker)
Duplicate of this bug: 774238
Created attachment 666580 [details]
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.
Attachment #665910 - Attachment is obsolete: true
Attachment #666580 - Flags: review?(dcamp)
Comment on attachment 666580 [details]
Addressed reviewers comments & fixed copy property

Wrong comments
Attachment #666580 - Flags: review?(dcamp)
Created attachment 666949 [details] [diff] [review]
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.
Attachment #666580 - Attachment is obsolete: true
Attachment #666949 - Flags: review?(jwalker)
Depends on: 786803
Created attachment 666985 [details] [diff] [review]
Added missing css
Attachment #666949 - Attachment is obsolete: true
Attachment #666949 - Flags: review?(jwalker)
Attachment #666985 - Flags: review?(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
Attachment #666985 - Flags: review?(jwalker)
Created attachment 667563 [details] [diff] [review]
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.
Attachment #666985 - Attachment is obsolete: true
Attachment #667563 - Flags: review?(jwalker)
Attachment #667563 - Flags: review?(jwalker) → review+
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/f3af7601c307
https://hg.mozilla.org/mozilla-central/rev/f3af7601c307
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
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.
(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?
I do agree, we can probably find another way to do this?
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?)
You need to log in before you can comment on or make changes to this bug.