163736 - consolidate gfx2 into gfx

Status: RESOLVED FIXED

(Core Graveyard :: Image: Painting, defect, P2)

Description

[Alec Flett]

a comment on a newsgroup made me wonder why we need gfx2.dll, gkgfxwin.dll, and
gkgfx.dll - I'm going to try to conslidate them in some way. some stuff links
against gkgfx.dll though, so I'm not sure where I'll go with that. I think this
stuff varies slightly from platform to platform too.

More than likely I'll combine gfx2 into gkgfx, since gfx2 is only 24k and half
of that is probably factory code.

Comment 1

[Alec Flett]

Attachment: combine gfx2 into gfx

ok, here is an initial strawman for the GFX combination. This eliminates the
mozilla/gfx2 directory from the build as well.

First a little background: gfx is actually divided into two seperate libraries.

* gkgfx.dll is built from gfx/src and lives in the bin directory, and a number
of other components (gkview, gklayout, etc) link against it in order to import
stuff like nsRect. 
* gkgfxwin.dll (or libgfx_gtk.so, etc) is built from gfx/src/<platform> and
lives bin/components. 

This provided an interesting challenge, as gfx2 is really just a single object
(in gfxImageFrame.cpp) that needs to be in the components directory..

So what I did was create gfx/src/shared which is a shared static library that
all the platforms should link against, and put gfxImageFrame in there.

As a part of this, I moved nsRenderingContextImpl.cpp into gfx/src/shared
because the component dll was actually the only consumer, and this allowed us
to stop exporting nsRenderingContextImpl which allows the linker to cull unused
methods/data/etc. This should reduce our code footprint a bit as well.

You'll also note that I moved some color stuff into layout/html/style/src -
these were a few helper routines that gfx was exporting but was only consumed
by layout. Again, this allows the linker to optimize use of these methods, cull
unused code, and stop exporting these methods again reducing code footprint.

I'm going to need leaf to fix bug 166874 in order to actually land this patch,
because he will move a bunch of files from gfx2 into gfx, so that we keep CVS
version history around.

This is a strawman patch that still needs to be built and tested on Mac and
unix.
It also doesn't include any packaging changes. I will post updated patches as I
work through each platform. I'll also work with mkaply to see about getting
OS/2 landed at the same time.

Comment 2

[Alec Flett]

Attachment: mac-specific project patch

this is a supplement to the above patch

Comment 3

[Alec Flett]

Attachment: combine gfx2 into gfx v1.0

ok, here is my first shot at a complete patch.
This patch combines everything on all _4_ platforms (yes, OS/2 included, though
it hasn't been tested) as well as packaging updates, etc.

Can I get some reviews? pav?

Comment 4

[Alec Flett]

adding rbs and blizzard for possible super reviews - guys I'm trying to get this
in for 1.2alpha, if possible.. overall this drops our footprint by a few k, as
well as knocking one more DLL off our list (which should improve startup time,
as well as the spacial locality of the gfx code as well!)

The patch isn't that big, it just touches a bunch of makefiles, a bit of source,
etc...

Comment 5

[Alec Flett]

argh, trying that again.. adding rbs and blizzard for super reviews (see my
previous comment)

Comment 6

[rbs]

Comment on attachment 98179 [details] [diff] [review]
combine gfx2 into gfx v1.0

+		     <PATH>gfx2types.idl</PATH>
+		     <PATH>gfxtypes.idl</PATH>

What about merging gfx2types.idl into gfxtypes.idl to retain a single file?

===================================================================
RCS file: /cvsroot/mozilla/gfx/src/os2/Makefile.in,v
+		  imglib2 \

Intentional dependency?

===================================================================
RCS file: /cvsroot/mozilla/gfx/src/Makefile.in,v
retrieving revision 1.103
diff -u -p -r1.103 Makefile.in
--- gfx/src/Makefile.in 30 Aug 2002 03:31:55 -0000	1.103
+++ gfx/src/Makefile.in 6 Sep 2002 19:02:16 -0000
@@ -44,7 +44,7 @@ REQUIRES	= xpcom \
		  unicharutil \
		  $(NULL)

-DIRS	     =
+DIRS	     = shared

Since the shared files are used everywhere, why not put them straight 
in gfx/src rather than creating another subdirectory.

Comment 7

[Alec Flett]

rbs: thanks for the review.. responses:
- yep, that's an intentional dependency, required in order to build
- the reason I couldn't put it directly into gfx/src is that the DLL build from
gfx/src is not a component DLL, and thus does not have a factory where I can
plug in the gfxImageFrame. If you look at the way GFX is structured, the DLL
that gets dumped into the components directory comes out of the
gfx/src/<platform> directory, and up until now there was no code that was
"shared" between platforms
- I like the idea of combining the 2 IDL files, but do you mind if I wait until
after this patch to do this? I'm having leaf copy the files in the CVS
repository so that we can maintain revision information, and they will be coming
over exactly as they exist in gfx2/public

Comment 8

[rbs]

> the DLL that gets dumped into the components directory comes out of the
> gfx/src/<platform> directory, and up until now there was no code that was
> "shared" between platforms

Actually, there are bits of code that are shared. But what you said reminded me
of a trick that is currently used and which might go away. If you look at
mozilla/gfx/src/windows/Makefile.in, you will see that it copies
a .cpp file to the gfx/src/<platform> before compiling:

export:: ../nsCompressedCharMap.cpp
ifeq ($(OS_ARCH),WINNT)
	$(INSTALL) $(srcdir)/../nsCompressedCharMap.cpp $(srcdir)
else
	$(INSTALL) $(srcdir)/../nsCompressedCharMap.cpp .
endif

Now that you have that shared sub directory, perhaps all these copying could be
avoided by consolidating into what you have started.

So there might be two possible items later on: merge the two IDLs and
consolidate the other bits.

Comment 9

[rbs]

Comment on attachment 98179 [details] [diff] [review]
combine gfx2 into gfx v1.0

sr=rbs

Comment 10

[Alec Flett]

oh man, I hadn't picked that up. Yuck. Yeah, I'll definitely consolidate that
into shared/ after this has landed. Thanks for bringing it to my attention..

Comment 11

[Stuart Parmenter]

Comment on attachment 98179 [details] [diff] [review]
combine gfx2 into gfx v1.0

r=pavlov

Comment 12

[Sergei Dolgov]

Patch for BeOS nsGFXFactory seems trivial

Index: nsGfxFactoryBeOS.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/beos/nsGfxFactoryBeOS.cpp,v
retrieving revision 1.21
diff -u -r1.21 nsGfxFactoryBeOS.cpp
--- nsGfxFactoryBeOS.cpp	13 Jul 2002 00:17:51 -0000	1.21
+++ nsGfxFactoryBeOS.cpp	10 Sep 2002 21:53:28 -0000
@@ -37,6 +37,7 @@
  * ***** END LICENSE BLOCK ***** */
 
 #include "nsIGenericFactory.h" 
+#include "gfxImageFrame.h
 #include "nsIModule.h" 
 #include "nsCOMPtr.h" 
 #include "nsGfxCIID.h" 
@@ -72,6 +73,7 @@
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsPrinterEnumeratorBeOS) 
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsPrintOptionsBeOS)
 NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsPrintSession, Init)
