487780 - JavaScript Debugger Venkman does not support language features of JS 1.7 in console

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Georg Maaß]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090410 SeaMonkey/2.0b1pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090410 SeaMonkey/2.0b1pre

The file venkman-eval.js is included from venkman-scripts.xul using a simple script tag without type attribute. This causes the language extensions of JavaScript 1.7 like "let" and the destructuring assignments not being enabled.

Reproducible: Always

Steps to Reproduce:
type "let a;" into the console, hit Return and get this error message:

SyntaxError: missing ; before statement @ <x-jsd:interactive-session> 1

corrct result would be undefined instead of an error message.

Comment 1

[Georg Maaß]

Attachment: patch to enable js 1.7 in console of venkman, if client supports it

The patch contains a patch version of venkman-scripts.xul, where the file venkman-eval.js is included twice. The first include is done with the original tag for backward compatibility to old browsers not yet supporting JavaScript 1.7.
 The second include is identical but now with type attribute set to JavaScript 1.7 to enable the language features.

The other modified files are command-manager-js, file.utils.js menu-manager.js, pref-manager.js, tree-utils.js, venkman-commands.js, venkman-debugger.js, venkman-eval.js. In these files I've only added missing semicolons; no other changes are made.

The source I've pulled from hg this morning, so it is up to date.

Comment 2

[Alex Vincent [:WeirdAl]]

The patch is absolutely unreadable.  How did you generate it?

Comment 3

[Georg Maaß]

It is a zip file. Don't know, why it assumes it being plain text. Work around: save link target as patch.zip

Comment 4

[Georg Maaß]

I've tested this in a XulRunner application from trunk (1.9.2.x), where it works: without the patch let and destructuring assignments throw errors when used in the console of venkman, with patch they work as desired.

But when I patch this into my SeaMonkey build just built from hg (1.9.1.x) few minutes ago, it does not work. It behaves as unpatched. Does this depend on any changes made in 1.9.2.x to have an effect?

Comment 5

[Karsten Düsterloh]

Comment on attachment 372046 [details]
patch to enable js 1.7 in console of venkman, if client supports it

This is not a patch. Like you said, it's a zip file, but it does not contain a patch, just a bundle of changed(?) files...

Since you are already using hg to pull from the Venkman, use the following steps to create a patch:
- do your changes
- use a shell/commandline
- enter Venkman's main directory in your source (c-c/mozilla/extensions/venkman/ or something like that)
- there, use 
    hg diff > 487780.1.diff
  to create the patch text file 487780.1.diff
- attach this text file here
- request review from a person (just review? is useless)

Comment 6

[Georg Maaß]

Attachment: diff file containing the patch

Comment 7

[Georg Maaß]

If this is now assigned to me, what are the next steps to do for me?

Comment 8

[Karsten Düsterloh]

Wait for reviews, (apply eventual review comments, wait for new reviews)*, nudge someone on IRC to check in/set the checkin-needed keyword.

Comment 9

[Karsten Düsterloh]

Comment on attachment 372193 [details] [diff] [review]
diff file containing the patch

>+    <script src="chrome://venkman/content/venkman-eval.js" type="application/x-javascript; version=1.7"/>

Btw1: As per RfC 4329, the type should be application/javascript. (See also bug 381467.)
Btw2: The patch here fixes the (unfiled?) bug that you can't watch variables defined with let!

Comment 10

[Georg Maaß]

Attachment: updated patch

changed mime type to application/javascript instead of old application/x-javascript as told by comment 9.

Comment 11

[Alex Vincent [:WeirdAl]]

Why does this patch include so many lines where all you do is add semicolons?  The meat of this patch seems to be only at the very end.

Don't get me wrong, it's not bad style.  It just sounds unnecessary for the intent of this patch.

Comment 12

[Georg Maaß]

The semicolons are not necessary for the patch working; this is true. The diff command recorded more than necessary.

Adding the missing semicolons does not affect this issue. I'm working on them too because they may cause strict warnings and even more worse when tools compress the source to a single line missing semicolons result in syntax errors. So single line compression is one step of automatized quality checks of my Mozilla based tools. This is the reason, why I started to add the missing semicolons in my hg checkout. They are not relevant for the issue of this bug.

Comment 13

[:Gijs (he/him)]

Comment on attachment 372312 [details] [diff] [review]
updated patch

