280245 - WINCE xpcom changes

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Doug Turner (:dougt)]

needed to make a changes to get xpcom to compile and work on wince

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Darin Fisher]

Comment on attachment 172729 [details] [diff] [review]
patch v.1

>Index: xpcom/build/dlldeps.cpp

>+#if defined DEBUG && !defined (WINCE)

you need parentheses around DEBUG right?


>Index: xpcom/glue/standalone/nsGREDirServiceProvider.cpp

> GRE_GetGREPath()
> {
>-  // we've already done this...
>+// we've already done this...
>   if (*sGRELocation)
>     return sGRELocation;

this looks like an unintended change.


>Index: xpcom/glue/standalone/nsXPCOMGlue.cpp

>+#if defined XP_WIN32

parens?


>Index: xpcom/io/nsAppFileLocationProvider.cpp

>+#if defined (XP_MAC) || defined (WINCE)

nit: other parts of this file seem to use different whitespacing:

  #if defined(XP_MAC) || defined(WINCE)


>Index: xpcom/io/nsLocalFileWin.cpp

>     if ( ( (secondChar == ':') && !FindCharInReadable('/', begin, end) ) ||  // normal path
>+         ( (firstChar == '\\') && (secondChar == '\\') )   // network path
>+#ifdef WINCE
>+         || ( (firstChar == '\\') )   // wince absolute path
>+#endif

hmm... not that it really matters but you could change this to:

>     if ( ( (secondChar == ':') && !FindCharInReadable('/', begin, end) ) ||  // normal path
>+#ifdef WINCE
>+         ( (firstChar == '\\') )   // wince absolute path or network path
>+#else
>+         ( (firstChar == '\\') && (secondChar == '\\') )   // network path
>+#endif

since otherwise you are doing a bit of extra work on wince testing secondChar
for no reason.


>+    if (path == nsnull) {
>+        printf("NS_ERROR_FILE_UNRECOGNIZED_PATH\n");
>         return NS_ERROR_FILE_UNRECOGNIZED_PATH;
>+    }

please remove this printf before checking in.



>+#ifdef WINCE
>+        ++path;
>+#else
>         // dealing with a UNC path here; skip past '\\machine\'

oh, so wince doesn't support UNC paths.  interesting...
so should the firstChar/secondChar code above enforce that?


> NS_IMETHODIMP
> nsLocalFile::Normalize()
> {
>+#ifndef WINCE

hmm.. are you sure?  i think there may be some new consumers
of this method that may expect path segments like "/../" to be
normalized.


>-        if (pGetDiskFreeSpaceExA(mResolvedPath.get(), &liFreeBytesAvailableToCaller, 
>-            &liTotalNumberOfBytes, NULL))
>+        if (pGetDiskFreeSpaceExA(mResolvedPath.get(), &liFreeBytesAvailableToCaller, &liTotalNumberOfBytes, NULL))

why change this?  it's nice to break up overly long lines of code.


>-    if (GetDiskFreeSpace(aDrive, &dwSecPerClus, &dwBytesPerSec, &dwFreeClus, &dwTotalClus))
>+
>+    if ((aDrive, &dwSecPerClus, &dwBytesPerSec, &dwFreeClus, &dwTotalClus))

something's not quite right here.


>+    //liar.
>+    *aDiskSpaceAvailable = 0;
>+    return NS_OK;
> }

does this need to be fixed?  or perhaps it doesn't matter for wince?


>-        // Convert extension to lower case.
>-        for( unsigned char *p = (unsigned char *)ext; *p; p++ )
>-            *p = _mbctolower( *p );
> 
>         // Search for any of the set of executable extensions.
>         const char * const executableExts[] = {
>@@ -1911,7 +1930,7 @@
>             ".wsh",
>             0 };
>         for ( int i = 0; executableExts[i]; i++ ) {
>-            if ( ::strcmp( executableExts[i], ext ) == 0 ) {
>+            if ( _strnicmp( executableExts[i], ext, 4 ) == 0 ) {

why this change?  isn't it more optimal to do the case conversion
once?


>Index: xpcom/obsolete/nsSpecialSystemDirectory.cpp

>+#if defined (WINCE)
>+            {
>+                *this = MakeUpperCase("\\Temp");
>+            }

This seems wrong to me.  MakeUpperCase modifies its
input argument.  Wouldn't that cause problems here?

Why not |*this = "\\TEMP";| instead?


>Index: xpcom/threads/plevent.c

bsmedberg would probably appreciate this patch.  i
recall when he had to add that #ifndef for LIBXUL.


>Index: xpcom/windbgdlg/windbgdlg.cpp

>-int WINAPI 
>-WinMain(HINSTANCE  hInstance, HINSTANCE  hPrevInstance, 
>-        LPSTR  lpszCmdLine, int  nCmdShow)
>+int main(int argc, char **argv)

this confuses me.  i thought you needed to use WinMain if
you didn't want to see a DOS prompt.  is that not so?


>-    wsprintf(msg,
>+    sprintf(msg,

I wonder why this was using wsprintf previously.  any idea?
I think timeless may know something about this code.

Comment 3

[Doug Turner (:dougt)]

darin, thanks for the comments.  I will clean up the patch and post another.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

This fixes most of comments.  I haven't implemented normalize or the free
diskspace yet.	I plan to, but I wanted to get what I have into cvs so that
other can share what I have already. For example, I haven't even started on the
xptcall stuff.	I added comments to places where I need to flush out still.

As to the question about main vrs. WinMain -- you can use either.  However, if
you use main, your compiler must set the linker option ENTRY to
mainACRTStartup.

Comment 5

[Darin Fisher]

Comment on attachment 172855 [details] [diff] [review]
patch v.2

This patch is only for windbgdlg.cpp.  Anyways, I'm fine with what you wrote in
your previous comment.	r=me with those other issues addressed.

Comment 6

[Doug Turner (:dougt)]

Checking in base/nsDebugImpl.cpp;
/cvsroot/mozilla/xpcom/base/nsDebugImpl.cpp,v  <--  nsDebugImpl.cpp
new revision: 3.9; previous revision: 3.8
done
Checking in base/nsTraceRefcntImpl.cpp;
/cvsroot/mozilla/xpcom/base/nsTraceRefcntImpl.cpp,v  <--  nsTraceRefcntImpl.cpp
new revision: 1.95; previous revision: 1.94
done
Checking in build/Makefile.in;
/cvsroot/mozilla/xpcom/build/Makefile.in,v  <--  Makefile.in
new revision: 1.83; previous revision: 1.82
done
Checking in build/dlldeps.cpp;
/cvsroot/mozilla/xpcom/build/dlldeps.cpp,v  <--  dlldeps.cpp
new revision: 1.120; previous revision: 1.119
done
Checking in build/nsXPCOMPrivate.h;
/cvsroot/mozilla/xpcom/build/nsXPCOMPrivate.h,v  <--  nsXPCOMPrivate.h
new revision: 1.22; previous revision: 1.21
done
Checking in build/nsXPComInit.cpp;
/cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v  <--  nsXPComInit.cpp
new revision: 1.206; previous revision: 1.205
done
Checking in glue/nsCOMPtr.h;
/cvsroot/mozilla/xpcom/glue/nsCOMPtr.h,v  <--  nsCOMPtr.h
new revision: 1.120; previous revision: 1.119
done
Checking in glue/standalone/nsXPCOMGlue.cpp;
/cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.cpp,v  <--  nsXPCOMGlue.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in io/SpecialSystemDirectory.cpp;
/cvsroot/mozilla/xpcom/io/SpecialSystemDirectory.cpp,v  <-- 
SpecialSystemDirectory.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in io/nsAppFileLocationProvider.cpp;
/cvsroot/mozilla/xpcom/io/nsAppFileLocationProvider.cpp,v  <-- 
nsAppFileLocationProvider.cpp
new revision: 1.54; previous revision: 1.53
done
Checking in io/nsLocalFileWin.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v  <--  nsLocalFileWin.cpp
new revision: 1.132; previous revision: 1.131
done
Checking in obsolete/Makefile.in;
/cvsroot/mozilla/xpcom/obsolete/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
Checking in obsolete/nsFileSpecWin.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecWin.cpp,v  <--  nsFileSpecWin.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in obsolete/nsSpecialSystemDirectory.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsSpecialSystemDirectory.cpp,v  <-- 
nsSpecialSystemDirectory.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in threads/plevent.c;
/cvsroot/mozilla/xpcom/threads/plevent.c,v  <--  plevent.c
new revision: 1.49; previous revision: 1.48
done
Checking in typelib/Makefile.in;
/cvsroot/mozilla/xpcom/typelib/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
Checking in typelib/xpt/Makefile.in;
/cvsroot/mozilla/xpcom/typelib/xpt/Makefile.in,v  <--  Makefile.in
new revision: 1.15; previous revision: 1.14
done

Thanks darin.  I am going to leave this bug open for XPTCall support.

Comment 7

[Doug Turner (:dougt)]

i am actually going to close this since I justed opened bug 281230 which will
track the XPTCall support.

Comment 8

[James Ross]

This broke XULrunner. :(

In the patch, you removed line:
  HINSTANCE _pr_hInstance;
which is needed to link toolkit/library/nsDllMain.cpp as part of libxul.

Comment 9

[Doug Turner (:dougt)]

james, did you file a bug against xul runner?

Comment 10

[James Ross]

No, because you have changed the code that used _pr_hInstance here. Also, you
should have removed the lines from nsDllMain if it really wasn't needed. If I
was being picky, I'd reopen this because you broke it, but I'll just file
ANOTHER bug.

Comment 11

[Doug Turner (:dougt)]

maybe my point is (and I could be wrong) is that I don't want anything linking
to xpcom to get _pr_hInstance

Comment 12

[James Ross]

I've filed bug 281952, but I was expecting you to check if things were used when
removing them, even if they shouldn't be used outside of xpcom. (it wasn't
/using/ _pr_hInstance, so much as *actually setting it*, too)

Comment 13

[Doug Turner (:dougt)]

I don't disagree. I should have checked if someone was linking to an exported
symbol that I removed.