Closed
Bug 1073912
Opened 10 years ago
Closed 10 years ago
octal literals and octal escape sequences are deprecated: components/XULStore.js
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: ishikawa, Assigned: ishikawa)
References
Details
Attachments
(1 file, 1 obsolete file)
972 bytes,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
During testing, I noticed that the following message was printed many times to the invoking console. JavaScript strict warning: file:///REF-OBJ-DIR/objdir-tb3/dist/bin/components/XULStore.js, line 147: SyntaxError: octal literals and octal escape sequences are deprecated Despite Bug 928725, I think 0o prefix still needs a time to be accepted widely obviously (from developers even by this bug, mind you), I suggest a patch to use ParseInt (..., 8) to shut up the warning completely. We need every trick to reduce the warning in the test output (noise and signal ratio is very bad.) If only many developers form a habit of checking the console log test suite runs with DEBUG version of binaries every month, and see if their new code or legacy code may have started to producing new warnings or not. TIA
Assignee | ||
Comment 1•10 years ago
|
||
Here is the patch. (In reply to ISHIKAWA, Chiaki from comment #0) > If only many developers form a habit of checking the console log test suite > runs with DEBUG version of binaries every month, > and see if their new code or legacy code may have started to producing new > warnings or not. This was meant NOT to this bugzilla entry alone, but to general mozilla developer community as a whole.
Comment 2•10 years ago
|
||
Comment on attachment 8496550 [details] [diff] [review] Replaced literal octal string(s) with ParseInt() call(s). Review of attachment 8496550 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/xulstore/XULStore.js @@ +143,5 @@ > }, > > readFile: function() { > const MODE_RDONLY = 0x01; > + const FILE_PERMS = ParseInt("0600", 8); I think you can use ES6 syntax 0o600 rather than parseInt.
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2) > > I think you can use ES6 syntax 0o600 rather than parseInt. Thank you for the comment. I was a little uncomfortable to use the 0o notation since ES6 is relatively new (?: I am not familiar with ES6 standard process), and if "0o" is the preferred way in the mozilla community today, I have no problem with it. Any thought about compatibility issues? (Not within the firefox/thunderbird internal usage only, but would it be OK to encourage the readers of the code to mimic the usage and still they don't run into compatibility issues? Now that I think about it, Google(chrome), Microsoft(IE6), etc. are behind ES6 efforts, maybe not much. ) I will create the modified patch with 0o prefix if there is an agreement on this. I am putting Gavin Sharp, one of the peers of Toolkit (XUL Toolkit) in http://www-archive.mozilla.org/owners.html as the reviewer who needs to ack the eventual patch. Let the peer(s) decide. For a discussion of 0o, maybe the almost one year old Bug 928725 (towards the end) might be interesting. I am afraid that we may need to educate people on the use of 0o if it is now supported sold and widely. TIA
Assignee: nobody → ishikawa
Assignee | ||
Updated•10 years ago
|
Attachment #8496550 -
Flags: review?(gavin.sharp)
Comment 4•10 years ago
|
||
Comment on attachment 8496550 [details] [diff] [review] Replaced literal octal string(s) with ParseInt() call(s). I think you mean "parseInt"... But I believe you can use 0o600 instead? I think that would be preferred. Thanks for the patch!
Attachment #8496550 -
Flags: review?(gavin.sharp) → review-
Comment 5•10 years ago
|
||
Oh, sorry, I missed the earlier comments. Yes, the 0o600 style is preferred. And by the way, Marco's also a toolkit peer! :) https://wiki.mozilla.org/Modules/Toolkit is the up-to-date page for that.
Assignee | ||
Comment 6•10 years ago
|
||
(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 #8497243 -
Flags: review?(gavin.sharp)
Updated•10 years ago
|
Attachment #8497243 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #5) > Oh, sorry, I missed the earlier comments. Yes, the 0o600 style is preferred. > And by the way, Marco's also a toolkit peer! :) > https://wiki.mozilla.org/Modules/Toolkit is the up-to-date page for that. Thank you for the review+. I will put in the checkin_needed keyword. I did not mean to slight Marco, sorry about my mixup. It is just that Google showed the outdated page BEFORE the up-to-date page when I searched for "mozilla module owner" :-( TIA
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Attachment #8496550 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/939cf8134947
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/939cf8134947
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•