I would prefer not to have all the semicolons after function declarations, and right now I don't have the time to go through this big a patch and filter it down to the actual functional changes. Could you please do so and submit only the actual functional changes, rather than the cosmetic ones?

Additionally, Venkman has its own hg repo at http://hg.mozilla.org/venkman. Feel free to use that one for development work. Thanks for working on this issue!

Comment 14

[Georg Maaß]

Attachment: patch to venkman-scripts.xul only

This patch does not add any missing semicolons.

Comment 15

[:Gijs (he/him)]

Comment on attachment 373062 [details] [diff] [review]
patch to venkman-scripts.xul only

r=me

Comment 16

[Karsten Düsterloh]

Pushed as <http://hg.mozilla.org/venkman/rev/f27ea2ff7bf3>.

Comment 17

[Georg Maaß]

After a long discussion in chatzilla we detected the patch working only, when preference "nglayout.debug.disable_xul_cache" is set to true. This occurs because of two consecutive script tags including both the same file but with different version attribute for not breaking backward compatibility in case of building it as XPI.

So the patch should be optimized by using only a single script tag with the necessary version specified to circumvent this XUL cache problem.

Comment 18

[Georg Maaß]

Attachment: Patch fixing the problem with XUL cache

This patch is incompatible to browsers not supporting JavaScript 1.7!

An additional patch should add a version check into the install.js of the XPI part if we want to prevent Gecko prior 1.9 from destroying an existing Venkman by an incompatible update. The install.rdf already has version numbers inside, may be some of them need to be increased too, when adding this patch, because this patch now makes JS 1.7 being required, because it no longer loads the eval.js script without version specification.

Comment 19

[Karsten Düsterloh]

(In reply to comment #17)
> After a long discussion in chatzilla we detected the patch working only, when
> preference "nglayout.debug.disable_xul_cache" is set to true.

To be a bit more precise:
For clients capabale of using JS 1.7 and with XUL caching on, the first script tag is cached and used - but this one has no version=1.7, hence this bug is not fixed. Turning XUL caching off, the last script tag is used and this bug seems to be fixed.
Changing the order of the script tags doesn't help, it just changes the condition of when breakage happens.

Comment 20

[:Gijs (he/him)]

(In reply to comment #18)
> Created an attachment (id=382766) [details]
> Patch fixing the problem with XUL cache
> 
> This patch is incompatible to browsers not supporting JavaScript 1.7!

Does that include SeaMonkey 1.1/1.5/whatever-the-latest-stable-is?

Comment 21

[Karsten Düsterloh]

(In reply to comment #20)
> > This patch is incompatible to browsers not supporting JavaScript 1.7!
> Does that include SeaMonkey 1.1/1.5/whatever-the-latest-stable-is?

The SeaMonkey 1.0.x series only supports JavaScript 1.6.
The SeaMonkey 1.1.x series supports JavaScript 1.7.
The SeaMonkey 2.x series as of now supports at least JavaScript 1.8.

Comment 22

[:Gijs (he/him)]

(In reply to comment #21)
> (In reply to comment #20)
> > > This patch is incompatible to browsers not supporting JavaScript 1.7!
> > Does that include SeaMonkey 1.1/1.5/whatever-the-latest-stable-is?
> 
> The SeaMonkey 1.0.x series only supports JavaScript 1.6.
> The SeaMonkey 1.1.x series supports JavaScript 1.7.
> The SeaMonkey 2.x series as of now supports at least JavaScript 1.8.

Right, but there are no releases in the 2.x series, hence why I asked about 1.1 and co. Anyway, that's fine then.

Comment 23

[:Gijs (he/him)]

Comment on attachment 382766 [details] [diff] [review]
Patch fixing the problem with XUL cache

r=me

Comment 24

[:Gijs (he/him)]

Sorry this took so long, it got lost in my "to do" queue, which is a bit too long at the moment. Fixed now:

http://hg.mozilla.org/venkman/rev/aa23cb11abc4

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > > This patch is incompatible to browsers not supporting JavaScript 1.7!
> > 
> > The SeaMonkey 1.1.x series supports JavaScript 1.7.
> > The SeaMonkey 2.x series as of now supports at least JavaScript 1.8.
> 
> Right, but there are no releases in the 2.x series, hence why I asked about 1.1
> and co. Anyway, that's fine then.

Ftr, install.rdf is currently targeted at "SeaMonkey version 2.0 to 2.1", which is fine wrt this bug.
Fwiw, you may still want to check/update the 5 other "<em:targetApplication>"...