Closed Bug 1073956 Opened 10 years ago Closed 10 years ago

octal literals and octal escape sequences are deprecated: resource://gre/modules/DownloadPaths.jsm

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: ishikawa, Assigned: ishikawa)

References

Details

Attachments

(1 file, 1 obsolete file)

During testing of DEBUG version of TB using "make mozmill" test suite
run, I noticed that the following message for the file:
comm-central/mozilla/toolkit/mozapps/downloads/DownloadPaths.jsm

JavaScript strict warning: resource://gre/modules/DownloadPaths.jsm, line 61: SyntaxError: octal literals and octal escape sequences are deprecated

A patch to replace an octet literal with a call to ParseInt(..., 8)
is attached.
Blocks: 826732
Comment on attachment 8496557 [details] [diff] [review]
Replaced a literal octal string with a ParseInt() call.

I won't mind creating a new patch if 0o prefix notation is preferable these days.

TIA
Attachment #8496557 - Flags: review?(bwinton)
See Also: → 1073955
Comment on attachment 8496557 [details] [diff] [review]
Replaced a literal octal string with a ParseInt() call.

Bug 931267 seems to indicate that we want 0o notation, so I would suggest using that.  Also, since this is toolkit code not Thunderbird code, I'm redirecting the review request to bbondy, who reviewed a similar patch over there.  ;)
Attachment #8496557 - Flags: review?(bwinton) → review?(netzen)
Comment on attachment 8496557 [details] [diff] [review]
Replaced a literal octal string with a ParseInt() call.

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

Thanks for the patch!

Yep please change to 0o, I think that is the preferred way.
I think originally octal literals were deprecated because it makes it hard to read and has unexpected consequences for new readers of the code.

Note that the patch as is uses: ParseInt("0644", 8) which would fail. I think you meant parseInt with a lowercase p.
Attachment #8496557 - Flags: review?(netzen) → review-
(I have four similar bugzilla entries/patches. Here is a template
response.)

Thank you for the review.

I am uploading a new patch with that uses "0o" prefix for
octal literal. It seems "0o..." notation is preferred over
parseInt(..., 8).

Regarding the misspelling of "ParseInt" for "parseint",
I thought initially people were referring to the misspelling
in the description field of my patch.

I said "Whoa!" when I realized that the code itself had this misspelling(!)

SURPRISE: I had applied the patch to my source tree, and had run the
|make mozmill| test, and JS and mozmill test frame never printed a
single complaint about this (!!!)

That is why I did not realize the misspelling in the code portion of
the patch.

I carefully checked the log files and found no mention of ParseInt at
all. Also, the number of failed errors were not that much
different before and after the patch.
(Because of timing-sensitive errors that happen during shutdown (bugs), etc.,
the number of failed errors may differ from one run to the other. I
did not see any major errors possibly due to the "ParseInt"
misspellings.)
So, I think JS interpreter is doing something without aborting the 
execution.

What gives? I think this merits a new bugzilla entry...

Anyway, the simple "Use 0o prefix for octal literal" patch is
uploaded.

TIA
Attachment #8496557 - Attachment is obsolete: true
Attachment #8497247 - Flags: review?(netzen)
Comment on attachment 8497247 [details] [diff] [review]
Use 0o prefix for octal literal.

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

Great, thank you!

> had applied the patch to my source tree, and had run the
> |make mozmill| test, and JS and mozmill test frame never printed a
> single complaint about this (!!!)

Perhaps the change wasn't built from the source or else maybe there's just not coverage hitting that part of the code. 
Thanks again for the patch!
Attachment #8497247 - Flags: review?(netzen) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #5)

> Perhaps the change wasn't built from the source or else maybe there's just
> not coverage hitting that part of the code. 
> Thanks again for the patch!

Oh, I see. 0666 or whatever caused the parser of JavaScript to complain about
deprecated octal literal on the first reading, but
ParseInt (that begins with capital P) did not get called since the
code in question was never executed.
Makes sense. For a moment, I forgot that JavaScript is a dynamically typed language.
(I don't think coding big program modules using a dynamically typed language is 
a good idea for long-term maintenance, but I digress.) 

I will put in checkin_needed keyword.

TIA.
Keywords: checkin-needed
Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0b3b8c18424
Status: NEW → ASSIGNED
Component: Untriaged → Download Manager
Keywords: checkin-needed
Product: Thunderbird → Toolkit
Target Milestone: --- → mozilla35
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/e0b3b8c18424
Assignee: nobody → ishikawa
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.