81311 - nsFontCache cannot be shared between multiple toolkits

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Roland Mainz]

I cannot use nsFontCache (see nsDeviceContext.cpp) in Xprint or PostScript
module because it always creates nsFontmetrics* objects from parent
devicecontext classes instead of the classes in the print device context.

Solution:
Implementation of nsFontCache::CreateFontMetricsInstance().
Default would be
-- snip --
nsresult nsFontCache :: GetDeviceContext(nsIFontMetrics* fm&afm)
{
  return nsComponentManager::CreateInstance(kFontMetricsCID,  nsnull, 
NS_GET_IID(nsIFontMetrics), (void **)&fm);
}
-- snip --

Then I would be able to create a subclass of nsFontCache which overrides this
function with it's own code...

----

mkaply, wanna do the checkin when patch got r=/sr= ?

Comment 1

[Roland Mainz]

Attachment: Patch for 2001-05-14-latest-trunk

Comment 2

[Roland Mainz]

Accepting bug.
Filed patch.
Set milestone to 0.9.2
This bug blocks bug 80224... ;-(

Question: 
Can I create a subclass from a class which is only defined as forwarded class
(see nsDeviceContext.h) ?

dbaron, wanna give me your r=, please ?

Comment 3

[Chad Austin]

Question: 
Can I create a subclass from a class which is only defined as forwarded class
(see nsDeviceContext.h) ?

To derive from a class it has to be defined, not just declared.  In 
simpler words, no.  :)

Comment 4

[Roland Mainz]

> To derive from a class it has to be defined, not just declared.  In 
> simpler words, no.  :)

xx!!@@§§xx... ;-(
Seems I have to file a new patch which moves the definition of the class into a
header... ;-(

And comments about the current patch ?

Comment 5

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

Additionally, if you want a subclass to be able to override it so that when it's
called from the base class you get the subclass's method (which is what you want
here), you should make it virtual.

Comment 6

[Roland Mainz]

Attachment: (New) patch for 2001-05-16-08-trunk

Comment 7

[Roland Mainz]

Created new patch based on dbaron's comments. Works on Solaris/Linux with Sun
Workshop 6U2EA2 and gcc 2.95.x...

Anyone wanna give me a r= for this beast, please ?

Comment 8

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

r=dbaron, although I think you should:
 * change the |nsIFontMetrics*&| to |nsIFontMetrics**|
 * fix the wacky indentation of what you added to nsDeviceContext.h
 * check with the owner of nsDeviceContext.cpp (is that [e-mail] ?
   who owns font code now that erik has left?)

Comment 9

[Roland Mainz]

> check with the owner of nsDeviceContext.cpp (is that [e-mail] ?
> who owns font code now that erik has left?)

Uhm... any idea how I can figure that out ?
Anyway... I filed a request for sr= to reviewers and added kmcclusk to the CC:
field...

Comment 10

[Roland Mainz]

Still no updates here... ;-(
Going to ping drivers...

Comment 11

[Roland Mainz]

retargeting to 0.9.3 per asa's email to all bug owners...

Comment 12

[Scott Collins]

I agree with dbaron's comments of 2001-05-20 09:23.  After you've handled that,
sr=scc.

Comment 13

[Roland Mainz]

Thanks !
I'll file a new patch...

Comment 14

[Roland Mainz]

Attachment: New patch for 2001-07-16-08-trunk

Comment 15

[Roland Mainz]

Filed all new patch.

The new patch implements the "subclass-able"-nsFontCache class, fixes related
some stuff in DeviceContextImpl and switches Xprint, PostScript and Xlib-port
(for speed and testing... :-) from home-grown+buggy font cache code to new
nsFontCache(Xp|PS|Xlib) class in one step.
Code has been tested from hell and back... works perfectly... :-)

Requesting r=, sr=

Comment 16

[timeless]

the code changes dbaron wanted look fine, 

one too many spaces after +  NS_IMETHOD in this block
+  NS_IMETHOD    GetSystemAttribute(nsSystemAttrID anID, SystemAttrStruct * 
aInfo) const;
   NS_IMETHOD   EndPage(void);
...
+  NS_IMETHOD    CreateFontCache();
 
If you're going to do 
+  //NS_ADDREF(aContext);
could you add a comment explaining why? perhaps //XXX bug Foo means that 
print contexts aren't refcounted.

I think this would be a better change for the license thing (it's relatively 
consistent w/ another change you made elsewhere, [otherwise beconsistent about 
"* - Roland","*   Roland","* Roland" ;-) ]

  * Contributor(s): 
+ * Roland Mainz <[e-mail]>
+ *
  * This Original Code has been modified by IBM Corporation. Modifications made 
by IBM 

I don't think you need to include your name here:
+/* PostScript and Xprint module may override this method to create 

you're still adding |if(| w/o a space :-(

() isn't needed in placeS like:
+      NS_ADDREF((*fm));

because of:
1103 #define NS_ADDREF(_ptr) \
1104   NS_LOG_ADDREF_CALL((_ptr), (_ptr)->AddRef(), __FILE__, __LINE__)

for things like
+    return (...)?(NS_OK):(NS_ERROR_OUT_OF_MEMORY);
drop the ()s around NS_...; (you might use spaces...)

fix these for r=timeless (although i think you have an r=dbaron ...)

Comment 17

[Roland Mainz]

Attachment: Patch for 2001-07-16-08-trunk to match r=timeless ...

Comment 18

[Roland Mainz]

Sorry... wrong patch... the current only only diffs orig/gfx/src to gfx/src ...
going to file the right patch...

Comment 19

[Roland Mainz]

Attachment: (full) patch for 2001-07-16-08-trunk to match r=timeless

Comment 20

[Roland Mainz]

Filed new patch to match r=timeless, requesting sr= and a= ...

Comment 21

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

Comments on testing:

    How carefully have you tested postscript printing in addition to
    XPrint?

    Have you had someone test these changes on Windows?  (Linkage changes
    should be tested on Windows.)

Comments on code:

    Could you uniformly indent your name two spaces within the
    "Contributors" section as it is generally done in other parts of the
    source code?


    Could you change the parameter name of |CreateFontMetricsInstance|
    to something like |aResult| or |aFontMetricsResult| and also could
    you change the old-style

    +  return nsComponentManager::CreateInstance(kFontMetricsCID,
    +                                            nsnull,
    +                                            NS_GET_IID(nsIFontMetrics),
    +                                            (void **)fm);

    to the typesafe form:

       return CallCreateInstance(kFontMetricsCID, aResult);


    In nsFontCachePS.cpp, could you move the definition of
    |CreateFontMetricsInstance| out of line (and make the same changes
    to parameter names)?  There's no reason it will ever be inline since
    the whole reason it is to be called is through a pointer to base
    class.  Also, it might be cleaner to write the function as:

        NS_PRECONDITION(aResult, "null out param");
        nsIFontMetrics *fm = new nsFontMetricsPS();
        if (!fm)
          return NS_ERROR_OUT_OF_MEMORY;
        NS_ADDREF(*aResult = fm);
        return NS_OK;


    Where you're reindenting in nsDeviceContextPS.h, could you either
    untabify throughout and fix the indentation (and also the spacing
    around the ',' in the prototype for GetDeviceContextFor or not touch
    the whitespace at all?


    There shouldn't be any reason to override for nsFontCacheXlib.  The
    CreateInstance should work correctly when using the Xlib port.


General comments:

  Why do you have so many methods on nsIDeviceContextXPrint?  Should you
  need more than two or three?

Comment 22

[Roland Mainz]

> Comments on testing:
> 
>     How carefully have you tested postscript printing in addition to
>     XPrint?

s/XPrint/Xprint/

Well, it works perfectly for normal ISO-Latin-1 pages. 
But I did not test it with additional PostScript fonts (for example, to print
japanese chars). AFAIK there is some (undocumented) magic to do that - I do not
have any clue how to test that. But based on the issues fixed in Xprint
module... it can only be better for PostScript, not worse... :-)
Xprint's font support is far superiour compared to what the PostScript module
can do... it works for Xprint module - I assume it should work for PostScript
module, too...

>     Have you had someone test these changes on Windows?  (Linkage changes
>     should be tested on Windows.)

No... I do not have any Windows build box to compile the Zilla.

> Comments on code:
> 
>     Could you uniformly indent your name two spaces within the
>     "Contributors" section as it is generally done in other parts of the
>     source code?

OK, will be fixed.

>     Could you change the parameter name of |CreateFontMetricsInstance|
>     to something like |aResult| or |aFontMetricsResult| and also could
>     you change the old-style
> 
>     +  return nsComponentManager::CreateInstance(kFontMetricsCID,
>     +                                            nsnull,
>     +                                            NS_GET_IID(nsIFontMetrics),
>     +                                            (void **)fm);
> 
>     to the typesafe form:
> 
>        return CallCreateInstance(kFontMetricsCID, aResult);

I'll try that...

>     In nsFontCachePS.cpp, could you move the definition of
>     |CreateFontMetricsInstance| out of line (and make the same changes
>     to parameter names)?  There's no reason it will ever be inline since
>     the whole reason it is to be called is through a pointer to base
>     class.  Also, it might be cleaner to write the function as:
> 
>         NS_PRECONDITION(aResult, "null out param");
>         nsIFontMetrics *fm = new nsFontMetricsPS();
>         if (!fm)
>           return NS_ERROR_OUT_OF_MEMORY;
>         NS_ADDREF(*aResult = fm);
>         return NS_OK;

Ugh... assignment in NS_ADDREF() ? Ouch... ;-(

>     Where you're reindenting in nsDeviceContextPS.h, could you either
>     untabify throughout and fix the indentation (and also the spacing
>     around the ',' in the prototype for GetDeviceContextFor or not touch
>     the whitespace at all?

OK... I'll go and hunt down some tabs... will be fixed...

>     There shouldn't be any reason to override for nsFontCacheXlib.  The
>     CreateInstance should work correctly when using the Xlib port.

1. This bypasses the normal CreateInstance() overhead ==> speed .. :-)
2. I'd like to keep this at least for debugging purposes. Xprint module is just
a Xlib-module adopted to match the need of a printer, nothing more... :-)

> General comments:
> 
>   Why do you have so many methods on nsIDeviceContextXPrint?  Should you
>   need more than two or three?

Good question. No real idea - I may be worth to investigate if that stuff is
really needed anymore or not... I did not wrote that code... I am just hacking
it... :-)

Comment 23

[Roland Mainz]

Attachment: Better patch for 2001-07-20-08-trunk based on dbaron's comments

Comment 24

[Roland Mainz]

Filed new patch based on dbaron's comments.
Requesting sr= and a= ...

Comment 25

[Roland Mainz]

Tree will freeze for 0.9.3 in a few hours, requesting sr= ... pleassseeee !!

Comment 26

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

You missed:

> In nsFontCachePS.cpp, could you move the definition of
> |CreateFontMetricsInstance| out of line (and make the same changes
> to parameter names)?  There's no reason it will ever be inline since
> the whole reason it is to be called is through a pointer to base
> class.

Also, why did you make all the methods in nsFontCache virtual?  Isn't the only
one that needs to be virtual |CreateFontMetricsInstance|?

Comment 27

[Roland Mainz]

In theory you are right (that code is a relict of my debugging session where I
tested whether overriding works in general).
It may be usefull to keep it for further work or debugging... or not ? Are there
any problems with that ?

Comment 28

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

I just made two comments and you responded only to the second.  In response to
your response to the second:  I think you shouldn't make methods virtual that
you know don't need to be virtual.  It is a slight performance overhead and it
serves as documentation of how the class |nsFontCache| was intended to be used.

Comment 29

[Roland Mainz]

dbaron:
> I just made two comments and you responded only to the second.  In response to
> your response to the second:

SORRY!!... I thought you quoted some earlier text here... ;-(( 
I really should read things twice...

> In nsFontCachePS.cpp, could you move the definition of
> |CreateFontMetricsInstance| out of line (and make the same changes
> to parameter names)?  There's no reason it will ever be inline since
> the whole reason it is to be called is through a pointer to base
> class.

Ok. Agreed.

New patch in ~30-60mins...

Comment 30

[Roland Mainz]

Attachment: New patch for 2001-07-23-08-trunk per dbaron's comments

Comment 31

[Roland Mainz]

Filed new patch per dbaron's comment.

Requesting r=/sr=/moa=/a=

Comment 32

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

r=dbaron, if you've tested a number of different examples of PostScript printing
to make sure there aren't any problems.

Comment 33

[Roland Mainz]

No problems found.

Requesting sr= for this patch, please...

scc ?

Comment 34

[Roland Mainz]

Missed 0.9.3... bad... _still_ waiting for sr= ...

Comment 35

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

I've reviewed the patch in detail (I was just working in the font cache stuff in
bug 90545 and looking at the problems in bug 91956 (we destroy the fontcache
with every webshell)).

This patch has been r='d by dbaron (after much back-and-forth), and scc gave
provisional sr= early on.  For whatever little it's worth, I approve of the
patch, and if someone could give it a final sr= we should get it in.  It may
make solving bug 91956 easier to have some of this cleaned up.

Comment 36

[Christopher Blizzard (:blizzard)]

STOP TOUCHING WHITESPACE

sr=blizzard

Comment 37

[Roland Mainz]

blizzard wrote:
> STOP TOUCHING WHITESPACE

OK.
And... I filed bug 94074 as a request to make CVSblame for whitespace tolerant.

> sr=blizzard

Thanks ! 

----

Jesup:
Is there anything alse required (moa=/a= ?) before I can ping someone for
checkin ?

Comment 38

[timeless]

fix checked in

Comment 39

[Roland Mainz]

Win32 does not like that patch... ;-(

Reopening...

Comment 40

[Roland Mainz]

Fixed (by timeless:
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=997348500&maxdate=997348740&who=timeless%25mac.com
... thanks !!).

Marking bug as FIXED.

Comment 41

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

Why was the second comment in my 2001-07-21 review ignored?  I specifically told 
you to test this on Windows because of exactly the type of bustage that actually 
happened.

Comment 42

[Roland Mainz]

dbaron:
Again - I replied to your comment: I do not have a Windows box (nor a Mac box)
with build stuff on it. I can _only_ test on unix platforms...

Comment 43

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

That's why you get someone else (before tinderbox) to test it for you if your 
changes are likely to cause bustage.  I didn't ask if you'd tested them yourself 
-- I asked if you had someone else test them for you on Windows.

Comment 44

[John Morrison]

May God have mercy on us all. The 212 bug spam-o-rama is Now!