+NS_GENERIC_FACTORY_CONSTRUCTOR(gfxImageFrame)
  
 // our custom constructors 
  
@@ -186,6 +188,10 @@
   	NS_PRINTSETTINGSSERVICE_CID,
   	"@mozilla.org/gfx/printsettings-service;1",
   	nsPrintOptionsBeOSConstructor },
+  { "windows image frame",
+    GFX_IMAGEFRAME_CID, 
+    "@mozilla.org/gfx/image/frame;2", 
+    gfxImageFrameConstructor, },  	
   { "Print Session",
     NS_PRINTSESSION_CID,
     "@mozilla.org/gfx/printsession;1",

but maybe it should be accompanied with changes in packager (probably under
unix), but i didn't find there mention of libgfx_beos.so and libgfx2.so

Comment 13

[Alec Flett]

mozilla 1.2alpha is more or less done. moving mozilla 1.2alpha bugs out to
mozilla 1.2beta

Comment 14

[Alec Flett]

el arreglo aterrizado

Comment 15

[hacker formerly known as [e-mail]]

We've still got all manners of bustage on the Ports.  Is gfxshared_s supposed to
be linked into gkgfx or do we need to explicitly link to gfxshared_s?

 

Comment 16

[Alec Flett]

its supposed to be linked against the platform component gfx dll, i.e. not
libgkgfx.so, but libgkgfx_<toolkit>.so - its a static library as well, so its
static linkage

I'll take a look at ports.

Comment 17

[hacker formerly known as [e-mail]]

Hrm.  It appears as though it will also need to be linked against gfxps &
gfxxprint which should be the only other libs that use classes derived from
nsRenderingContextImpl.  This also means that we'll need to explicitly link
against gfxshared_s for static builds.

Comment 18

[Alec Flett]

well, I fixed the gfxps issue last night, and I think I fixed the gfxxprint
issue just now.. and I think I also fixed the BeOS issue, so hopefully we're
just down to  the static build.

Comment 19

[hacker formerly known as [e-mail]]

I just checked in a fix for the static build.  You need to add EXPORT_LIBRARY to
Makefile.in so that the library will be automatically linked into the final binary.

Comment 20

[Darin Fisher]

this seems to have broken the mach-o build (see planetoid on the tinderbox ports
page).

Comment 21

[Roland Mainz]

Somehow you forgot the Xlib code in gfx/src/xlib again (it can't be that _hard_
to keep it in sync since most of the changes to gfx/src/gtk can be applied to
gfx/src/xlib, too) ;-(

Comment 22

[Roland Mainz]

Attachment: Fix for the Xlib bustage for 2002-09-14-08-trunk

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

I have just checked in the xlib bustage attachment.

Comment 24

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(What about the photon and qt gfx ports?  I'm guessing xprint and ps aren't
broken since nobody complained, though.)

Comment 25

[Roland Mainz]

David Baron wrote:
> (What about the photon and qt gfx ports? 

We do not have tinderboxen for them - both have to be checked manually... ;-(
I guess they all need a similar patch as for the Xlib bustage fix...

For "Qt" I guess it's still suffering from other bustage but AFAIK we having a
new module owner and may see some light at the end of the tunnel for that
port... :-))

> I'm guessing xprint and ps aren't broken since nobody complained, though.)

No, they are working. We would have a couple bugs in bugzilla if one of them
would be busted...

Comment 26

[Alec Flett]

roland - I do what I can, I covered 4 platforms initially and have thus far
covered 6, including MachO, BeOS and OS/2 - its not suprising that I missed one.
Gimme a break :)