Closed Bug 327448 Opened 14 years ago Closed 14 years ago

Redundant implicit calls in NSPR

Categories

(NSPR :: NSPR, defect, P2)

x86
Windows XP
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: emk, Assigned: emk)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 2 obsolete files)

CreateFileW, FindFirstFileW, FindNextFileW, GetDriveTypeW, and GetFullPathNameW are  exported from Win95 kernel as stubs.
Therefore it's meaningless to call them implicitly.
Attached patch patch (obsolete) — Splinter Review
I have confirmed this works on Win95.
Assignee: wtchang → VYV03354
Status: NEW → ASSIGNED
Attachment #212102 - Flags: review?(wtchang)
Comment on attachment 212102 [details] [diff] [review]
patch

Masatoshi: thank you for the patch.  If we
call the W functions on Windows 95, do they
fail with the ERROR_CALL_NOT_IMPLEMENTED error?

Do these W function stubs exist in Windows
95 even if you don't install the Microsoft
Layer for Unicode (MSLU)?
Attachment #212102 - Flags: superreview?(jshin1987)
Attachment #212102 - Flags: review?(wtchang)
Attachment #212102 - Flags: review+
> If we call the W functions on Windows 95, do they
> fail with the ERROR_CALL_NOT_IMPLEMENTED error?
Yes.

> Do these W function stubs exist in Windows
> 95 even if you don't install the Microsoft
> Layer for Unicode (MSLU)?
Yes. I've confirmed it on very early version of Win95a without IE, without MSLU.
In general, Win95 Kernel32 exports all documented WinNT3.5 APIs (including all W functions) even if they are not implemented.

FYI W functions will succeed instead of failing with the ERROR_CALL_NOT_IMPLEMENTED error if we install MSLU.
> if we install MSLU.
if we install MSLU and our program link with unicows.lib.
I checked in the patch on the NSPR trunk (NSPR 4.7).

Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.27; previous revision: 3.26
done

jshin, I'd still appreciate your review of the patch.

The Mozilla trunk is closed right now.  I will check
in the patch on the NSPRPUB_PRE_4_2_CLIENT_BRANCH when
the Mozilla trunk re-opens.
Priority: -- → P2
Target Milestone: --- → 4.7
Comment on attachment 212102 [details] [diff] [review]
patch

add. r=me (because I'm not a superreviewer). btw, these function pointers need to be included back later when we add UTF16 APIs on Win 9x/ME without MSLU (i.e. by emulating 'W' APIs ourselves).
Attachment #212102 - Flags: superreview?(jshin1987)
Attachment #212102 - Flags: superreview?(darin)
Attachment #212102 - Flags: review+
Given jshin's comment 6, I think we should keep the
function pointers.  But we don't need to look up the
function symbols at run time.  Here is my proposed
alternate patch that reduces the amount of work when
we emulate the MSLU W functions.

jshin: in NSPR I'm using the superreview to mean
"second review".  Feel free to mark review+ and
cancel the super-review on patches you like.
Attachment #212263 - Flags: superreview?(jshin1987)
Attachment #212263 - Flags: review?(VYV03354)
Comment on attachment 212263 [details] [diff] [review]
Alternate patch: keep function pointers but do not lookup the W functions

It's better to initialize function pointers with W functions than to initialize them with NULL and assign W functions later. Because we are trying to eliminate NULL checks.
Leave InitUnicodeSupport() as an empty function for when we emulate the MSLU W functions.

BTW, these functions had not returned PR_NOT_IMPLEMENTED_ERROR because GetProcAddress() would never fail.
Perhaps it's better to add ERROR_CALL_NOT_IMPLEMENTED case in _MD_win32_map_default_error.
Attachment #212263 - Flags: review?(VYV03354) → review-
Attachment #212102 - Attachment is obsolete: true
Attachment #212263 - Attachment is obsolete: true
Attachment #212270 - Flags: review?(wtchang)
Attachment #212102 - Flags: superreview?(darin)
Attachment #212263 - Flags: superreview?(jshin1987)
Attachment #212270 - Flags: review?(jshin1987)
Comment on attachment 212270 [details] [diff] [review]
Alternative patch, take 3.

r=wtc. Thank you, Masatoshi.
Attachment #212270 - Flags: review?(wtchang) → review+
Comment on attachment 212270 [details] [diff] [review]
Alternative patch, take 3.

thanks. r=jshin
Attachment #212270 - Flags: review?(jshin1987) → review+
I checked in the "alternative patch, take 3" on the NSPR
trunk (NSPR 4.7) and the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(mozilla 1.9 alpha).

Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.29; previous revision: 3.28
done
Checking in win32_errors.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/win32_errors.c,v  <--  win32_errors.c

new revision: 3.11; previous revision: 3.10
done

Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.22.4.6; previous revision: 3.22.4.5
done
Checking in win32_errors.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/win32_errors.c,v  <--  win32_errors.c

new revision: 3.9.4.2; previous revision: 3.9.4.1
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The patch for bug 326168 got rejected in w95io.c (in NSPR corresponding to 1.8.x branch) because this bug hasn't been fixed in 1.8.x branch. Although it's trivial to modify it so that it can be applied to the branch,  it might be a good idea to fix this bug in 1.8.x branch as well.
Comment on attachment 212270 [details] [diff] [review]
Alternative patch, take 3.

a=darin for the MOZILLA_1_8_BRANCH
Attachment #212270 - Flags: approval-branch-1.8.1+
Darin, if you check this patch in on the MOZILLA_1_8_BRANCH,
you also need to check it in on the NSPR_4_6_BRANCH and change
the target milestone to 4.6.2.
OK, good to know.  I'm not sure that we should check this in until we know that we wish to take the whole set of i18n changes on the MOZILLA_1_8_BRANCH.  Jshin: Can you set bug dependencies appropriately?
I checked in the patch on the NSPR_4_6_BRANCH (NSPR 4.6.2)
and MOZILLA_1_8_BRANCH (Mozilla 1.8.1), to make it easier
to apply the patch for bug 326168.
Keywords: fixed1.8.1
Target Milestone: 4.7 → 4.6.2
You need to log in before you can comment on or make changes to this bug.