333235 - Get Thebes to work on OS/2

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Peter Weilbacher]

Now that Thebes is working on BeOS we should also get going to get it working for OS/2. The BeOS patch from bug 331888 nicely summarizes what is needed. For us it will be a bit harder because AFAIK we cannot use Pango for the font stuff.

I have already made the first steps to changes the build config and created skeleton files in gfx/thebes/{public,src} and gfx/src/thebes to get it to compile. Once I get it to start up, too, I will upload my first work-in-progress patch.

Comment 1

[Peter Weilbacher]

Attachment: Work in progress

Just to show that I am still working on this. Surface creation seems to work but otherwise it's a lot harder than I thought. The font stuff is just a placeholder so far, and so a build with this still crashes before showing anything...

Comment 2

[Peter Weilbacher]

A short status update: I filled in some more of the functions in gfx/thebes/{public,src}/gfx*OS2* and now I have a Firefox that uses Thebes and actually starts up without crashing. At the moment it doesn't paint anything in the client area, though, don't know yet why that is. And it is really slow...

Comment 3

[Peter Weilbacher]

OK, the reason was just some mixup in the stuff that needs to be added to nsWindow. Now I have something that starts up and displays something, but without any text characters so far. And when scrolling lots of stuff gets mixed up.

However, I think I can already say that I can load sites like hamberg.dk and cairographics.org without any special CPU load, so it seems that with Thebes bug 167884 might get fixed. :-)

Comment 4

[Andy Willis (abwillis)]

I have been thinking of adding support on OS/2 for --enable-Freetype2.  Now that we are using mzftcfg I don't see any reason not to.  My question is would it help any for getting Thebes setup?  If so I will up the priority I assign to looking into it.

Comment 5

[Peter Weilbacher]

No, FreeType is never used directly in any of the Thebes code AFAICT, so that won't help. If you could quickly port Pango to OS/2-PM that would help a lot, unfortunately I think it is not that easy...

Otherwise we have to port much of the stuff from gfx/os2/ to thebes/src/gfxOS2* and while most of the functions are there, they are sometimes supposed to return or take different parameters, and so it's not just cut-and-paste. I have gotten a bit further, but it takes time. (I hope that I can show something at Warpstock Europe, Nov 17/18).

Comment 6

[Peter Weilbacher]

*** Bug 355043 has been marked as a duplicate of this bug. ***

Comment 7

[Peter Weilbacher]

Attachment: First real attempt

OK, to get the process started on this again, especially in the light of bug 177805, I thought I should upload the current status.

Notes:
- To build, use --enable-default-toolkit=cairo-os2
- I hope I have included all required changes to get it at least to
  build...
- I only tried Firefox so far.
- The text on the menubar does not display, often parts of pages and
  scrollbars are not painted correctly.
- To get the window contents to display one has to maximise it. When
  restoring the window does not appear. Alt-F10 or Alt-F7 shows where it
  is.
- All text is currently drawn as the OS/2 GPI default font (System
  Proportional 10).
- When visiting one of the examples of bug 167884, alpha-blending seems
  to be fast.
- On the contrary, text drawing is _really_ slow.
- The current code contains lots of other dirty hacks that are definitely
  no longterm solution.
- There is lots of debug printfs that slows it down even more...

- I somehow have the idea that the division into nsWindow and nsFrameWindow
  on OS/2 is partly responsible for the difficulties but haven't had time
  to investigate in detail.

Perhaps we should get it in -- even in this state -- sooner rather than later, because then there is hope that the patch will not break as often as it did in the last months (I several times had to spend hours to days on making it compile again when trying to work on this again).

Comment 8

[Peter Weilbacher]

This is actually a blocker for future development on OS/2. Should keep Mike in the loop as well.

Comment 9

[Peter Weilbacher]

Attachment: Required build additions and easy widget changes [checked into trunk]

OK, to get us started, this is a first patch that I think can go in now. The files referenced as gfxOS2* are not actually contained in the patch yet but are the minimum ones that we will need and which also exist for Windows.

As usual, I request one review from a person familiar with OS/2 (mkaply) and one (misusing the sr field) from a person familiar with Thebes (vlad).

Comment 10

[Peter Weilbacher]

Comment on attachment 248446 [details] [diff] [review]
Required build additions and easy widget changes
[checked into trunk]

Oh, vlad's away. pavlov, perhaps you can review earlier?

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

You should implement |gfxOS2Platform::ResolveFontName| for text rendering.
The method returns error code. By this, gfxFont fails to enum the fonts for gfxFontGroup. You should use following code for temp (like as mac) :

