Closed
Bug 461859
Opened 14 years ago
Closed 14 years ago
windows mobile minor cleanup of xpcom
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dougt, Assigned: blassey)
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 4 obsolete files)
14.24 KB,
patch
|
blassey
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
we removed most of the shunt library, and these are the changes to continue to compile.
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Comment 2•14 years ago
|
||
Assignee: nobody → blassey
Attachment #347482 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #348092 -
Flags: review?(benjamin)
Comment 3•14 years ago
|
||
Comment on attachment 348092 [details] [diff] [review] patch v2 >diff --git a/xpcom/glue/nsGREGlue.cpp b/xpcom/glue/nsGREGlue.cpp >@@ -185,20 +185,32 @@ GRE_GetGREPathWithProperties(const GREVe > // if GRE_HOME is in the environment, use that GRE > const char* env = getenv("GRE_HOME"); > if (env && *env) { > char p[MAXPATHLEN]; > snprintf(p, sizeof(p), "%s" XPCOM_FILE_PATH_SEPARATOR XPCOM_DLL, env); > p[sizeof(p) - 1] = '\0'; > > #if XP_UNIX > if (realpath(p, aBuffer)) > return NS_OK; >+#elif WINCE >+ if (p[0] != '\\') >+ { >+ unsigned short dir[MAX_PATH]; >+ CeGetCanonicalPathName(NS_ConvertUTF8toUTF16(p).get(), dir, MAX_PATH, 0); >+ WideCharToMultiByte(CP_ACP, 0, dir, MAX_PATH, >+ aBuffer, MAX_PATH, NULL, NULL); >+ } I can't quite figure out what you're doing here. `p` is taken from `env`, which is going to be in the native charset, not UTF8. In any case, You can't use the NS_Convert functions in this codepath, because we haven't found XPCOM yet and dynamically linked up those functions. >- else if (!wcsncat(aBuffer, L"\\" LXPCOM_DLL, aBufLen) || >- _waccess( >- aBuffer, R_OK)) { >+ else if (!wcsncat(aBuffer, L"\\" LXPCOM_DLL, aBufLen) >+#ifdef WINCE >+ || (GetFileAttributesW(aBuffer) != 0xFFFFFFFF) The magic number is not good. Is this the value of INVALID_FILE_ATTRIBUTES and is there any reason you can't just use that symbolic name? >diff --git a/xpcom/glue/standalone/nsGlueLinkingWin.cpp b/xpcom/glue/standalone/nsGlueLinkingWin.cpp >- _wfullpath >- (xpcomDir, xpcomFile, sizeof(xpcomDir)); >+#ifdef WINCE >+ CeGetCanonicalPathName(xpcomFile, xpcomDir, >+ sizeof(xpcomDir)/sizeof(wchar_t), 0); >+#else >+ _wfullpath(xpcomDir, xpcomFile, sizeof(xpcomDir)); >+#endif I read the CeGetCanonicalPathName docs, and it wasn't clear to me whether the buffer size is characters or bytes. But this does expose a bug in the desktop impl: _wfullpath takes it length in characters, not bytes. Please fix that while you're here, and check on the wince version please? >diff --git a/xpcom/io/nsLocalFileWin.cpp b/xpcom/io/nsLocalFileWin.cpp >@@ -2033,51 +2036,70 @@ nsLocalFile::SetPermissions(PRUint32 aPe >+#ifndef WINCE > > // windows only knows about the following permissions > int mode = 0; > if (aPermissions & (PR_IRUSR|PR_IRGRP|PR_IROTH)) // any read > mode |= _S_IREAD; > if (aPermissions & (PR_IWUSR|PR_IWGRP|PR_IWOTH)) // any write > mode |= _S_IWRITE; > > if (_wchmod(mResolvedPath.get(), mode) == -1) > return NS_ERROR_FAILURE; > > return NS_OK; >+#else >+ >+ // windows only knows about the following permissions You mean Windows CE in this comment, right? > nsLocalFile::Launch() > // use the app registry name to launch a shell execute.... >- LONG r = (LONG) ::ShellExecuteW(NULL, NULL, path.get(), NULL, NULL, >- SW_SHOWNORMAL); >- >- // if the file has no association, we launch windows' "what do you want to do" dialog >+ SHELLEXECUTEINFOW seinfo; >+ memset(&seinfo, 0, sizeof(seinfo)); >+ seinfo.cbSize = sizeof(SHELLEXECUTEINFOW); >+ seinfo.fMask = SEE_MASK_NOCLOSEPROCESS; >+ seinfo.hwnd = NULL; >+ seinfo.lpVerb = NULL; >+ seinfo.lpFile = path.get(); >+ seinfo.lpParameters = NULL; >+ seinfo.lpDirectory = NULL; >+ seinfo.nShow = SW_SHOWNORMAL; Are you doing all this because WINCE doesn't have ShellExecute or because you need the SEE_MASK_NOCLOSEPROCESS flag to get hProcess? I don't see you actually using or closing hProcess, which is puzzling. >diff --git a/xpcom/typelib/xpt/tools/xpt_dump.c b/xpcom/typelib/xpt/tools/xpt_dump.c >+#ifdef WINCE >+#include "windows.h" nit, use angle brackets <windows.h> here and below
Attachment #348092 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #3) > >- else if (!wcsncat(aBuffer, L"\\" LXPCOM_DLL, aBufLen) || > >- _waccess( > >- aBuffer, R_OK)) { > >+ else if (!wcsncat(aBuffer, L"\\" LXPCOM_DLL, aBufLen) > >+#ifdef WINCE > >+ || (GetFileAttributesW(aBuffer) != 0xFFFFFFFF) > > The magic number is not good. Is this the value of INVALID_FILE_ATTRIBUTES and > is there any reason you can't just use that symbolic name? The 0xFFFFFFFF is from the wince docs http://msdn.microsoft.com/en-us/library/ms960701.aspx "The attributes of the specified file or directory, returned in a DWORD, indicates success. 0xFFFFFFFF indicates failure." looks like INVALID_FILE_ATTRIBUTES will work: #define INVALID_FILE_ATTRIBUTES ((DWORD)-1) > > >diff --git a/xpcom/glue/standalone/nsGlueLinkingWin.cpp b/xpcom/glue/standalone/nsGlueLinkingWin.cpp > > >- _wfullpath > >- (xpcomDir, xpcomFile, sizeof(xpcomDir)); > >+#ifdef WINCE > >+ CeGetCanonicalPathName(xpcomFile, xpcomDir, > >+ sizeof(xpcomDir)/sizeof(wchar_t), 0); > >+#else > >+ _wfullpath(xpcomDir, xpcomFile, sizeof(xpcomDir)); > >+#endif > > I read the CeGetCanonicalPathName docs, and it wasn't clear to me whether the > buffer size is characters or bytes. But this does expose a bug in the desktop > impl: _wfullpath takes it length in characters, not bytes. Please fix that > while you're here, and check on the wince version please? You're right, the documentation is not clear. Assuming its the the number of wchar's is safer. > > nsLocalFile::Launch() > > > > // use the app registry name to launch a shell execute.... > >- LONG r = (LONG) ::ShellExecuteW(NULL, NULL, path.get(), NULL, NULL, > >- SW_SHOWNORMAL); > >- > >- // if the file has no association, we launch windows' "what do you want to do" dialog > >+ SHELLEXECUTEINFOW seinfo; > >+ memset(&seinfo, 0, sizeof(seinfo)); > >+ seinfo.cbSize = sizeof(SHELLEXECUTEINFOW); > >+ seinfo.fMask = SEE_MASK_NOCLOSEPROCESS; > >+ seinfo.hwnd = NULL; > >+ seinfo.lpVerb = NULL; > >+ seinfo.lpFile = path.get(); > >+ seinfo.lpParameters = NULL; > >+ seinfo.lpDirectory = NULL; > >+ seinfo.nShow = SW_SHOWNORMAL; > > Are you doing all this because WINCE doesn't have ShellExecute or because you > need the SEE_MASK_NOCLOSEPROCESS flag to get hProcess? I don't see you actually > using or closing hProcess, which is puzzling. wince doesn't have ShellExecute, the intent here is to use ShellExecuteEx for the same functionality.
Assignee | ||
Comment 5•14 years ago
|
||
Attachment #348092 -
Attachment is obsolete: true
Attachment #348245 -
Flags: review?(benjamin)
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #348245 -
Attachment is obsolete: true
Attachment #348258 -
Flags: review?(benjamin)
Attachment #348245 -
Flags: review?(benjamin)
Comment 7•14 years ago
|
||
Comment on attachment 348258 [details] [diff] [review] patch v4 tabs and trailing whitespace have crept in... please fix before commit. >- // if the file has no association, we launch windows' "what do you want to do" dialog >+ SHELLEXECUTEINFOW seinfo; >+ memset(&seinfo, 0, sizeof(seinfo)); >+ seinfo.cbSize = sizeof(SHELLEXECUTEINFOW); >+ seinfo.fMask = SEE_MASK_NOCLOSEPROCESS; I still don't understand why you're passing this flag. Please remove this flag, or re-request review explaining why you need it, and why you're not calling CloseHandle on the result.
Attachment #348258 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 8•14 years ago
|
||
I am not sure why I set that flag. I've set it to NULL now.
Assignee | ||
Comment 9•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/652cba7f3f1d
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•14 years ago
|
||
committed patch with change from CeGetCanonicalPath to _wfullpath as approved by bsmedberg on irc
Attachment #348258 -
Attachment is obsolete: true
Attachment #351195 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #351195 -
Flags: approval1.9.1?
Comment 11•14 years ago
|
||
Comment on attachment 351195 [details] [diff] [review] committed patch a191=beltzner
Attachment #351195 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 12•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/97915cec79f5
Reporter | ||
Comment 13•14 years ago
|
||
this landed on the 1.9.1 branch. adding keyword fixed1.9.1
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•