Last Comment Bug 487780 - JavaScript Debugger Venkman does not support language features of JS 1.7 in console
: JavaScript Debugger Venkman does not support language features of JS 1.7 in c...
Status: RESOLVED FIXED
:
Product: Other Applications
Classification: Client Software
Component: Venkman JS Debugger (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Georg Maaß
:
Mentors:
Depends on:
Blocks: 273073
  Show dependency treegraph
 
Reported: 2009-04-10 05:53 PDT by Georg Maaß
Modified: 2009-12-01 19:03 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch to enable js 1.7 in console of venkman, if client supports it (55.15 KB, application/zip)
2009-04-10 06:09 PDT, Georg Maaß
no flags Details
diff file containing the patch (35.52 KB, patch)
2009-04-10 23:52 PDT, Georg Maaß
no flags Details | Diff | Splinter Review
updated patch (35.52 KB, patch)
2009-04-12 10:20 PDT, Georg Maaß
gijskruitbosch+bugs: review-
Details | Diff | Splinter Review
patch to venkman-scripts.xul only (1.18 KB, patch)
2009-04-15 23:45 PDT, Georg Maaß
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
Patch fixing the problem with XUL cache (858 bytes, patch)
2009-06-11 10:42 PDT, Georg Maaß
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review

Description Georg Maaß 2009-04-10 05:53:56 PDT
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ß 2009-04-10 06:09:50 PDT
Created attachment 372046 [details]
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] 2009-04-10 07:15:36 PDT
The patch is absolutely unreadable.  How did you generate it?
Comment 3 Georg Maaß 2009-04-10 09:15:12 PDT
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ß 2009-04-10 09:27:09 PDT
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 2009-04-10 14:09:36 PDT
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ß 2009-04-10 23:52:24 PDT
Created attachment 372193 [details] [diff] [review]
diff file containing the patch
Comment 7 Georg Maaß 2009-04-11 03:42:46 PDT
If this is now assigned to me, what are the next steps to do for me?
Comment 8 Karsten Düsterloh 2009-04-11 03:55:46 PDT
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 2009-04-11 16:59:46 PDT
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ß 2009-04-12 10:20:04 PDT
Created attachment 372312 [details] [diff] [review]
updated patch

changed mime type to application/javascript instead of old application/x-javascript as told by comment 9.
Comment 11 Alex Vincent [:WeirdAl] 2009-04-12 15:24:35 PDT
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ß 2009-04-13 00:15:39 PDT
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 Kruitbosch (Gone July 28 - Aug 11) 2009-04-15 03:21:22 PDT
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ß 2009-04-15 23:45:33 PDT
Created attachment 373062 [details] [diff] [review]
patch to venkman-scripts.xul only

This patch does not add any missing semicolons.
Comment 15 :Gijs Kruitbosch (Gone July 28 - Aug 11) 2009-05-02 05:46:47 PDT
Comment on attachment 373062 [details] [diff] [review]
patch to venkman-scripts.xul only

r=me
Comment 16 Karsten Düsterloh 2009-05-20 15:24:49 PDT
Pushed as <http://hg.mozilla.org/venkman/rev/f27ea2ff7bf3>.
Comment 17 Georg Maaß 2009-06-11 09:49:28 PDT
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ß 2009-06-11 10:42:59 PDT
Created attachment 382766 [details] [diff] [review]
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 2009-06-11 11:29:55 PDT
(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 Kruitbosch (Gone July 28 - Aug 11) 2009-06-15 07:33:59 PDT
(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 2009-06-16 16:00:44 PDT
(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 Kruitbosch (Gone July 28 - Aug 11) 2009-06-16 16:11:32 PDT
(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 Kruitbosch (Gone July 28 - Aug 11) 2009-06-16 16:11:58 PDT
Comment on attachment 382766 [details] [diff] [review]
Patch fixing the problem with XUL cache

r=me
Comment 24 :Gijs Kruitbosch (Gone July 28 - Aug 11) 2009-12-01 09:03:18 PST
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) 2009-12-01 19:03:35 PST
(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>"...

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