aAborted = !(*aCallback)(aFontName, aClosure);
return NS_OK;

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 248446 [details] [diff] [review]
Required build additions and easy widget changes
[checked into trunk]

r=mkaply - what's the extra comment change in GFXAsurface.h (same order as cairo.h)

Comment 13

[Peter Weilbacher]

(In reply to comment #12)
> (From update of attachment 248446 [details] [diff] [review] [edit])
> r=mkaply - what's the extra comment change in GFXAsurface.h (same order as
> cairo.h)

In the first version I had the SurfaceTypeOS2 line before Quartz2 and then was searching for hours why I got the wrong surface type in some other code. Once I ordered it the same way as the enum _cairo_surface_type in cairo.h it worked... But I can remove it for the checkin.


(In reply to comment #11)
> You should implement |gfxOS2Platform::ResolveFontName| for text rendering.
> The method returns error code. By this, gfxFont fails to enum the fonts for
> gfxFontGroup. You should use following code for temp (like as mac) :
> 
> aAborted = !(*aCallback)(aFontName, aClosure);
> return NS_OK;

Thanks for the hint, will do that until I find something better.

Comment 14

[Andy Willis (abwillis)]

Finally got around to building with thebes.  SM (static built) was fine till Seamonkey.exe was being built and then I had to had TK_LIBS but no other build issues.  It isn't quite usable as there is no menu items (just dashes) and no mouse clicks are registered on either the dashes or in the url bar and cannot type in a URL (but it does render the home pages tabs, albeit slowly).  Not sure of the general state of Seamonkey with Thebes I then tried to build FF (same cvs pull) and get 6 errors:
g++ -o firefox.exe -I/usr/X11R6/include  -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -Zomf -pipe  -DNDEBUG -DTRIMMED -O2 -s  nsBrowserApp.o nsStaticComponents.o splashos2.res  -Zmap   -Zlinker /NOE -L../../dist/bin -L../../dist/lib ../../toolkit/xre/xulapp_s.lib -L../../staticlib -lthebes -L../../dist/bin -lmozjs -L../../dist/lib ../../dist/lib/xpcom.lib ../../dist/lib/xpcomcor.lib -L../../dist/lib -lplds4 -lplc4 -lnspr4 -ldl -lsocket -L../../dist/lib -lmozcairo -lmozlibpixman -LE:/MZFTCFGFT/lib -lmozft -lmzfntcfg  -lsocket -ldl -lm  ../../dist/lib/components/rwsos2.lib ../../dist/lib/components/pref.lib ../../dist/lib/components/mozuconv.lib ../../dist/lib/components/ucvmath.lib ../../dist/lib/components/i18n.lib ../../dist/lib/components/necko.lib ../../dist/lib/components/auth.lib ../../dist/lib/components/xpconect.lib ../../dist/lib/components/chardet.lib ../../dist/lib/components/mork.lib ../../dist/lib/components/cookie.lib ../../dist/lib/components/perms.lib ../../dist/lib/components/strgcmps.lib ../../dist/lib/components/rdf.lib ../../dist/lib/components/caps.lib ../../dist/lib/components/gkparser.lib ../../dist/lib/components/gkgfxthb.lib ../../dist/lib/components/imgicon.lib ../../dist/lib/components/imglib2.lib ../../dist/lib/components/gkplugin.lib ../../dist/lib/components/wdgtos2.lib ../../dist/lib/components/gklayout.lib ../../dist/lib/components/docshell.lib ../../dist/lib/components/embedcmp.lib ../../dist/lib/components/webbrwsr.lib ../../dist/lib/components/editor.lib ../../dist/lib/components/txmgr.lib ../../dist/lib/components/composer.lib ../../dist/lib/components/appshell.lib ../../dist/lib/components/xmlextra.lib ../../dist/lib/components/websrvcs.lib ../../dist/lib/components/ucdet.lib ../../dist/lib/components/oji.lib ../../dist/lib/components/access.lib ../../dist/lib/components/chrome.lib ../../dist/lib/components/mozfind.lib ../../dist/lib/components/intlapp.lib ../../dist/lib/components/windowds.lib ../../dist/lib/components/srchsvc.lib ../../dist/lib/components/appcomps.lib ../../dist/lib/components/tkautoc.lib ../../dist/lib/components/tkhstory.lib ../../dist/lib/components/satchel.lib ../../dist/lib/components/cmdlines.lib ../../dist/lib/components/tkitcmps.lib ../../dist/lib/components/spellchk.lib ../../dist/lib/components/pipboot.lib ../../dist/lib/components/pipnss.lib ../../dist/lib/components/pippki.lib ../../dist/lib/components/autocfg.lib ../../dist/lib/mozreg_s.lib ../../dist/lib/unicharutil_s.lib ../../dist/lib/ucvutil_s.lib ../../dist/lib/platlocale_s.lib ../../dist/lib/morkreader_s.lib ../../dist/lib/thebes.lib ../../dist/lib/gfxshared_s.lib ../../dist/lib/gkgfx.lib ../../dist/lib/jsj.lib  -L../../dist/lib -lmozpng -L../../dist/lib -lmozjpeg -L../../dist/lib -lmozz  -L../../dist/bin -L../../dist/lib -lcrmf -lsmime3 -lssl3 -lnss3 -lsoftokn3  -Zlinker /ST:0x100000 -Zlinker -PM:PM   browser.def
E:\cvs\work\mozilla\ffobj\dist\lib\thebes.lib(gfxOS2Fonts.o) : error LNK2025: WideCharToMultiByte(int, unsigned short const*, int, nsAutoBuffer<char, 1024>&, int&) : symbol defined more than once. Original definition in E:\cvs\work\mozilla\ffobj\dist\lib\gkgfx.lib(nsOS2Uni.o)
E:\cvs\work\mozilla\ffobj\dist\lib\thebes.lib(gfxOS2Fonts.o) : error LNK2025: OS2Uni::GetUconvObject(int, ConverterRequest) : symbol defined more than once. Original definition in E:\cvs\work\mozilla\ffobj\dist\lib\gkgfx.lib(nsOS2Uni.o)
E:\cvs\work\mozilla\ffobj\dist\lib\thebes.lib(gfxOS2Fonts.o) : error LNK2025: OS2Uni::FreeUconvObjects() : symbol defined more than once. Original definition in E:\cvs\work\mozilla\ffobj\dist\lib\gkgfx.lib(nsOS2Uni.o)
E:\cvs\work\mozilla\ffobj\dist\lib\thebes.lib(gfxOS2Fonts.o) : error LNK2025: MultiByteToWideChar(int, char const*, int, nsAutoBuffer<unsigned short, 1024>&, int&) : symbol defined more than once. Original definition in E:\cvs\work\mozilla\ffobj\dist\lib\gkgfx.lib(nsOS2Uni.o)
E:\cvs\work\mozilla\ffobj\dist\lib\thebes.lib(gfxOS2Fonts.o) : error LNK2025: _gConverterInfo : symbol defined more than once. Original definition in E:\cvs\work\mozilla\ffobj\dist\lib\gkgfx.lib(nsOS2Uni.o)
E:\cvs\work\mozilla\ffobj\dist\lib\thebes.lib(gfxOS2Fonts.o) : error LNK2025: OS2Uni::gCharsetManager : symbol defined more than once. Original definition in E:\cvs\work\mozilla\ffobj\dist\lib\gkgfx.lib(nsOS2Uni.o)

Comment 15

[Andy Willis (abwillis)]

 ifeq ($(OS_ARCH),OS2)
+ifndef MOZ_ENABLE_CAIRO_GFX
+CPPSRCS += os2/nsOS2Uni.cpp
+endif
 CPPSRCS += \
-        os2/nsOS2Uni.cpp \
         os2/nsPaletteOS2.cpp \
in the gfx/src/makefile.in allowed me to build FF.  I don't understand why it gave me an error with FF and not SM but the results were basically the same.  I was able to test a site that uses the transparent png by typing the url into a command prompt and then dragging and dropping it onto the browser and the graphic now loads quite fast though as reported the text was really slow.  I was hopeful that it would be faster here as I am using Doodle's Fontconfig in mzftcfg and hoped it would be faster but it doesn't appear that it is.
Paul Smedley may do a Pango port (if not I may take a look at it).  I was looking at it and there is a way to build pango so that it uses Cairo instead of having to port it to PM.  The only problem for us is that we would either have to build Pango as part of the build to use mozcairo or we would have to have a system cairo as well as the mozcairo.  

Comment 16

[Peter Weilbacher]

Yep, I just re-used the same class and method names from nsOS2Uni in the preliminary Thebes code. Perhaps that was not very clever as this is bound to fail when building statically, as you found out. But I think for now we can be content, if building a non-static Firefox works.

Comment 17

[Peter Weilbacher]

I made a tiny bit of progress:

- Changing the first argument of the WideCharToMultiByte() as called in gfxOS2Fonts.cpp from 850 to 0 helps to get the menu items displayed. (I should have looked at what that function actually does before using it...)

- That the window appears invisible / off screen when FF is started in a non-maximized manner is probably due to some coordinate problem. I see that nsWindow::Resize() and nsWindow::SetWindowPos() get called with negative (for y position) or zero arguments (for width/height). Don't know where that comes from yet. Just taking absolute numbers in those functions doesn't help.

- The reason that menus and popups don't appear is probably
     ###!!! ASSERTION: Computed overflow area must contain frame bounds: 
     'aNewSize.width == 0 || aNewSize.height == 0 ||
     aOverflowArea->Contains(nsRect(nsPoint(0, 0), aNewSize))',
     file X:/trunk/mozilla/layout/generic/nsFrame.cpp, line 5214
  At the moment I am not sure where these values originate, the debugger
  hangs the system before I get to the point. :-(

Comment 18

[Peter Weilbacher]

Comment on attachment 248446 [details] [diff] [review]
Required build additions and easy widget changes
[checked into trunk]

I think it's more important to get sr on real code patches instead of trivialities like this. I will check it in with the change implied by Mike's remark in comment 12.

Comment 19

[Peter Weilbacher]

Attachment: Further widget changes

These are basically the same widget changes I posted in this bug earlier. Mostly just to keep running. Printing will still be hosed of course, and menus still don't work, but I want this in for now instead of redoing this part over and over again... At the end of the patch I appended a hack to continue building through nsObjectFrame.cpp.
The changes to nsWindow::Destroy() and nsWindow::OnPaint() are mostly whitespace changes so that I could see to which level of ifs to add the Thebes stuff...

Comment 20

[Peter Weilbacher]

Attachment: New OS/2 files for Thebes

This updates the new files in gfx/thebes/{public,src}/ that I showed in attachment 248300 [details] [diff] [review] following the suggestion from comment 11 and the work needed to get up to speed since bug 333659 landed. The main things missing are support for non-CP850 characters and display of anything but the OS/2 default font.

If there are no grave errors in this patch I would like to get this in like this and then file follow-up bugs to step-by-step get a cairo build really working on OS/2.

Comment 21

[Peter Weilbacher]

Comment on attachment 253118 [details] [diff] [review]
New OS/2 files for Thebes

Vlad, even though this is about OS/2, perhaps you can take a look to see if I understood the structure of the classes involved.

Comment 22

[Peter Weilbacher]

Attachment: Further widget changes, v2 [checked into trunk]

Args! I hunted for _days_ now (see my postings to mozilla.dev.tec.gfx) why the menus did not appear and the window and other stuff appeared offscreen etc. It's simply that the bustage fix done to nsScreenManagerOS2 in Bug 353873 was not really clever (in hindsight). Simply changing nsScreenManagerOS2 :: ScreenForNativeWidget to a slightly less dummy version helps at lot...

Comment 23

[Mike Kaply [:mkaply]]

Comment on attachment 254222 [details] [diff] [review]
Further widget changes, v2 
[checked into trunk]

I'll trust you on this one. :)

Why are there so many white space changes?

Comment 24

[Peter Weilbacher]

(In reply to comment #23)
> Why are there so many white space changes?

Yes, I know, whitespace changes make patches unreadable and interfere with the blame. But nsWindow.cpp is such a mess of different styles that I had to clean up at least the functions that I was changing to see at which level to add code... (The parts that I patched now use the style given by the top two lines for Emacs and VIM.)

Comment 25

[Peter Weilbacher]

Comment on attachment 254222 [details] [diff] [review]
Further widget changes, v2 
[checked into trunk]

OK, just checked this into trunk (slightly adapted the new gfxOS2Surface line due to the changes done in bug 368910).

Comment 26

[Peter Weilbacher]

Attachment: New OS/2 files for Thebes, updated after bug 368910

OK, the old files didn't compile any more due to the recent changes from bug 368910.

Comment 27

[Stuart Parmenter]

Comment on attachment 254464 [details] [diff] [review]
New OS/2 files for Thebes, updated after bug 368910

maybe also get rid of GetDimensions/mWidth/mHeight from the surface and store a gfxIntSize and use GetSize like the rest of the surfaces that provide that?

all of thebes is 4 space indent (or should be).  Would you please reindent this to match?

Comment 28

[Peter Weilbacher]

Attachment: New OS/2 files for Thebes, v3

pavlov, thanks for the comments. As suggested, this patch updates the size stuff to use gfxIntSize and I hope I changed all indents to 4 spaces now.

Comment 29

[Walter Meinl]

Peter, as building with --enable-default-toolkit=os2 isn't working anymore since the checkin for bug #177805 (all non-thebes builds are broken now) I tried to build with cairo-os2 applying also your patch https://bugzilla.mozilla.org/attachment.cgi?id=254471 thats pending for rewiew.
I failed to build here
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp:277: error: type specifier 
   omitted for parameter `SpecialString'
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp:277: error: parse error 
   before `)' token
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp:279: error: `SpecialString' 
   was not declared in this scope
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp:279: error: parse error 
   before `,' token
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp:280: error: `SpecialString' 
   was not declared in this scope
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp:280: error: parse error 
   before `)' token
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp: In constructor `
   gfxWrapperTextRun::gfxWrapperTextRun(gfxOS2FontGroup*, const PRUint8*, 
   unsigned int, gfxTextRunFactory::Parameters*)':
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp:257: error: no matching 
   function for call to `gfxTextRun::gfxTextRun(
   gfxTextRunFactory::Parameters*&, int)'
../../../dist/include/thebes/gfxFont.h:383: error: candidates are: 
   gfxTextRun::gfxTextRun(gfxTextRun&)
../../../dist/include/thebes/gfxFont.h:690: error:                 
   gfxTextRun::gfxTextRun(gfxTextRunFactory::Parameters*)
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp: In constructor `
   gfxWrapperTextRun::gfxWrapperTextRun(gfxOS2FontGroup*, const PRUnichar*, 
   unsigned int, gfxTextRunFactory::Parameters*)':
D:/usr/src/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp:266: error: no matching 
   function for call to `gfxTextRun::gfxTextRun(
   gfxTextRunFactory::Parameters*&, int)'
../../../dist/include/thebes/gfxFont.h:383: error: candidates are: 
   gfxTextRun::gfxTextRun(gfxTextRun&)
../../../dist/include/thebes/gfxFont.h:690: error:                 
   gfxTextRun::gfxTextRun(gfxTextRunFactory::Parameters*)
make.exe[6]: *** [gfxOS2Fonts.o] Error 1
make.exe[6]: Leaving directory `D:/mozbuild/gfx/thebes/src'

To overcome this I // the lines containg SpecialString (lines 277, 279 and 280) of gfxOS2Fonts.cpp
The SpecialString was also removed from gfxWindowsFonts.cpp and gfxAtsuiFonts.cpp in bug #368799
The later errors are also due to the checkin for bug #368799
for other platforms (windows,mac) PR_TRUE was removed from the two lines
+        : gfxTextRun(aParams, PR_TRUE), mContext(aParams->mContext),
in gfx(Platform)Fonts.cpp
After cleaning these I could build thebes.dll with your patch.

One more comment about gfxOS2Fonts.h of the patch
the line
+    gfxOS2Font(const nsAString &aName, const gfxFontStyle *aFontStyle);
reads in other platforms
+    gfxWindowsFont(const nsAString& aName, const gfxFontStyle *aFontStyle);
look at the postion of the "&" after nsAString. It's different (by intention or a typo?)

Comment 30

[Peter Weilbacher]

(In reply to comment #29)

> I tried to build with cairo-os2 applying also your patch
> https://bugzilla.mozilla.org/attachment.cgi?id=254471 thats pending for rewiew.

Yeah, there is lots of work going on in Thebes and it will continue to break in a matter of days, even if I updated it now. That's why I want to get it in, even though the result is not pretty, in the hope that afterwards we will break less often.

> One more comment about gfxOS2Fonts.h of the patch
> the line
> +    gfxOS2Font(const nsAString &aName, const gfxFontStyle *aFontStyle);
> reads in other platforms
> +    gfxWindowsFont(const nsAString& aName, const gfxFontStyle *aFontStyle);
> look at the postion of the "&" after nsAString. It's different (by intention or
> a typo?)

I'm no C++ guru, but I don't think it matters where the space is, in gfxPangoFont it's the other way around, too. It should just signify passing that class by reference.

Comment 31

[Mike Kaply [:mkaply]]

Peter, you have my approval to checkin any Os/2 only changes without specific review.

At this point we're broken anyway, so we just need to get something in.

Comment 32

[Peter Weilbacher]

Comment on attachment 254471 [details] [diff] [review]
New OS/2 files for Thebes, v3

OK, thanks. Cancelling review, there isn't much OS/2 specific stuff in there yet anyway...

Comment 33

[Peter Weilbacher]

OK, checked in attachment 254471 [details] [diff] [review] with the changes suggested by wuno and the addition of the TEXT_IS_8BIT flag in gfxOS2FontGroup::MakeTextRun.

This should now at least get us building again, even though there are still lots of problems. I mark this fixed and instead open a tracking bug for Thebes on OS/2.