nsFontCache cannot be shared between multiple toolkits

RESOLVED FIXED in mozilla0.9.4

Status

()

RESOLVED FIXED
18 years ago
17 years ago

People

(Reporter: roland.mainz, Assigned: roland.mainz)

Tracking

Trunk
mozilla0.9.4
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Assignee)

Description

18 years ago
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= ?
(Assignee)

Comment 1

18 years ago
Created attachment 34849 [details] [diff] [review]
Patch for 2001-05-14-latest-trunk
(Assignee)

Comment 2

18 years ago
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 ?
Blocks: 80224
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2

Comment 3

18 years ago
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.  :)
(Assignee)

Comment 4

18 years ago
> 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 ?
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.
(Assignee)

Comment 6

18 years ago
Created attachment 34949 [details] [diff] [review]
(New) patch for 2001-05-16-08-trunk
(Assignee)

Comment 7

18 years ago
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 ?
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 kmcclusk@netscape.com ?
   who owns font code now that erik has left?)
(Assignee)

Comment 9

18 years ago
> check with the owner of nsDeviceContext.cpp (is that kmcclusk@netscape.com ?
> 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...
(Assignee)

Comment 10

17 years ago
Still no updates here... ;-(
Going to ping drivers...
(Assignee)

Comment 11

17 years ago
retargeting to 0.9.3 per asa's email to all bug owners...
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 12

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

Comment 13

17 years ago
Thanks !
I'll file a new patch...
(Assignee)

Comment 14

17 years ago
Created attachment 42842 [details] [diff] [review]
New patch for 2001-07-16-08-trunk
(Assignee)

Comment 15

17 years ago
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=
Keywords: review
Whiteboard: seeking r=, sr=

Comment 16

17 years ago
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 <Roland.Mainz@informatik.med.uni-giessen.de>
+ *
  * 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 ...)
Keywords: approval, patch
(Assignee)

Comment 17

17 years ago
Created attachment 43036 [details] [diff] [review]
Patch for 2001-07-16-08-trunk to match r=timeless ...
(Assignee)

Comment 18

17 years ago
Sorry... wrong patch... the current only only diffs orig/gfx/src to gfx/src ...
going to file the right patch...
(Assignee)

Comment 19

17 years ago
Created attachment 43038 [details] [diff] [review]
(full) patch for 2001-07-16-08-trunk to match r=timeless
(Assignee)

Comment 20

17 years ago
Filed new patch to match r=timeless, requesting sr= and a= ...
Whiteboard: seeking r=, sr= → seeking sr=, a=
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?
(Assignee)

Comment 22

17 years ago
> 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... :-)
(Assignee)

Comment 23

17 years ago
Created attachment 43131 [details] [diff] [review]
Better patch for 2001-07-20-08-trunk based on dbaron's comments
(Assignee)

Comment 24

17 years ago
Filed new patch based on dbaron's comments.
Requesting sr= and a= ...
(Assignee)

Updated

17 years ago
Blocks: 91831
(Assignee)

Comment 25

17 years ago
Tree will freeze for 0.9.3 in a few hours, requesting sr= ... pleassseeee !!
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|?
(Assignee)

Comment 27

17 years ago
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 ?
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.
(Assignee)

Comment 29

17 years ago
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...
(Assignee)

Comment 30

17 years ago
Created attachment 43469 [details] [diff] [review]
New patch for 2001-07-23-08-trunk per dbaron's comments
(Assignee)

Comment 31

17 years ago
Filed new patch per dbaron's comment.

Requesting r=/sr=/moa=/a=
Whiteboard: seeking sr=, a= → seeking r=, sr=, moa=, a=
r=dbaron, if you've tested a number of different examples of PostScript printing
to make sure there aren't any problems.

Updated

17 years ago
Blocks: 79119
(Assignee)

Comment 33

17 years ago
No problems found.

Requesting sr= for this patch, please...

scc ?
(Assignee)

Updated

17 years ago
Whiteboard: seeking r=, sr=, moa=, a= → seeking sr=, moa=, a=
(Assignee)

Comment 34

17 years ago
Missed 0.9.3... bad... _still_ waiting for sr= ...
Target Milestone: mozilla0.9.3 → mozilla0.9.4
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.
STOP TOUCHING WHITESPACE

sr=blizzard
(Assignee)

Comment 37

17 years ago
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 ?
Keywords: review
Whiteboard: seeking sr=, moa=, a= → seeking moa=, a=

Comment 38

17 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Keywords: approval
Resolution: --- → FIXED
Whiteboard: seeking moa=, a=
(Assignee)

Comment 39

17 years ago
Win32 does not like that patch... ;-(

Reopening...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 40

17 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 42

17 years ago
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...
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

17 years ago
May God have mercy on us all. The 212 bug spam-o-rama is Now!
QA Contact: aegis → jrgm
You need to log in before you can comment on or make changes to this bug.