281948 - Port gfx/widget to WinCE

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Doug Turner (:dougt)]

Comment 1

[Doug Turner (:dougt)]

Attachment: Patch v.1

Comment 2

[:deanis74]

I'd love to review, but I'm not up on my CE API.

Comment 3

[Ere Maijala (slow)]

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 
>  BOOL APIENTRY DllMain(  HINSTANCE hModule, 

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
change?

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

Indentation?

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

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

Thanks for the comments.  I have updated the patch to fix all of your comments.

Comment 5

[Ere Maijala (slow)]

How about my first question? Is this really supposed to be #if 0:

+
+#if 0
+  { "nsPrintOptionsWin",
...

?

Comment 6

[Ere Maijala (slow)]

Add "And" to the second sentence (two issues here).

Comment 7

[Doug Turner (:dougt)]

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 8

[Ere Maijala (slow)]

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.

Comment 9

[Darin Fisher]

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

>Index: gfx/src/windows/Makefile.in

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

nit: use tabs here instead of spaces.


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

>+#ifdef WINCE
>+  aIsFound = PR_FALSE;
>+#else
...
>+#endif

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?

Comment 10

[Doug Turner (:dougt)]

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
ifdef'ed. 

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.

Comment 11

[Darin Fisher]

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

Comment 12

[Darin Fisher]

Ere: my apologies... i did not realize that you are that peer ;-)

Comment 13

[Doug Turner (:dougt)]

tinderbox is on the way.  I am the person to deal with any bustages.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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
example

+#ifndef WINCE
         if(nsToolkit::gAIMMApp)
             nsToolkit::gAIMMApp->Activate(TRUE);
+#endif

can't we just have it that nsToolkit::gAIMMapp is always NULL on WINCE?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

plus what ere said about defining inline functions that do nothing on WINCE or
something on real Windows

Comment 16

[Doug Turner (:dougt)]

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?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Darin Fisher]

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.

Comment 19

[Doug Turner (:dougt)]

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.

Comment 20

[Doug Turner (:dougt)]

Checking in widget/src/build/Makefile.in;
/cvsroot/mozilla/widget/src/build/Makefile.in,v  <--  Makefile.in
new revision: 1.53; previous revision: 1.52
done
no host:  at /cvsroot/CVSROOT/sendnotification.pl line 49.
Checking in widget/src/build/nsWinWidgetFactory.cpp;
/cvsroot/mozilla/widget/src/build/nsWinWidgetFactory.cpp,v  <-- 
nsWinWidgetFactory.cpp
new revision: 1.65; previous revision: 1.64
done
Checking in widget/src/windows/Makefile.in;
/cvsroot/mozilla/widget/src/windows/Makefile.in,v  <--  Makefile.in
new revision: 3.22; previous revision: 3.21
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done
Checking in gfx/src/windows/Makefile.in;
/cvsroot/mozilla/gfx/src/windows/Makefile.in,v  <--  Makefile.in
new revision: 1.25; previous revision: 1.24
done
Checking in gfx/src/windows/nsDeviceContextSpecWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsDeviceContextSpecWin.cpp,v  <-- 
nsDeviceContextSpecWin.cpp
new revision: 3.53; previous revision: 3.52
done
Checking in gfx/src/windows/nsDeviceContextWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsDeviceContextWin.cpp,v  <-- 
nsDeviceContextWin.cpp
new revision: 3.119; previous revision: 3.118
done
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
done
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
done
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
done
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
done
Checking in gfx/src/windows/nsRenderingContextWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsRenderingContextWin.cpp,v  <-- 
nsRenderingContextWin.cpp
new revision: 3.176; previous revision: 3.175
done

Thanks all.

Comment 21

[Ere Maijala (slow)]

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.