Closed Bug 1052734 Opened 10 years ago Closed 10 years ago

nsSearchService.js:38 - octal literals and octal escape sequences are deprecated

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 34

People

(Reporter: dholbert, Assigned: tomasz, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

Noticed these JS warnings at startup of my debug build, with a fresh profile:
{
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:38 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:38 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:38 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:39 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:39 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:39 - octal literals and octal escape sequences are deprecated
}

(Not sure why I get 3 copies of each warning, but I do.)

This is from these lines:
> const PERMS_FILE      = 0644;
> const PERMS_DIRECTORY = 0755;
which date back to bug 317107.

(And for reference, we disallowed octal literals in strict-mode JS in bug 514559.)
It looks like PERMS_DIRECTORY is completely unused in nsSearchService.js, so we can presumably just drop that line entirely.

For PERMS_FILE, we probably want something like this chunk from NativeApp.jsm:
> 32 // 0644
> 33 const PERMS_FILE = OS.Constants.libc.S_IRUSR | OS.Constants.libc.S_IWUSR |
> 34                    OS.Constants.libc.S_IRGRP |
> 35                    OS.Constants.libc.S_IROTH;

http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#32
Lines to change: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=30445fb9d44f#40
Mentor: MattN+bmo
OS: Linux → All
Hardware: x86_64 → All
(In reply to Daniel Holbert from comment #1)
> It looks like PERMS_DIRECTORY is completely unused in nsSearchService.js, so
> we can presumably just drop that line entirely.
> 
> For PERMS_FILE, we probably want something like this chunk from NativeApp.jsm:
> > 32 // 0644
> > 33 const PERMS_FILE = OS.Constants.libc.S_IRUSR | OS.Constants.libc.S_IWUSR |
> > 34                    OS.Constants.libc.S_IRGRP |
> > 35                    OS.Constants.libc.S_IROTH;
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#32

Does that work on Windows? Probably just easier to drop the "o" into the constant. (And change PERMS_FILE to 0o666 while we're about it; let the umask do its job.)
(I have no idea. But if it doesn't, NativeApp.jsm might need a tweak.)
I wanted to work on this bug as my first bug and here's what I found:

* indeed PERMS_DIRECTORY is not used so we can just drop it
* we have two methods to refer to octal 644:
  - http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/FileUtils.jsm#26
  - http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#33

For me 0o644 seems like the way to go.
Attached patch octal-patch.diff (obsolete) — Splinter Review
This is my first (omg 2 lines!) bug attempt so please be gentle.

I generated this patch from git (git patch-format) and I'm not 100% sure that it's in the right format so please have a careful look at it.
Attachment #8471926 - Flags: review?(MattN+bmo)
Assignee: nobody → tkolodziejski
Comment on attachment 8471926 [details] [diff] [review]
octal-patch.diff

I would prefer the NativeApp.jsm approach since it gets rid of the magic number.

I was in the middle of commenting when you were working on the patch.
Attachment #8471926 - Flags: review?(MattN+bmo)
Also, please include more lines of context in future patches. We recommend eight. Also see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#I%27m_all_used_to_git.2C_but_how_can_I_provide_Mercurial-ready_patches_.3F
Actually, we are already importing FileUtils.jsm so you can just update the const to reference FileUtils.PERMS_FILE.
Status: NEW → ASSIGNED
I followed your suggestions. And I converted the patch to hg style using moz-git-tools. Please have a look.
Attachment #8471926 - Attachment is obsolete: true
Attachment #8471991 - Flags: review?(MattN+bmo)
Attachment #8471991 - Flags: review?(MattN+bmo) → review+
Thanks! This is a pretty simple rename so I don't think a Try push is worth the resources.
QA Whiteboard: [qa-]
Keywords: checkin-needed
Note: Commit message needs the bug number and have "r=MattN" included before this can land.

Also, RE Try & checkin-needed -- last I heard [1], sheriffs are now *requiring* a Try push of some sort before landing checkin-needed patches.  This is to avoid the scenario where they push 10 patches, some of which lack a try push because they're totally-safe, and then that push causes bustage, and then the sheriff has to do non-trivial sleuthing to figure out which patch(es) to backout.

If you really think a Try push would be wasteful, that's fine (and I think I agree) -- but that just means someone non-sherriffy should land this. (e.g. you or me).

I nominate MattN since he's the reviewer/mentor. :D

[1] https://groups.google.com/d/msg/mozilla.dev.platform/9w0-Bh_3vVI/xWebYK64GdwJ
Flags: needinfo?(MattN+bmo)
Keywords: checkin-needed
(In reply to Daniel Holbert [:dholbert] from comment #12)
> Note: Commit message needs the bug number and have "r=MattN" included before
> this can land.

Good catch, I forgot to check that but thought the docs would explain it.

> Also, RE Try & checkin-needed -- last I heard [1], sheriffs are now
> *requiring* a Try push of some sort before landing checkin-needed patches. 
> This is to avoid the scenario where they push 10 patches, some of which lack
> a try push because they're totally-safe, and then that push causes bustage,
> and then the sheriff has to do non-trivial sleuthing to figure out which
> patch(es) to backout.

I've had success recently commenting that I don't think Try is necessary on some patches.

> If you really think a Try push would be wasteful, that's fine (and I think I
> agree) -- but that just means someone non-sherriffy should land this. (e.g.
> you or me).
> 
> I nominate MattN since he's the reviewer/mentor. :D

Luckily I just got r+ in the meantime on another bug so I'll push this when those Try results are green.
I'm generally willing to take a dev's word for it if they say it isn't required, but in general I get nervous about patches that aren't NPOTB or CSS/HTML-only type changes.
https://hg.mozilla.org/mozilla-central/rev/d178c0b658c5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: