90380 - Get rid of |#ifdef _IMPL_NS_XPRINT|

Status: VERIFIED FIXED

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

Description

[Roland Mainz]

RFE: get get of the use of _IMPL_NS_XPRINT (which was introduced for staticbuild
support) in places where it is possible. Most use of that CPP symbol is
unneccesary and can be removed.

Comment 1

[Roland Mainz]

Swapping QA<-->Owner

Comment 2

[Roland Mainz]

Accepting bug, CC:'ing Xlib-toolkit folks.

Comment 3

[Roland Mainz]

Now we're too close to 0.9.4-freeze to get this fixed in 0.9.4. Retargeting to
0.9.5 - but I cannot promise that I have enougth time to get it fixed in that
milestone.

----

ToDo:
Replace _IMPL_NS_XPRINT with (at least) two defines: One for stuff mandatory for
static build stuff and one #define to seperate the differences between Xlib and
Xprint module. I have to investigate whether we need more categories or not...

Comment 4

[Roland Mainz]

Attachment: Prototype patch, requires patch from bug 85388

Comment 5

[Roland Mainz]

Filed prototype patch, the CPP symbol |_IMPL_NS_XPRINT| is not completely
"gone".

ToDo:
Make StaticBuild patch and check whether the patch needs adjustments or not ...

Comment 6

[Roland Mainz]

s/Make StaticBuild patch and/Make StaticBuild build and/

Comment 7

[Roland Mainz]

Attachment: New patch for 2001-09-01-08-trunk (requires patch from bug 85388)

Comment 8

[Roland Mainz]

Filed patch, this one works with StaticBuild (Solaris 7 SPARC/WS6U2FCS).

Requesting r=/sr= ...

Comment 9

[dcone (gone)]

you use the "#ifdef USE_XPRINT " alot, is there a way around using all those 
ifdef's.  I thought this was an XPRINT only modulule?

Comment 10

[Roland Mainz]

dcone:
|#ifdef USE_XPRINT| is required because Xprint is not available on all unix
platforms (for example IRIX). Therefore we need a way to turn-off matching code
if the Xprint module is not build (e.g. libXp.so is not present or someone
explicitly sets the configure option --disable-xprint ).

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Why remove NS_INIT_REFCNT()?

