Closed Bug 281948 Opened 17 years ago Closed 17 years ago

Port gfx/widget to WinCE


(Core :: Widget: Win32, defect)

Windows XP
Not set





(Reporter: dougt, Assigned: dougt)




(1 file, 1 obsolete file)

Attached patch Patch v.1 (obsolete) — Splinter Review
I'd love to review, but I'm not up on my CE API.
Comment on attachment 174050 [details] [diff] [review]
Patch v.1

Did you really mean to leave the ifndef'd part commented out in components[]
array? I suspect this could be the reason my build failed to start after
applying the patch.

Anyway, here are some issues in order I found them (meaning the important ones
are hidden in the soup):

>   * ***** END LICENSE BLOCK ***** */
> -
> +#ifndef WINCE

I wouldn't remove that space.

> -        }        
> +        } 
> +#endif       
>          aMetric = sDragFullWindow ? 1 : 0;

Cleaning trailing spaces? Finish the work, please :)

>  #endif
> -
> +#ifndef WINCE 

I would, again, keep that line there for readability

> LRESULT CALLBACK DetectWindowMove(int code, WPARAM wParam, LPARAM lParam)
>  {
> +#ifndef WINCE

Could you ifndef out this function altogether?

> -
>        // if we've still determined that we should still rollup everything, do it.
> -      else if ( rollup ) {
> +      else 
> +#endif
> +     if ( rollup ) {

Please check the indentation.

>      mWnd = nsToolkit::mCreateWindowEx(extendedStyle,
>                                        aInitData && aInitData->mDropShadow ?
>                                        WindowPopupClassW() : WindowClassW(),
>                                        L"",
> -                                      style,
> +                                      WS_CHILD, //style,

We just did some work to setup style, you think you can get away with this

> -      DispatchStandardEvent(NS_DISPLAYCHANGED);
> -      break;
> +#ifndef WINCE
> +        case WM_DISPLAYCHANGE:
> +          DispatchStandardEvent(NS_DISPLAYCHANGED);
> +        break;
> +#endif


>    return ::CallNextHookEx(gMsgFilterHook, code, wParam, lParam);
> +#endif
> +  return 0;

This would be unreachable code in others but WINCE. Add #else etc.

Same goes for next two functions.
Attachment #174050 - Flags: review-
Attached patch patch v.2Splinter Review
Thanks for the comments.  I have updated the patch to fix all of your comments.
Attachment #174050 - Attachment is obsolete: true
Attachment #174635 - Flags: review?(emaijala)
How about my first question? Is this really supposed to be #if 0:

+#if 0
+  { "nsPrintOptionsWin",

Add "And" to the second sentence (two issues here).
That `#if 0` need to be removed from the patch.  I have done that locally.

basically, i have disabled some of the components in the components array on
WINCE.  During testing, i had a standard win32 build that I was using to verify
a crash.  I commented out components with the #if 0 to reproduce the crash and
should have taken it out before generating this diff.  So, in the end I do want
to disable the components on wince but not on win32. 

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

r=emaijala provided you remove the comments from nsWinWidgetFactory.cpp and #if
0 from nsGfxFactoryWin.cpp.
Attachment #174635 - Flags: review?(emaijala) → review+
Attachment #174635 - Flags: superreview?(darin)
Comment on attachment 174635 [details] [diff] [review]
patch v.2

>Index: gfx/src/windows/

>+ifneq ($(OS_ARCH), WINCE)
>+CPPSRCS += 	                       \
>+            nsPrintOptionsWin.cpp  \
>+            nsPrintSettingsWin.cpp \
>+			$(NULL)

nit: use tabs here instead of spaces.

>Index: gfx/src/windows/nsDeviceContextSpecWin.cpp

>+#ifdef WINCE
>+  aIsFound = PR_FALSE;

nit: maybe do this to make it easy to jump up to the #ifdef:

  #endif  // WINCE

same comment applies to the rest of the #ifdef/#ifndef's

from the context its hard for me to tell how easy it is to see
these #endif's without this comment.

>Index: gfx/src/windows/nsGfxFactoryWin.cpp

>+#if 0

badness!  should this be removed?  #ifdef WINCE or what?

>Index: gfx/src/windows/nsRenderingContextWin.cpp

the changes in this file make it seem like things might get fragile
as folks start hacking on the windows gfx implementation.  would it
be better to fork?  can you reduce the number of #ifdefs by 
implementing some of the removed functions as inlines that do nothing
on WINCE?  stuff like that that helps avoid #ifdef'ing the code so
heavily might be nice :)

>Index: widget/src/build/nsWinWidgetFactory.cpp
>Index: widget/src/windows/nsToolkit.cpp

>-NS_DefWindowProc    nsToolkit::mDefWindowProc = DefWindowProcA;
>+NS_DefWindowProc    nsToolkit::mDefWindowProc = (long (__stdcall *)(struct HWND__ *,unsigned int,unsigned int,long)) DefWindowProcA;

why is this ugly casting happening?  can you change the type of
mDefWindowProc instead?

> NS_CallWindowProc   nsToolkit::mCallWindowProc = CallWindowProcA;
> NS_SetWindowLong    nsToolkit::mSetWindowLong = SetWindowLongA;
> NS_GetWindowLong    nsToolkit::mGetWindowLong = GetWindowLongA;
> NS_SendMessage      nsToolkit::mSendMessage = nsSendMessage;
> NS_DispatchMessage  nsToolkit::mDispatchMessage = DispatchMessageA;
> NS_GetMessage       nsToolkit::mGetMessage = GetMessageA;
>-NS_PeekMessage      nsToolkit::mPeekMessage = PeekMessageA;
>+NS_PeekMessage      nsToolkit::mPeekMessage = (NS_PeekMessage)PeekMessageA;

same here.  casting like this is dangerous since it allows mismatches
of calling conventions to slip into the code.  are you sure there isn't
a better way?  can we typedef NS_PeekMessage differently on WINCE?

given all the #ifdef's might it not make sense to fork gfx and widget
for WINCE?
I fixed: nsWinWidgetFactory.cpp  and nsGfxFactoryWin.cpp.

I will drop the ugly casts in widget/src/windows/nsToolkit.cpp.  I think i was
playing around gettin the emulator SDK working which seams to not define
something of these routines.  A safe bet is to define them in my winCE stub.

I don't think we should fork:

1) the amount of code that is shared greatly exceeds that amount that has to be

1a) the stuff ifdef'ed is not really new or different functionality, but rather
stuff that isn't available on WinCE.  

2) I much rather support the #defines than a entire port. The WinCE port will
benefit greatly from having active develpers touching code around it.  I am
fearful that a CE port of gfx and widget will just bitrot.

3) It is very likely that people that know WinCE gfx and widget issues know
Win32.  More eyes on common code.

