83242 - Add GC-cache to Xprint module

Status: VERIFIED FIXED

(Core Graveyard :: Printing: Xprint, enhancement, P3)

Description

[Roland Mainz]

Xprint does not have a GC-cache yet, that slows down it a bit. May also be
usefull to keep code in sync with Xlib-toolkit...

Comment 1

[Roland Mainz]

Swapping owner<-->QA.
Adding dependicy to bug 66082 ("add gc-cache to xlib port")...

Comment 2

[Roland Mainz]

Accepting, setting milestone...

Comment 3

[Roland Mainz]

ToDo:
- Fix "[Bug 86267] Xlib: form checkboxes are not drawn" in Xprint module
         

Comment 4

[Roland Mainz]

Adding dependicy to bug 80224 ("Xlib-toolkit's nsFontMetricsXlib.cpp is missing
tons of nsFontCharSetMap mappings...") which fixes tons of i18n issues in
Xlib-toolkit land...

Comment 5

[Roland Mainz]

Attachment: Patch for 2001-06-16-08-trunk + patch 38719 for bug 80224

Comment 6

[Roland Mainz]

ToDo:
- Add API to nsFontMetricsXlib.cpp to release global vars
- Add assert for access to fonts before XpSetContext() was called (e.g.
accessing fonts without having a valid font context) - this is BAD and should be
avoided !!
- Sync to more changes in bug 80224
- More code cleanup

Comment 7

[Roland Mainz]

Attachment: Better patch for 2001-06-16-08-trunk + patch 38719 for bug 80224

Comment 8

[Roland Mainz]

pocemit:
Wanna r= the patch, please ?

Comment 9

[Roland Mainz]

Attachment: New patch for 2001-06-16-08-trunk + patch 38719 for bug 80224 - fixed issues digged-out by timeless

Comment 10

[pocemit]

As the majority of the code comes from Xlib, this looks good to me.
Use diff -uw though, so that all these spacing changes dont get included. :)

r=pocemit (if that means anything)

> +INCLUDES       += -I$(srcdir)/../xprint
extra tab, please fix

> +
> +  static PRPackedBool mPrintermode;
I am guessing you decided to go with gPrinterMode and make it static, and
this is not under USE_XPRINT, so I guess it should be removed


> +  mPrintContext =    nsnull;
> +  mContext          = nsnull;
should probably keep indentation consistent


-
+  
lots of these all over the place :) Next person after you is going to 
remove them and make another large patch. Also there is a lot of extra line
ending spaces.

Comment 11

[Roland Mainz]

Attachment: New patch for 2001-06-18-08-trunk + patch 38719 for bug 80224 to patch r=pocemit/timecop

Comment 12

[Roland Mainz]

s/patch/match/

Fixed all issues listed in pocemit's review except the space/indentation
stuff...

Requesting sr= - tor/blizzard ?

Comment 13

[kinmoz]

sr=[e-mail]

Comment 14

[Roland Mainz]

kin:
Thanks !

requesting a= drivers...

Comment 15

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

@@ -41,11 +41,12 @@
 
 CPPSRCS
	= \
 
	nsDeviceContextXP.cpp \
-
	nsFontMetricsXP.cpp \
+
	../xlib/nsFontMetricsXlib.cpp \
 
	nsRenderingContextXP.cpp \
 
	nsGfxFactoryXP.cpp \
 
	nsXPrintContext.cpp \
 
	../xlib/nsRegionXlib.cpp \
+
	../xlib/nsGCCache.cpp \
 
	$(NULL)
 


Isn't this going to mess up the build system in certain cases (like storage of
compile-generated dependencies)?  I remember the various hacks needed to allow
building of source files in 2 places for zlib/standalone, libjar/standalone, etc.

Comment 16

[Roland Mainz]

dbaron:
AFAIK no, works here with Sun Workshop and gcc builds and GTK+/Xlib-toolkits 
without problems (there may be a small risk for Xlib-toolkit port (but
nsFontmetricsXlib.o is build with the _same_ flags/defines in xlib/ and xprint/
dirs) - but it is not part of the default build and I am monitoring this. And
I'll fix that isse - see below).

And this stuff will be removed soon - shared code gets a new module - this is
bug 85527...

Comment 17

[cls]

dbaron's right, those cases that use ../xlib/ should be using VPATH as we
discussed on irc. 

Comment 18

[Roland Mainz]

cls:
If you could remember your solution... ;-(
I can fix this as part of bug 86291 if you want - please let this bug get it's
a= ...

Comment 19

[Roland Mainz]

cls:
Thanks.
Issue will be killed as part of bug 86291...

Requesting a= for this bug, please...

Comment 20

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

a=dbaron for trunk checkin (on behalf of drivers), conditional on fixing the
build system changes to cls's approval.  I really don't want to see something go
in that breaks builds (did you try srcdir and objdir builds?), and it shouldn't
be hard for you to fix this.

Comment 21

[Roland Mainz]

dbaron:
Agreed. bug 86291 will be fixed tomorrow - cls needs it urgendly for his
StaticBuild stuff. This issue should be fixed before friday (<= 2 days...)...

Thanks !

----

[e-mail] - wanna do the checkin, please ?

Comment 22

[cls]

Provided that the current patch builds in an objdir and I have Roland's
guarantee that the VPATH issue will be fixed in tomorrow's carpool, I have no
problem with the patch going in now.  If it doesn't build in an objdir most of
the tinderboxes will break so it should be tested before it is checked in.

Comment 23

[Roland Mainz]

cls wrote:
> I have Roland's guarantee that the VPATH issue will be fixed in tomorrow's 
> carpool

I've added this issue to the ToDo list in bug 86291...
And I always build my Mozilla (WS6U2, gcc both SPARC and x86) with:
% mkdir objdir; cd objdir
% ../configure myoptions

No problems here...

Comment 24

[Mike Kaply [:mkaply]]

I would prefer not to take ownership for checking this in if it has the 
possibility of breaking builds.

I can't test build it.

All the other patches I have checked in have been code that wasn't built by 
default.

Comment 25

[cls]

So anyway, after having a fruitless discussion with Roland on irc for the past
1/2 hr, does he or does he not have approval to check this patch in?  The
objectionable part that would need to be fixed with VPATH was partially fixed in
the static build landing this afternoon.

Comment 26

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

cls:  This has approval if you're happy with the build system stuff.

Comment 27

[Roland Mainz]

Patch has been checked-in by cls (THANKS !!), all staticbuild issues have been
solved in bug 86291.

making bug as FIXED.

Comment 28

[pocemit]

verifying