The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

Other Applications
Venkman JS Debugger
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Georg Maaß, Assigned: Georg Maaß)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
Attachment #372046 - Flags: review?
The patch is absolutely unreadable.  How did you generate it?
(Assignee)

Comment 3

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

Comment 4

8 years ago
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?

Updated

8 years ago
Attachment #372046 - Attachment is patch: false
Attachment #372046 - Attachment mime type: text/plain → application/zip

Comment 5

8 years ago
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)
Attachment #372046 - Flags: review?
(Assignee)

Comment 6

8 years ago
Created attachment 372193 [details] [diff] [review]
diff file containing the patch
Attachment #372046 - Attachment is obsolete: true
Attachment #372193 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #372193 - Flags: review? → review?(gijskruitbosch+bugs)

Updated

8 years ago
Assignee: rginda → georg
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 7

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

Comment 8

8 years ago
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

8 years ago
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!
(Assignee)

Comment 10

8 years ago
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.
Attachment #372193 - Attachment is obsolete: true
Attachment #372193 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

8 years ago
Attachment #372312 - Flags: review?(gijskruitbosch+bugs)
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.
(Assignee)

Comment 12

8 years ago
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.

Updated

8 years ago
Attachment #372312 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 13

8 years ago
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!
(Assignee)

Comment 14

8 years ago
Created attachment 373062 [details] [diff] [review]
patch to venkman-scripts.xul only

This patch does not add any missing semicolons.
Attachment #372312 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #373062 - Flags: review?(gijskruitbosch+bugs)

Comment 15

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

r=me
Attachment #373062 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

8 years ago
Blocks: 273073

Comment 16

8 years ago
Pushed as <http://hg.mozilla.org/venkman/rev/f27ea2ff7bf3>.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

8 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

8 years ago
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.
Attachment #382766 - Flags: review?(gijskruitbosch+bugs)

Comment 19

8 years ago
(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

8 years ago
(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

8 years ago
(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

8 years ago
(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

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

r=me
Attachment #382766 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 24

7 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED
(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>"...
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.