461859 - windows mobile minor cleanup of xpcom

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

we removed most of the shunt library, and these are the changes to continue to compile.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v1

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v2

Comment 3

[Benjamin Smedberg]

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

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v3

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v4

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

I am not sure why I set that flag.  I've set it to NULL now.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

http://hg.mozilla.org/mozilla-central/rev/652cba7f3f1d

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: committed patch

committed patch with change from CeGetCanonicalPath to _wfullpath as approved by bsmedberg on irc

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 351195 [details] [diff] [review]
committed patch

a191=beltzner

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/97915cec79f5

Comment 13

[Doug Turner (:dougt)]

this landed on the 1.9.1 branch.  adding keyword fixed1.9.1