windows mobile minor cleanup of xpcom

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dougt, Assigned: blassey)

Tracking

({fixed1.9.1})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

11 years ago
we removed most of the shunt library, and these are the changes to continue to compile.
Posted patch patch v1 (obsolete) — Splinter Review
Posted patch patch v2 (obsolete) — Splinter Review
Assignee: nobody → blassey
Attachment #347482 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #348092 - Flags: review?(benjamin)

Comment 3

11 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-
(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.
Posted patch patch v3 (obsolete) — Splinter Review
Attachment #348092 - Attachment is obsolete: true
Attachment #348245 - Flags: review?(benjamin)
Posted patch patch v4 (obsolete) — Splinter Review
Attachment #348245 - Attachment is obsolete: true
Attachment #348258 - Flags: review?(benjamin)
Attachment #348245 - Flags: review?(benjamin)

Comment 7

11 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+
I am not sure why I set that flag.  I've set it to NULL now.
http://hg.mozilla.org/mozilla-central/rev/652cba7f3f1d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
committed patch with change from CeGetCanonicalPath to _wfullpath as approved by bsmedberg on irc
Attachment #348258 - Attachment is obsolete: true
Attachment #351195 - Flags: review+
Attachment #351195 - Flags: approval1.9.1?
Comment on attachment 351195 [details] [diff] [review]
committed patch

a191=beltzner
Attachment #351195 - Flags: approval1.9.1? → approval1.9.1+
Reporter

Comment 13

11 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.