4) In other places of the code we have already taken the approach that our WinCE
code is just a configuration of the Win32 code we have.  Even NSPR -- the vast
majority of the changes are in #define's -- there are very few new window ce
specific files.

#ifdefs are likely to bitrot as well.  Either way it seems like getting a
tinderbox up and running would be the prudent thing to do.  I think that roc or
some other GFX/Widget peer needs to approve these changes.  I'm not that person.
Ere: my apologies... i did not realize that you are that peer ;-)
tinderbox is on the way.  I am the person to deal with any bustages.
I don't think we should fork the code.

However, we should do what we can to avoid #ifdefs on a case by case basis. For

+#ifndef WINCE

can't we just have it that nsToolkit::gAIMMapp is always NULL on WINCE?
plus what ere said about defining inline functions that do nothing on WINCE or
something on real Windows
thank you.  I fixed up ere's and darin's comments.

I do agree that we want to reduce the #defines to a min.  The example you point
out is also good.  however, IActiveIMMApp, the type of gAIMMApp, isn't defined
on WinCE.

so, at this point, do I have enough to check this patch in? sr=whom?
(In reply to comment #16)
> thank you.  I fixed up ere's and darin's comments.
> I do agree that we want to reduce the #defines to a min.  The example you
> point out is also good.  however, IActiveIMMApp, the type of gAIMMApp, isn't
> defined on WinCE.

We could define a dummy type for it in WINCE, then.

> so, at this point, do I have enough to check this patch in? sr=whom?

Looks like Darin's doing the sr :-)
Comment on attachment 174635 [details] [diff] [review]
patch v.2

sr=darin provided you minimize ifdefs by defining dummy types and inline
do-nothing functions.  the compiler should help us out here by eliminating most
of the code without manually inserting ifdefs.
Attachment #174635 - Flags: superreview?(darin) → superreview+
agreed.  the long term plan is to have the WinCE stub implement most of the
missing functionality that we need.  

As far as active input, I am not sure if there is anything we can do about this
as there really isn't a neat way to implement this on winCE.  Stubbing it
correctly is a fair chuck of work; the interface has about 70 methods and many
of these methods have types which are not defined in WinCe.  This only to save 5
discrete #defines.  

However, the general point is well taken -- we want to reduce, where possible,
the number of #defines.
Checking in widget/src/build/;
/cvsroot/mozilla/widget/src/build/,v  <--
new revision: 1.53; previous revision: 1.52
no host:  at /cvsroot/CVSROOT/ line 49.
Checking in widget/src/build/nsWinWidgetFactory.cpp;
/cvsroot/mozilla/widget/src/build/nsWinWidgetFactory.cpp,v  <-- 
new revision: 1.65; previous revision: 1.64
Checking in widget/src/windows/;
/cvsroot/mozilla/widget/src/windows/,v  <--
new revision: 3.22; previous revision: 3.21
Checking in widget/src/windows/nsImageClipboard.h;
/cvsroot/mozilla/widget/src/windows/nsImageClipboard.h,v  <--  nsImageClipboard.h
new revision: 1.7; previous revision: 1.6
Checking in widget/src/windows/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/windows/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.49; previous revision: 1.48
Checking in widget/src/windows/nsToolkit.cpp;
/cvsroot/mozilla/widget/src/windows/nsToolkit.cpp,v  <--  nsToolkit.cpp
new revision: 3.45; previous revision: 3.44
Checking in widget/src/windows/nsToolkit.h;
/cvsroot/mozilla/widget/src/windows/nsToolkit.h,v  <--  nsToolkit.h
new revision: 3.27; previous revision: 3.26
Checking in widget/src/windows/nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.539; previous revision: 3.538
Checking in widget/src/windows/nsWindow.h;
/cvsroot/mozilla/widget/src/windows/nsWindow.h,v  <--  nsWindow.h
new revision: 3.197; previous revision: 3.196
Checking in widget/src/windows/nsWindowAPI.h;
/cvsroot/mozilla/widget/src/windows/nsWindowAPI.h,v  <--  nsWindowAPI.h
new revision: 3.7; previous revision: 3.6
Checking in gfx/src/windows/;
/cvsroot/mozilla/gfx/src/windows/,v  <--
new revision: 1.25; previous revision: 1.24
Checking in gfx/src/windows/nsDeviceContextSpecWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsDeviceContextSpecWin.cpp,v  <-- 
new revision: 3.53; previous revision: 3.52
Checking in gfx/src/windows/nsDeviceContextWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsDeviceContextWin.cpp,v  <-- 
new revision: 3.119; previous revision: 3.118
Checking in gfx/src/windows/nsFontMetricsWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsFontMetricsWin.cpp,v  <--  nsFontMetricsWin.cpp
new revision: 3.223; previous revision: 3.222
Checking in gfx/src/windows/nsGfxFactoryWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsGfxFactoryWin.cpp,v  <--  nsGfxFactoryWin.cpp
new revision: 3.38; previous revision: 3.37
Checking in gfx/src/windows/nsImageWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsImageWin.cpp,v  <--  nsImageWin.cpp
new revision: 3.138; previous revision: 3.137
Checking in gfx/src/windows/nsNativeThemeWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsNativeThemeWin.cpp,v  <--  nsNativeThemeWin.cpp
new revision: 3.42; previous revision: 3.41
Checking in gfx/src/windows/nsRenderingContextWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsRenderingContextWin.cpp,v  <-- 
new revision: 3.176; previous revision: 3.175

Thanks all.
Closed: 17 years ago
Resolution: --- → FIXED
Just for the record:

I also considered the fork, but discarded the idea quite quickly as vast
majority of the code is common. The dummies is a good idea (not mine!). I
would've liked to see the final checked-in patch attached here too.
You need to log in before you can comment on or make changes to this bug.