In this:
+     rv = ((nsDrawingSurfaceXlibImpl*)surface)->Init(
Should that be an NS_STATIC_CAST?  Ditto these:
+    ((nsDrawingSurfaceXlibImpl *)mRenderingSurface)->Init(mXlibRgbHandle, 
+    ((nsDrawingSurfaceXlibImpl *)surf)->Init(mXlibRgbHandle,

I don't understand why you had to replace all the aSurface->GetBlah()s with
xxlib_rgb_get_blah()'s, though I don't see any harm in it.  What was the
reasoning?

+  mRenderingSurface = mPrintContext;
+  NS_ADDREF(mRenderingSurface);

Why is this not an nsCOMPtr? (and the NS_IF_RELEASE or whatever could go away as
well).

Seeing these raises warning flags - direct calls to Release/AddRef???  Also,
what happens in GetGC if mGC is nsnull?  I don't know if the logic allows that
to occur, but my assumption is that it might be possible.  What about
SetGC(nsnull)?

+    if (mGC)
+    {
+      mGC->Release();
+      mGC = nsnull;
+    }

+  NS_IMETHOD_(xGC *)      GetGC() { mGC->AddRef(); return mGC; }
+  void                    SetGC(xGC *aGC) { mGC = aGC; mGC->AddRef(); }

Comment 12

[Roland Mainz]

> Why remove NS_INIT_REFCNT()?

The superclass does that job... it is not required to do it in each subclass
constructor, too - right ? :-)

> In this:
> +     rv = ((nsDrawingSurfaceXlibImpl*)surface)->Init(
> Should that be an NS_STATIC_CAST?  Ditto these:
> +    ((nsDrawingSurfaceXlibImpl *)mRenderingSurface)->Init(mXlibRgbHandle, 
> +    ((nsDrawingSurfaceXlibImpl *)surf)->Init(mXlibRgbHandle,
> 
> I don't understand why you had to replace all the aSurface->GetBlah()s with
> xxlib_rgb_get_blah()'s, though I don't see any harm in it.  What was the
> reasoning?

I simply cleaned-up the API a little bit. All the
Get[Display|Screen|Visual|Depth|etc.|etc.]() information is always the same as
in the |XlibRgbHandle|. It is usefull to cache the vars in the objects to avoid
unneccesary function calls... but IMHO there is no reason to add/keep extra APIs
for that stuff... |XlibRgbHandle| does the job already ...

> +  mRenderingSurface = mPrintContext;
> +  NS_ADDREF(mRenderingSurface);
> 
> Why is this not an nsCOMPtr? (and the NS_IF_RELEASE or whatever could go 
> away as well).

Good question... because I did not change that (yet) ? Most
nsDeviceContext[GTK|PS|Win|Mac|Qt|Photon|etc.] is not completely moved over to
nsCOMPtr land yet... this is only another thing which should be fixed some day.

> Seeing these raises warning flags - direct calls to Release/AddRef???  Also,
> what happens in GetGC if mGC is nsnull?  I don't know if the logic allows that
> to occur, but my assumption is that it might be possible.  What about
> SetGC(nsnull)?
> 
> +    if (mGC)
> +    {
> +      mGC->Release();
> +      mGC = nsnull;
> +    }
> 
> +  NS_IMETHOD_(xGC *)      GetGC() { mGC->AddRef(); return mGC; }
> +  void                    SetGC(xGC *aGC) { mGC = aGC; mGC->AddRef(); }

No... that's the GC-cache which does not inherit from nsISupports... don't
worry... see mozilla/gfx/src/xlib/nsGCCache.h ... :-)

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Ok, I guess that answers all my quibbles except for the NS_STATIC_CAST
question.  Deal with that quibble, and for what it's worth r=rjesup for general
issues.
You'll need other reviewers.

Comment 14

[jag (Peter Annema)]

+nsDrawingSurfaceXlibImpl::Init(XlibRgbHandle *aXlibRgbHandle,
                            Drawable  aDrawable, 
                            xGC        *aGC) 

Line up your arguments. Repeat for all places you added |Impl|.


+      rv = ((nsDrawingSurfaceXlibImpl*)surface)->Init(

Why not just make |surface| of type nsDrawingSurfaceXlibImpl* and be done with
it?


-#ifdef _IMPL_NS_XPRINT
 #define XPRINT_FONT_HACK 1
-#endif /* _IMPL_NS_XPRINT */

Please put #ifdef USE_XPRINT around this #define to clarify when this define is
actually relevant.

Looks okay to me otherwise, r=jag. Think you can find someone who knows this
code slightly better to have a look too?

Comment 15

[Roland Mainz]

Attachment: New patch for 2001-09-05-08-trunk to match r=jesup,r=jag

Comment 16

[Roland Mainz]

Requesting r= from [e-mail] for the Makefile.in change ...

Comment 17

[Donnie Cranford]

Ok I have used this patch on my Solaris 8 tree using Forte 6 UD2 and it looks
good to me and works just fine...

r=dcran

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

+  NS_IMETHOD_(Drawable)   GetDrawable();
+  NS_IMETHOD_(XlibRgbHandle) *GetXlibRgbHandle();
+  NS_IMETHOD_(xGC *)      GetGC();

Don't use NS_IMETHOD_(type) in interfaces.  Proper XPCOM methods return
nsresults and use out parameters.

Could you explain the reference counting and ownership system here for me?  Why
are you keeping a weak reference to the nsIDeviceContext (in
nsFontMetricsXlib::mDeviceContext)?

+nsRenderingContextXp::nsRenderingContextXp()
+  : nsRenderingContextXlib()
+{
+  mPrintContext     = nsnull;
+}

...and if you do switch to a strong reference and an nsCOMPtr, you should use an
initializer rather than explicit assignment in the constructor.

+  NS_ADDREF(mRenderingSurface);

Is there a reason you're using manual refcounting?  Unless it's a highly
exceptional case, you should be using nsCOMPtr.

+  NS_IMETHOD         GetDepth(PRUint32 &depth) 
+                     { depth = xxlib_rgb_get_depth(GetXlibRgbHandle()); return
NS_OK; }

I don't see that the reviewers' comments about C-style casts have been reviewed.
 Please respond to them, either in the patch or in a bug comment, before
submitting for re-super-review.

Comment 19

[Roland Mainz]

> +  NS_IMETHOD_(Drawable)   GetDrawable();
> +  NS_IMETHOD_(XlibRgbHandle) *GetXlibRgbHandle();
> +  NS_IMETHOD_(xGC *)      GetGC();
> 
> Don't use NS_IMETHOD_(type) in interfaces.  Proper XPCOM methods return
> nsresults and use out parameters.

Fixed.

> Could you explain the reference counting and ownership system here for me?  
> Why are you keeping a weak reference to the nsIDeviceContext (in
> nsFontMetricsXlib::mDeviceContext)?

nsFontMetricsXlib::mDeviceContext is a reference back to the device for which
this font is for. Per definition, the DeviceContextImpl keeps (or "owns") all
these fonts in it's font cache - which gets cleaned-up&destroyed in the
DeviceContextImpl destructor. Therefore the DeviceContextImpl-based object
always "lives" (little bit) longer than the nsFontMetrics*-objects used for that
device.

> +nsRenderingContextXp::nsRenderingContextXp()
> +  : nsRenderingContextXlib()
> +{
> +  mPrintContext     = nsnull;
> +}
> 
> ...and if you do switch to a strong reference and an nsCOMPtr, you should use 
> an initializer rather than explicit assignment in the constructor.

Fixed.

> +  NS_ADDREF(mRenderingSurface);
> 
> Is there a reason you're using manual refcounting?  Unless it's a highly
> exceptional case, you should be using nsCOMPtr.

No reason. That's simply some sort of "historical" stuff which wasn't updated
yet... 
There is still a lot of stuff which still waits to be switched-over from manual
refcounting to the nsCOMPtr world.
Fixed; |mRenderingSurface| and |mOffscreenSurface| are now guarded by nsCOMPtr's

> +  NS_IMETHOD         GetDepth(PRUint32 &depth) 
> +                     { depth = xxlib_rgb_get_depth(GetXlibRgbHandle()); 
> return NS_OK; }
> 
> I don't see that the reviewers' comments about C-style casts have been 
> reviewed.
>  Please respond to them, either in the patch or in a bug comment, before
> submitting for re-super-review.

I removed the cast.
Fixed.

Comment 20

[Roland Mainz]

Attachment: New patch for 2001-09-08-08-trunk per shaver's comments

Comment 21

[Roland Mainz]

Requesting sr= 
(I am aware that I still miss a r=[e-mail] - but I think it can be "done"
in parallel to the sr= assuming the Makefile.in-part won't be touched anymore).

Comment 22

[Christopher Blizzard (:blizzard)]

Comment on attachment 48804 [details] [diff] [review]
New patch for 2001-09-08-08-trunk per shaver's comments

>diff -r -u original/mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp
>--- original/mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp	Wed Aug 29 01:12:50 2001
>+++ mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp	Sun Sep  9 21:24:56 2001
>@@ -65,7 +65,6 @@
>   : DeviceContextImpl()
> {
>   PR_LOG(DeviceContextXlibLM, PR_LOG_DEBUG, ("nsDeviceContextXlib::nsDeviceContextXlib()\n"));
>-  NS_INIT_REFCNT();
>   mTwipsToPixels = 1.0;
>   mPixelsToTwips = 1.0;
>   mNumCells = 0;

Why is that being removed?  Does that leave things uninitialized?

>-  nsDrawingSurfaceXlib *surf = (nsDrawingSurfaceXlib *)mSurface;
>+  nsIDrawingSurfaceXlib *surf = (nsIDrawingSurfaceXlib *)mSurface;

There are lots of these instances in the file.  If you're going to touch them anyway
then please convert them to NS_STATIC_CAST() macro calls.

Other than that sr=blizzard

Comment 23

[Roland Mainz]

blizzard wrote:
> >diff -r -u original/mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp 
> mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp
> >--- original/mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp      Wed Aug 29 
> 01:12:50 2001
> >+++ mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp       Sun Sep  9 21:24:56 
> 2001
> >@@ -65,7 +65,6 @@
> >   : DeviceContextImpl()
> > {
> > PR_LOG(DeviceContextXlibLM,PR_LOG_DEBUG, ("nsDeviceContextXlib :: 
> nsDeviceContextXlib()\n" ));
> >-  NS_INIT_REFCNT();
> >   mTwipsToPixels = 1.0;
> >   mPixelsToTwips = 1.0;
> >   mNumCells = 0;
> 
> Why is that being removed?  Does that leave things uninitialized?

The superclass (DeviceContextImpl) already does that job. That's why I removed
it (I saw that in OS/2 code and asked mkaply):

> >-  nsDrawingSurfaceXlib *surf = (nsDrawingSurfaceXlib *)mSurface;
> >+  nsIDrawingSurfaceXlib *surf = (nsIDrawingSurfaceXlib *)mSurface;
> 
> There are lots of these instances in the file.  If you're going to touch them 
> anyway then please convert them to NS_STATIC_CAST() macro calls.

Fixed in the new patch...

> Other than that sr=blizzard

Thanks!

Comment 24

[Roland Mainz]

Attachment: New patch for 2001-09-08-08-trunk per blizzard's&&cls's comments

Comment 25

[cls]

Same problem as last time:

../../../../mozilla/gfx/src/xlib/nsRenderingContextXlib.cpp:38: imgIContainer.h: 
No such file or directory


Configuring using: --with-xlib --with-qt --with-qtdir=/usr/lib/qt-2.2.4

Build with xlib & xprint enabled.

Comment 26

[jag (Peter Annema)]

Dauphin: was that a dep tracking build? I suspect any of the REQUIRES lines in
the dirs we normally don't build are fairly dead.

Comment 27

[Roland Mainz]

Attachment: New patch for 2001-09-08-08-trunk to get rid of this mess...

Comment 28

[Roland Mainz]

> Same problem as last time:
> 
> ../../../../mozilla/gfx/src/xlib/nsRenderingContextXlib.cpp:38: 
> imgIContainer.h: 
> No such file or directory
> Configuring using: --with-xlib --with-qt --with-qtdir=/usr/lib/qt-2.2.4
>
> Build with xlib & xprint enabled.

I added "imglib2" to gfx/src/xlib/Makefile.in 's REQUIRES line which should
(hopefully) fix this...

BTW: I usually build by Xlib-toolkit builds with "configure
--enable-toolkit=xlib"

Comment 29

[Roland Mainz]

Comment on attachment 49200 [details] [diff] [review]
New patch for 2001-09-08-08-trunk to get rid of this mess...

Someone touched REQUIRES line of gfx/src/xlib/Makefile.in between 2001-09-08-08-trunk and todays tip. I'll file a new patch ...

Comment 30

[Roland Mainz]

Attachment: patch for 2001-09-08-08-trunk + fix for gfx/src/xlib/Makefile.in bustage

Comment 31

[Roland Mainz]

OK... thanks to jaggernauts's help I did the following to test the patch:
1. Set MOZ_TRACK_MODULE_DEPS=1 in config/autoconf.mk
2. Cleaned dist/
3. (cd gfx; gmake clean) + removed all remaining files (*/Makefile) from gfx/
4. rebuild the tree with "export MOZ_TRACK_MODULE_DEPS=1; nice gmake"

Results:
Build works :-)

Comment 32

[jag (Peter Annema)]

cls: this should fix your problem, I think.

r=jag on the Makefile.in change.

Comment 33

[Roland Mainz]

Fix checked in
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1000387560&maxdate=1000387807&who=timeless%25mac.com).

Marking bug as FIXED.

Comment 34

[Masaki Katakai]

Roland or cls,

Can you verify this for me? Or could you give
the instruction how to verify?

Thanks.

Comment 35

[Roland Mainz]

Verifying - the evil |_IMPL_NS_XPRINT| cpp symbol is gone... :)