Closed Bug 99010 Opened 23 years ago Closed 23 years ago

font changes

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.5

People

(Reporter: rbs, Assigned: rbs)

References

Details

(Keywords: fonts)

Attachments

(4 files)

total patch (diff -u)
427.42 KB, patch
Details | Diff | Splinter Review
total patch (diff -wu)
390.83 KB, patch
Details | Diff | Splinter Review
Implement new font routines for BeOS - based upon OS/2 changes.
6.99 KB, patch
Details | Diff | Splinter Review
Get text rendering on qt port again
6.26 KB, patch
Details | Diff | Splinter Review 
This is a continuation of the discussion in bug 74186 which is now too long.
I am re-summarizing the key elements here for ease of reference.

Here is a brief summary of what I did:

1. [Feature] Added support for multiple fallback fonts with factory 
   pre-built default fonts as discussed in bug 61883
   -> non-GfxWin status: pending (the layout part is XP).
      However, the new font prefs keys constitute a superset of the
      current ones so that other platforms can continue without this
      and support it later.

2. [I18N Consolidation] Brought the support of the 'A' functions
   used in Win95-J in parity with the other functionalities provided
   in the font subsystem (notably, the support of the substitute fallback
   font for missing glyphs and the handling of buffer overrun when
   converting a long Unicode string to the ANSI code page of each font
   subset). Also eliminated nsRenderingContextWinA with a re-work. 
   -> Specific to GfxWin. No action needed on other platforms.

3. [Code optimization] Switched to ResolveForwards() and ResolveBackwards()
   -> non-GfxWin status: pending, but only ResolveForwards() is needed --
      since ResolveBackwards() is for BiDi.
      This is not essential for a start. Other platforms can
      operate without this and support it later.

4. [Serious bug / Feature] Added support for GetDimensions()
   which is needed to allow layout to support dynamic font heights - the
   long standing bug 20304 - the layout part is XP.
   -> non-GfxWin: initiated only in GTK - bug 95721. This is _the_
      BLOCKER because all zillions versions of GFX are required to
      support it. Now that the dreaded layout part is complete, I am
      hoping that platform gurus can chime in... see bug 96609. Your help
      will be much appreciated. See the template in bug 95721 for an
      example of how it is being hooked in GfxGTK.

5. [Accessibility / Feature] Added support of a font's minimum-size
   -> Entirely handled in the Style System while leaving the chrome alone
      and honoring the CSS notion of "actual value" vs. "computed value"

6. [CSS2 / Feature] Added support for |font-size-adjust|
   http://www.w3.org/TR/REC-CSS2/fonts.html#propdef-font-size-adjust
   -> non-GfxWin status: pending. This is not essential for a start, but
      will be desirable for platform parity. Other platforms can
      operate without this and support it later.

   Of note: this was the intended change for bug 74186, but it proved to be the 
   major catalyst whose fix required & motivated the other changes. Indeed, if
   you have:

   <span style="font-family:Verdana; font-size:16px; font-size-adjust:0.55">
      a Verdana text... a Hebrew text....
   </span>

   then, sizeAdjust has to infiltrate everything during font-switching, and
   because each of the inner fonts need its own metrics, you basically need
   GetDimensions() to align the pieces, and you need to avoid to jam the text
   by letting layout know the overall extent of the text. Furthermore, the
   infiltration of sizeAdjust involved touching many functions at once, and
   that was a suitable opportunity than any to fix other long-standing bugs
   like #2 and #7 while things are fresh in the memory. 

7. [MLK/Cleanup] systematic fix of memory leaks in nsFontMetricsWin, by greping
   all places with 'new|calloc|malloc', and finding their corresponding
   delete/free. By adding comments on all the allocation lines that have their
   matching de-allocation lines, it became possible to pinpoint the culprits
   (i.e., the allocation lines which didn't get comments) and these could be
   fixed after a few iterations of this systematic process. It was like finding
   closing tags in an XML document. Fixing was possible because the font
   sub-system is pretty much self-contained, and the memory allocated there is
   also deleted there. Also did some cleanup while there, e.g., by using
   nsVoidArray() instead of hand-rolled growing arrays. There were also some
   changes of a typographical nature, like renaming 'CharSet' to 'Charset'
   because the latter is easier to read, or like prefixing globals variables
   with 'g'. These are changes that are easy to forget about.

8. [Outcome] Fix for a number of bugs, as summarized here:
http://groups.google.com/groups?selm=3B879CEF.E36F5A3%40maths.uq.edu.au&output=g
plain

For background on the whole story, please read the comments in bug 74186
bug (and the other bugs too).

Reminder: the minimalist thing needed to hook my patch on other platforms 
is to hook GetDimensions(). On these platforms, other extra things (like
|font-size-adjust|) would result in a no-op for the moment, although
XP parts (like the font's min-size) already benefit all platforms.

Testings
========

Since I have been carrying the bug for quite some time now, I did run the 
block and table regression tests several times, and have recently stepped 
through my changes in the debugger and refine them (as well as fixing other 
subtle preceding bugs that I discovered in the area). 

Also, QA (hixie) has been experimenting and providing feedback on the binaries
that I have been making available. Others are welcome to try it too. The last
drop is available here:

ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-win32-font-20010904.exe

Diffs
=====

Since the tree is ever changing and the number of patches attached to the bug
grew large, I am now keeping the whole set of changes locally. The diffs against
a tree of 2001-09-07 are at:

ftp://ftp.maths.uq.edu.au/pub/rbs/bug74186-diff-review.txt
-> For review (the path was obtained with diff -wu from mozilla/)

ftp://ftp.maths.uq.edu.au/pub/rbs/bug74186-diff-apply.txt
-> For application from mozilla/ by those interested.
   Simply run "patch -p0 < bug74186-diff-apply.txt" from mozilla/ to apply
   all of the changes on a tip of a tree as of today, and with a depend build,
   you are all set.

Requested r/sr
==============

* changes in content/ 
   attinasi, dbaron, hyatt, pierre.

* changes in layout/nsTextFrame and gfxwin/nsRenderingContext::GetDimensions()  
    attinasi, dbaron, waterson.
    [This is the crux of the measuring code. Sorry guys, the measuring code with 
    break indices is inherently cryptic :-) I have added helpful comments to    
    detail what is happening. I can provide more details if needed.]

* changes in gfxwin/nsFontMetrics
    ftang, brendan, kevin (kmcclusk).

* changes in gfxmac
    ftang, pierre.
    [Testing is still pending here, but note that only GetDimensions() is being
    hooked up as per bug 96609. Other changes are entirely XP and covered   
    elsewhere.]

* changes in gfxgtk (and other ports)
    blizzard, bstell, ftang
    [Testing is still pending here, but note that only GetDimensions() is being
    hooked up as per bug 96609. Other changes are entirely XP and covered
    elsewhere.]

Of course, this is just a subdivision to balance the workload. Feel free to r/sr 
any area you read and deem OK on the way. I am collecting the first r/sr that 
come along, thanks!

Status of reviews so far:
=========================

------- Additional Comments From Brian Stell 2001-09-07 20:08 -------


re: attachment 48545 [details] [diff] [review]

I r=bstell@netscape.com (approve) the code in

  nsFontMetricsGTK.cpp
  nsFontMetricsGTK.h
  nsRenderingContextGTK.cpp
  nsRenderingContextGTK.h

I do ask that we open a bug at this time as a marker to clean up 
the extra DrawString call.


Although my experience is more limited in the gfx/src/ps and
gfs/src/xlib areas, it seems okay. If you have trouble finding
a reviewer for these file let me know.
At the present I make no statement about these files: ()

  nsRenderingContextPS.cpp (katakai could you check this?)
  nsRenderingContextPS.h   (katakai could you check this?)

  nsFontMetricsXlib.cpp
  nsFontMetricsXlib.h
  nsRenderingContextXlib.cpp
  nsRenderingContextXlib.h


Although I see no immediate problems in the following, I have no 
expertise in these areas so I cannot express an opinion either
positive or negative of the changes to:

  nsFont.h
  nsFont.cpp
  nsIRenderingContext.h
  nsRenderingContextMac.cpp
  nsRenderingContextMac.h
  nsUnicodeRenderingToolkit.cpp
*** Bug 74186 has been marked as a duplicate of this bug. ***
re: attachment 48545 [details] [diff] [review] 
Patch for gfx/src/xlib looks fine, but I have to wait for bug 90380 until I can
test it in detail ... AFAIK it will take another two or three days until it gets
it's sr= ... is that acceptable ?
I guess it is OK since there are other parts as well. But of course, the sooner 
the better.
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.5
Keywords: fonts
add mkaply@us.ibm.com to cc: since this affects layout
Latest binary drop...
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-win32-font-20010911.exe

And following a request, I am providing the full source for less hassle:
ftp://ftp.maths.uq.edu.au/pub/rbs/bug99010-content.zip
ftp://ftp.maths.uq.edu.au/pub/rbs/bug99010-layout.zip
ftp://ftp.maths.uq.edu.au/pub/rbs/bug99010-gfx.zip

For each zip, cd to the corresponding directory and unzip therein (there are no
top-level wrapping paths). And with a depend build from mozilla/, all is set.
rbs/bstell:
Idea/suggestion:
What about wrapping your code with a |#ifdef FONT_DRAWSTRING2|+"configure option
--use-drawstring2" and check it in _now_ instead of trying to wait for
each&everyone ?

For myself I find it so D@MN hard to get all the reviews/super-reviews done
that I break up big tasks so I only need to corral a few people at a time
to get ANYTHING past the checkin point (and STILL it takes for ever).

At present I'm a bit frustrated that the tree was closed ~40% of this
current 5 week period.

Rbs has been busting his butt to try and get this in. (RBS,  by-the-way: It 
really helped that you separated out the part for me to review.)

Asa/Brendan: any suggestions on how we can help rbs get this checked in?

I agree smaller patches are far easier to get reviewed, but if you have a big
one, we can find the reviewers.  If you are getting no response from a
particular r= or sr= giver, feel free to mail staff@mozilla.org and we'll find
out what's causing the non-response, and fix that problem, or find an alternate
reviewer.

rbs: you need to mail individuals with just the diffs you want reviewed, for
best accountability and likely responsiveness.

Is there a branch?  Should there be?  Then others could pull, test, and help
hack and debug more readily.

/be
>rbs: you need to mail individuals with just the diffs you want reviewed, for
>best accountability and likely responsiveness.

OK, will have a go at this.

>Is there a branch?  Should there be?  Then others could pull, test, and help
>hack and debug more readily.

There is a branch but I have discontinued landing changes there. It is too much 
hassle on me to maintain a branch in sync for that long giving all the massive 
checkins that are happening on the trunk.

The easiest way for people to try things now is as per my comments of 2001-09-10 
23:23 (I really mean 'now' because another checkin on thise fast changing trunk 
can render these obsolote...). To try, just unzip the zip files as indicated.
You can first duplicate these directories and restore your originals when you 
are done, or you can duplicate your entire tree if you prefer. The first 
approach is probably the easiest but requires two depend builds, whilst the 
second means you can rm -r the other tree and won't have to depend build twice.

I have split the changes into pieces intended for individual reviews. Since the 
understanding of one piece may involve looking at other pieces, I am listing
everything here so that all reviewers can quickly jump back and forth between
the pieces if need arises. I have everything ready and can mass-mail easily but
I will wait a little bit to see how things go now. I have already explained on 
several occasions what is happening, so...

CONTENT
=======

->awaiting r/sr: attinasi, dbaron, hyatt, pierre
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-content.txt
M content/base/src/nsStyleContext.cpp
M content/html/content/src/nsHTMLFontElement.cpp
M content/html/style/public/nsIRuleNode.h
M content/html/style/src/nsCSSStyleRule.cpp
M content/html/style/src/nsComputedDOMStyle.cpp
M content/html/style/src/nsRuleNode.cpp
M content/html/style/src/nsRuleNode.h
M content/shared/public/nsStyleStruct.h
M content/shared/src/nsStyleStruct.cpp

LAYOUT
======

->awaiting r/sr: attinasi, dbaron, waterson
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-layout.txt
M layout/base/public/nsIPresContext.h
M layout/base/public/nsStyleConsts.h
M layout/base/src/nsPresContext.cpp
M layout/base/src/nsPresContext.h
M layout/html/base/src/nsLineLayout.cpp
M layout/html/base/src/nsTextFrame.cpp

GFX
===

->awaiting r/sr: ftang, brendan, kevin (kmcclusk)
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-pub.txt
M gfx/public/nsIRenderingContext.h
M gfx/public/nsFont.h
M gfx/src/nsFont.cpp

->have r=bstell
->awaiting sr: blizzard
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-gtk.txt
M gfx/src/gtk/nsFontMetricsGTK.cpp
M gfx/src/gtk/nsFontMetricsGTK.h
M gfx/src/gtk/nsRenderingContextGTK.cpp
M gfx/src/gtk/nsRenderingContextGTK.h

->awaiting r/sr: blizzard, bstell, ftang
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-ps.txt
M gfx/src/ps/nsRenderingContextPS.cpp
M gfx/src/ps/nsRenderingContextPS.h

->have r=gisburn (alright with you?)
->awaiting r/sr: blizzard
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-xlib.txt
M gfx/src/xlib/nsFontMetricsXlib.cpp
M gfx/src/xlib/nsFontMetricsXlib.h
M gfx/src/xlib/nsRenderingContextXlib.cpp
M gfx/src/xlib/nsRenderingContextXlib.h

->awaiting r/sr: ftang, pierre
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-mac.txt
M gfx/src/mac/nsRenderingContextMac.cpp
M gfx/src/mac/nsRenderingContextMac.h
M gfx/src/mac/nsUnicodeRenderingToolkit.cpp

->awaiting r/sr: ftang, brendan, kevin (kmcclusk)
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-win-metrics.txt
M gfx/src/windows/nsFontMetricsWin.cpp
M gfx/src/windows/nsFontMetricsWin.h

->awaiting r/sr: attinasi, dbaron, waterson
(note: these changes are algorithmic. They hook ResolveForwards() and 
ResolveBackwards(). The Windows GDI isn't the primary issue here)
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-win-rendering.txt
M gfx/src/windows/nsGfxFactoryWin.cpp
M gfx/src/windows/nsRenderingContextWin.cpp
M gfx/src/windows/nsRenderingContextWin.h
> ->awaiting r/sr: blizzard, bstell, ftang
> ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-ps.txt
> M gfx/src/ps/nsRenderingContextPS.cpp
> M gfx/src/ps/nsRenderingContextPS.h

I'm going to look into PS codes by Brian's request.
But should we ask dcone and syd for review also?
It is alright by me to get other people's screening if that helps to speed things
up a little bit -- changes like these are better early in a milestone.
> ->awaiting r/sr: blizzard, bstell, ftang
> ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-ps.txt
> M gfx/src/ps/nsRenderingContextPS.cpp
> M gfx/src/ps/nsRenderingContextPS.h

I have reviewed the patch around gfx/src/ps and it looks OK.
I asked module owners (dcone and syd) for review also.

I have tested Mozilla itself (with whole changes) and printing
in *Japanese* locale and I could not find any regression.
Sorry, I didn't mention about platform. I tested on Redhat 6.2 Japanese.

r=attinasi for the content patch:
  ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-content.txt

[s]r=attinasi for the layout patch:
  ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-layout.txt
excepth that I am a little weak on the details of the TextFrame and the changes
theirin. I would especially like to see aonther of set of validating eyes on
that of possible, although I do believe that the testing on these changes has
been quite excellent.



I'm reviewing the text measurment code in patch
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-win-rendering.txt

I apologize, but I am slow with this kind of code. I have tried to understand
the code that lives in there before and found it too to be quite difficult.

The 'code' looks fine, my concerns are with the performance of the code after
the change, and with the elimination of the 'A' variant. Please set my mind at
ease by telling me (or pointing me to previous posted data on) how you tested
the performance of the changes, and how you were able to eliminate the 'A'
variants. Thanks. Tantative [s]r=attinasi pending the performance analysis info. 
Thanks for the reviews. I got the news that ftang and the intl guys are away 
in holidays and the Unicode conference, so kevin, please r= the changes in 
gfxwin-metrics -- that will be much appreciated; pierre, please r= the gfx-mac 
changes (the mac changes are just to hook GetDimensions() so as not to break the 
mac build).

---
To answer Marc questions:

I havn't done any focussed performance measurements, except complexity analysis
in a theoretical sense. What I am practically doing is to keep two trees, one 
called "trunk" which matches the trunk, and another called "trunk-font" which is 
the trunk with all my font changes. I am not able to perceive a difference when 
I randomly pick either of the launching shortcuts.

A criteria that buster gave in bug 20394 for this functionality was that:

  "One goal should be that "normal" pages that don't include any interesting
   glyph anomalies should display just as fast with the new code as with the
   old, and that the performance price we pay should at worst be proportional to 
   the number of additional fonts that are loaded."

This criteria is met because the ASCII path doesn't involve font-switching,
there is only one font in this case, leading to straight calls to the
GetDimensions(const char*, ...) functions (there is even an added bonus because 
I didn't involve additional style context data structures as erik alluded to in 
that bug). 

When there are several fonts, the extra cost is in the code-section that I 
commented as "post-processing for the ascent and descent". It can be seen that 
this is a negligible linear loop in the number of fonts (how many fonts will 
arise in practice, 2 or perhaps 3, not that much really). What troubled me the 
most on the whole patch is the addition of |mAscent| to nsTextFrame, but 
avoiding this requires fixing that other bug 64763 as I have already noted.

For the elimination of nsRenderingContextWinA: the problem is that a Unicode 
font file can embed many "subfonts" (or "subsets"). For example, a font like 
Times New Roman is really a package of several subfonts: one for the "Western" 
charset, another for the "Arabic" charset, another for the "Greek" charset, etc.
This packaging has the advantage of offering a collection of homogeneously 
designed glyphs. A subset is a selection of indices for the glyphs relevant to 
the charset (some glyphs may physically be the same and be re-used between 
charsets). Depending on the currently active language group, the Windows GDI 
auto-select the relevant subset so that applications don't have to worry about 
the inner workings. Apparently, this mechanism has a bug in the Win95-J version 
and so erik had to manually handle the individual subsets. To implement the 
needed functions, GetWidth() and DrawString(), the loop for resolving text
fragments that are representable in the same font had to dig further down the 
subsets. With my addition of ResolvedForwards(), I pushed this logic down to the 
fonts themselves: it is now nsFontMetricsA::ResolvedForwards() that digs into 
the subsets, and so the rendering context could be freed from this knowledge.
These changes are extensive enough to warrant a spot on chofmann's branch
landing plan, IMO. This will require creation of a branch (if only for the
purpose of easily spinning builds), but will also dramatically increase testing
coverage (for correctness, performance, etc.)
re: spot on chofmann's branch landing plan
seems okay

re: branch
as noted earlier, I had/have a branch but it achieved little as far as I am 
concerned.

re: perf
I have been providing a number of binary drops on the way. The latest drop is:
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-win32-font-20010911.exe

If people can tell/measure any difference they see... I cannot tell a difference 
myself. Although it is only the win32 binary, it is largely indicative of the 
overall performance impact because there are only minimalist changes on other 
flavors of GFX.

re: correctness/testing
As far as hixie's torture testing and my own testing go, there are no _known_ 
regressions at the moment (there is no denying an element of risk in the checkin 
-- all I am saying is that there are no _know_ regressions). I should perhaps 
jot down again that I intensively tested the 'A' changes by forcibly setting the 
'useAfunctions' flag in nsGfxFactoryWin. With this flag on, the 'A' code-path 
gets activated and emulates the Win95-J situation -- except that the low system 
calls are different. If people on Win95-J can download the drop... (what else 
can I say...).

It may sound like I am trying to avoid doing any further work. But in reality, I 
see little room for further optimizing iterations in my changes -- apart from 
some obvious cleanup in nsTextFrame to remove some variables called smallY, 
lastY, ..., that have been obsoleted by the introduction of |mAscent|. These can 
be removed at the forthcoming cleanup phase. And with all the testing, I may 
break something now by trying to optimize things that gain nothing. And yes, it 
is true that after carrying this bug for so long, I am instead looking forward 
to be relieved and to get ready for the second phase -- the cleanup of the 
transient APIs which I had hoped to also fit in the timeframe of this milestone.
[s]r=attinasi for the changes in
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-win-rendering.txt

I recommend getting a carpool, landing early, and having a good set of testing
criterion during a verification phase after the carpool and before accepting
then general landings. This will make regression-managment more, well, manageable.

Added an entry on chofmann's branch landing plan:
http://komodo.mozilla.org/planning/branches.cgi
Blocks: 96928
> ->awaiting r/sr: ftang, brendan, kevin (kmcclusk)
> ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-win-metrics.txt
> M gfx/src/windows/nsFontMetricsWin.cpp
> M gfx/src/windows/nsFontMetricsWin.h
I finally finished reviewing this part, and have some questions to rbs:
1, @@ 3957, 25 +4075,11
  pstr is not freed in all cased. See following lines:
     if (GDI_ERROR == len) {
       return NS_ERROR_UNEXPECTED;
  When we hit GDI_ERROR, there is possible memory leak.
  Another question here, why SelectObject/DeleteObject is removed here?
2,@@ -1822,13 +1822,95
  For function InitMetricsFor, do you need to select font into hDC before 
retrieving metrics? This function does not seem to be called by anybody (yet?). 

> When we hit GDI_ERROR, there is possible memory leak.
Good catch. Will cleanup properly...

> Another question here, why SelectObject/DeleteObject is removed here?
These calls were redundant because when font-switching arises the rendering 
context, higher up, selects the font in the DC and restores its own current font 
at the end. Moreover, instead of pairs of select-delete select-delete, one gets: 
select(font-switching) - select(font-switching) - ... - select(currentFont).
(BTW see bug 27056, Troy did a general cleanup about these and observed a 
speedup in doing so. That bit was a left-over)

> For function InitMetricsFor, do you need to select font into hDC before 
>retrieving metrics? This function does not seem to be called by anybody (yet?). 

Indeed, but I am letting select be done by the caller (and the function is 
indeed called -- that's where the metrics for sizeAjust come from):
+    if (font) {
+      HFONT oldFont = (HFONT)::SelectObject(aDC, (HGDIOBJ)hfont);
+      InitMetricsFor(aDC, font);
+      mLoadedFonts.AppendElement(font);
+      ::SelectObject(aDC, (HGDIOBJ)oldFont);
     return font;
   }
Updated diff as per shanjian's comments (may need a reload):
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-win-metrics.txt

shanjian, I assumed you are also okay with the contingent patch:
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-pub.txt

I haven't heard from pierre re: the last piece
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-mac.txt

which means it is ok? I think there have been lots of eyeballs so far on all the 
diffs and it is ok to kick off the next round, pending any other comments.

All is waterson's for a global sr= for the next round, thanks.
> ->pierre
> ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-mac.txt
> M gfx/src/mac/nsRenderingContextMac.cpp
> M gfx/src/mac/nsRenderingContextMac.h
> M gfx/src/mac/nsUnicodeRenderingToolkit.cpp

I can't really review without this code without having the declaration of
nsDimensions and an explanation on what GetDimensions() is for but...

- In nsUnicodeRenderingToolkit.cpp, you changed DrawString and made it behave
like a DrawString2.  Maybe the name should be changed too. 

- I don't like the name DrawString2.  Can you rename DrawString into
DrawStringAtBaseline and DrawString2 into DrawString, or something similar?

- I'm wary about the comments in GetDimensions(const PRUnichar* aString,...).
 I'm not sure we can checkin the code as-is and implement
nsUnicodeRenderingToolkit::GetDimensions() later.  Franck Tang will have to
sign on that one.

> I can't really review without this code without having the declaration of
> nsDimensions and an explanation on what GetDimensions() is for but...

I have been explaining what it does, it is declared in the public diff:
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-pub.txt

Maybe you have a filter for your bugzilla mails and read them in a 
delay mode? I have been giving lot of details ever since bug 74186...

> - In nsUnicodeRenderingToolkit.cpp, you changed DrawString and made it behave
> like a DrawString2.  Maybe the name should be changed too.
>
> - I don't like the name DrawString2.  Can you rename DrawString into
> DrawStringAtBaseline and DrawString2 into DrawString, or something similar?
> 
> - I'm wary about the comments in GetDimensions(const PRUnichar* aString,...).
>  I'm not sure we can checkin the code as-is and implement
> nsUnicodeRenderingToolkit::GetDimensions() later.  Franck Tang will have to
> sign on that one.

Note that the name DrawString2() is just a transitory name -- as I already
noted in the bug, it will be renamed to DrawString() and the old DrawString()
will go away. The current patch is already a large patch and the review process
became significantly complicated. It wouldn't be rocket science for a Mac guru
to hook nsUnicodeRenderingToolkit::GetDimensions() as I explained in bug 96609
http://bugzilla.mozilla.org/show_bug.cgi?id=96609
But I am getting little backup (not only on the Mac, I might hasten to add). 
I concede that Mac people are few and the same people are called to rescue
all the time. That's why I have that fix-up patch whose sole aim is not to
break the Mac build, knowing that parity doesn't need to happen synchronously.
> I have been explaining what it does, it is declared in the public diff:
> ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-pub.txt

Correct, fine with me.

> Maybe you have a filter for your bugzilla mails and read them in a
> delay mode? I have been give lot of details ever since bug 74186...

It was a little bit difficult to follow your progress on a daily basis.  I kept 
an eye on it and commented once or twice when it seemed really important.  I also 
played with the first binaries you posted a while ago and agreed with Ian that it 
wasn't ready for showtime yet.  Recently I think I saw other people recommending 
you to break up the code into smaller pieces and have them reviewed separately.  
That's what you did, I was just missing some details to review the part you sent 
me.  Otherwise and overall, I regret not having given to your work all the 
attention and the support it deserves.  I was waiting for the changes to 
stabilize a little bit and help you out on the Mac, and here we are.

> It wouldn't be rocket science for a Mac guru
> to hook nsUnicodeRenderingToolkit::GetDimensions() as I explained in bug 96609
> http://bugzilla.mozilla.org/show_bug.cgi?id=96609
> But I am getting little backup (not only on the Mac, I might hasten to add).

I don't know whether it will be of any comfort but you are not the only one.  
It's becoming increasingly difficult for everybody here to get anything reviewed 
seriously and checked in.

> I concede that Mac people are few and the same people are called to rescue
> all the time. That's why I have that fix-up patch whose sole aim is not to
> break the Mac build, knowing that parity doesn't need to happen synchronously.

I don't mind a temporary lack of parity as long as it does not introduce any 
regression.  

We also need to an estimate regarding the feasibility on the Mac - just in case. 
That's why I copied Franck to have his opinion on the matter.

The fix-up patch on the Mac is aimed at retaining the current trunk behavior (no 
regressions). Therefore, the Mac will be missing the newly added dynamic font 
heights -- until nsUnicodeRenderingToolkit::GetDimensions() is implemented. 
That's the piece that is lacking. As you know, Mac isn't my developmental 
platform and its GFX code. I can be of assistance in providing more details as 
to what is needed.

LXR gives the blame to ftang for nsUnicodeRenderingToolkit::GetWidth(). I think
he will not have that much trouble hooking GetDimensions() once/if he sets out
to do it. Some of the i18n people have become quite familiar with what is going 
on too.

The code for nsUnicodeRenderingToolkit::GetDimensions() will be a slight 
extension to nsUnicodeRenderingToolkit::GetWidth(). At the place where 
GetTextSegmentWidth() is called, the maxAscent/maxDescent will also needto be 
sync:ed. This means that the ascent/descent of the "inner" fonts used in
GetTextSegmentWidth() will have to be cached when these fonts are created. This 
is basically the scenario that is happening on other platforms.
>The code for [nsUnicodeRenderingToolkit::]GetDimensions() will be a slight 
>extension to [nsUnicodeRenderingToolkit::]GetWidth(). At the place where 
>GetTextSegmentWidth() is called, the maxAscent/maxDescent will also needto be 
>sync:ed.

BTW, this is also why I said that there is little perf impact. For ASCII text, 
there is 1 font -- meaning no font-switching, and for Unicode text, mostly 2
or 3 font-switching operations, and the extra overhead in the sync:ing of
the maxAscent/maxDescent is:
  maxAscent = PR_MAX{maxAscent, current' font ascent}
  maxDescent = PR_MAX{maxDescent, current' font descent}
which is just noise amongst the surrounding heavy-weight operations that are
going on.
Sorry, still haven't found three our four contiguous hours to spend catching up
on the bug and cozying up with the patch for a proper super-review.
Thanks for the explanations.  r=pierre for 'diff-gfx-mac.txt'.
I or ftang will take care of the unicode function later. 

I'd still like to spend some time looking over your changes to
nsRenderingContextWin and nsTextFrame. I thought I'd post the
following comments so that you could respond to them sooner rather
than later. :-)

. Get hyatt to super-review your changes to the style system,
  especially to nsRuleNode. hyatt: any issues with exposing the rule
  node bits?

. nsDimensions is a pretty vague name for a structure. I'd prefer
  something more apropriate given its purpose. nsTextExtent, perhaps?
  (Or at least nsTextDimensions?) Ibid for methods and arguments that
  use the structure; e.g., nsIRenderingContext::GetDimensions() looks
  at first blush as if it ought return the dimensions of the rendering
  context object.

.  Furthermore, don't use operator+= on this structure. I'd prefer you
  give this method a meaningful name, especially since it's not
  immediately obvious by what it means to ``add'' two of these
  together. It would also be surprising for somebody to discover that
  they could ``add'' two of these using operator+=, but not
  operator+. The point being, if you're going to do operator
  overloading, do it all the way or not at all. (BTW, operator+=(T&)
  ought to return a T& so its result can be used as an lvalue).

. Get separate bugs filed on appropriate people to implement
  nsUnicodeRenderingTookit::GetDimensions() for all platforms: Mac, PS, 

. Verify (or find someone to verify) that there is no performance
  degradation on Win32 according to both iBench and jrgm's test
  harness.

. Have you verified that this works properly with bidirectional text?

. Have you verified that this works properly on all the flavors of
  Windows? (Note to self: need to apply changes in
  nsRenderingContextWin.[h|cpp])

. The nomenclature for ``generic font ID'' conveys remarkably little
  information. Don't these ``generic IDs'' really correspond to the
  basic CSS font families? Could we use the nomenclature of ``generic
  font family'' here; e.g., nsFont::GetGenericFamily()? Using
  |aFontFamily| instead of |aFontID| as arguments?

. 32 bit quantities are passed by value, so I'm not sure what we gain
  here by requiring that |aPrefType| be |const| in nsIPresContext.h:

+  NS_IMETHOD GetCachedBoolPref(const PRUint32 aPrefType, PRBool& aValue) = 0;

. Use NS_ConvertUCS2toUTF8() as opposed to *WithConversion() in
  nsPresContext.cpp:

+  pref.Assign("font.minimum-size."); pref.AppendWithConversion(langGroup);

. In nsPresContext::GetFontPreference(), font size won't get assigned
  properly if somebody puts something other than `px' or `pt' into the
  `font.size.unit' pref (e.g., `cm'). I suspect things will rapidly go
  south from here? Perhaps you ought to ensure that the unit is
  something reasonable up front.

. Why not keep the default fonts in an array and use the font family
  as an index into that array? (You'd avoid several switch statements,
  I think.)

. Does your change to line layout have any impact on the recent work
  attinasi has been doing to deal with whitespace contained in table
  cells?

Finally, although I honestly appreciate the effort that's gone into
all this work, I'll ask that you break massive changes like this up
into bite-sized portions in the future. You ought to submit separate
patches, each fixing a specific problem (I plan to insist on it, in
fact -- I'll sr no more 11K line patches). I'll also insist on
separating non-trivial code-cleanup from ``real'' bug fixes. The issue
is that I simply cannot do a quality code review on something this
large.

I realize the down-side is that you end up with multiple trees,
patches, etc., or that you may need to spend extra effort staging the
work, but I think that the benefit (to you) will be quicker turnaround
on individual pieces, as well as a review of higher quality.
Why are there no patches attached to the bug?  Which patch am I supposed to
review exactly?

Hyatt: they are available via FTP from the links in the header. I assume this is
because doing multiple revisions using Bugzilla-attachments would quickly get
unwieldy. I'm sure rbs will attach the final versions after super-review.

Gerv
I personally prefer attachments but this is a very large set of patches and
bitrot is a serious problem.
>. Get hyatt to super-review your changes to the style system,
>  especially to nsRuleNode. hyatt: any issues with exposing the rule
>  node bits?

As per my comments of 2001-09-11 17:07, the changes are:
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-content.txt
M content/base/src/nsStyleContext.cpp
M content/html/content/src/nsHTMLFontElement.cpp
M content/html/style/public/nsIRuleNode.h
M content/html/style/src/nsCSSStyleRule.cpp
M content/html/style/src/nsComputedDOMStyle.cpp
M content/html/style/src/nsRuleNode.cpp
M content/html/style/src/nsRuleNode.h
M content/shared/public/nsStyleStruct.h
M content/shared/src/nsStyleStruct.cpp

>. nsDimensions is a pretty vague name for a structure. I'd prefer
>  something more apropriate given its purpose. nsTextExtent, perhaps?
>  (Or at least nsTextDimensions?) Ibid for methods and arguments that
>  use the structure; e.g., nsIRenderingContext::GetDimensions() looks
>  at first blush as if it ought return the dimensions of the rendering
>  context object.

If that makes you happier, I will s/Dimensions/StringDimensions/, and 
later see (in the cleanup patch) if bstell's suggestion can be 
taken -- he suggested to eliminate GetWidth() altogther now.
[Your comments could be said about nsIRenderingContext::GetWidth()...]

>. Furthermore, don't use operator+= on this structure. I'd prefer you
>  give this method a meaningful name, especially since it's not
>  immediately obvious by what it means to ``add'' two of these
>  together. It would also be surprising for somebody to discover that
>  they could ``add'' two of these using operator+=, but not
>  operator+. The point being, if you're going to do operator
>  overloading, do it all the way or not at all. (BTW, operator+=(T&)
>  ought to return a T& so its result can be used as an lvalue).

aStringDimensions.Combine(aOther) ?

>. Get separate bugs filed on appropriate people to implement
>  nsUnicodeRenderingTookit::GetDimensions() for all platforms: Mac, PS, 

Filed bug 100868 - Add GfxMac support for getting actual string 
                   height (GetStringDimensions) 

Filed bug 100871 - Add GfxPS support for getting actual string 
                   height (GetStringDimensions)

And as a reminder, there is a tracker bug - bug 96609. 

>. Verify (or find someone to verify) that there is no performance
>  degradation on Win32 according to both iBench and jrgm's test
>  harness.

My current build setup doesn't include all the nifty tools that you people have. 
So here is a latest binary drop for QA to test any perf impact (I waited for it 
to complete so that I can reply to the super-reviewer comments with a link :-)
ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-win32-font-20010921.exe

>. Have you verified that this works properly with bidirectional text?

My comparative testings with my "trunk-font" and "trunk" builds show similar 
renderings when visiting bidi pages. I privately notified mkaply early on when I 
started the changes. Bidi people, if I have missed something, please do well to 
tell.

>. Have you verified that this works properly on all the flavors of
>  Windows? (Note to self: need to apply changes in
>  nsRenderingContextWin.[h|cpp])

I personally tested on Win95 and Win2K. But perhaps, those who grabbed the 
binary drops may have used different Win32 flavors as well.

>. The nomenclature for ``generic font ID'' conveys remarkably little
>  information. Don't these ``generic IDs'' really correspond to the
>  basic CSS font families? Could we use the nomenclature of ``generic
>  font family'' here; e.g., nsFont::GetGenericFamily()? Using
>  |aFontFamily| instead of |aFontID| as arguments?

The intention here is to map CSS generic fonts ("serif", etc.) to numeric 
identifiers and then, I use the cheaper IDs throughout to avoid repeated string 
comparisons. GetGenericFamily() isn't exactly reflecting that fact. But I don't 
mind changing to something more intituive if a better name is suggested.

>. 32 bit quantities are passed by value, so I'm not sure what we gain
>  here by requiring that |aPrefType| be |const| in nsIPresContext.h:
>
>+  NS_IMETHOD GetCachedBoolPref(const PRUint32 aPrefType, PRBool& aValue) = 0;

My thinking at the time was to highlight the input/output dichotomy.
Since there is neither gain nor loss, maybe the 'const' can be retained.

>. Use NS_ConvertUCS2toUTF8() as opposed to *WithConversion() in
>  nsPresContext.cpp:
>
>+  pref.Assign("font.minimum-size."); pref.AppendWithConversion(langGroup);

Fixed.
(latest: ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-layout.txt)

>. In nsPresContext::GetFontPreference(), font size won't get assigned
>  properly if somebody puts something other than `px' or `pt' into the
>  `font.size.unit' pref (e.g., `cm'). I suspect things will rapidly go
>  south from here? Perhaps you ought to ensure that the unit is
>  something reasonable up front.

Fixed.

>. Why not keep the default fonts in an array and use the font family
>  as an index into that array? (You'd avoid several switch statements,
>  I think.)

I first tried using an array, but it creates a mess w.r.t. initialization in
the constructor of nsPresContext. A font may be initiliazed as "cursive", 
another as "serif", etc, and the various member fields of these fonts need to be 
initialized individually. On the other hand, GetFontPreference() -- with its
switch -- is cally only once or twice during the lifetime of a presentation 
context or when the user explicitly change a pref.

>. Does your change to line layout have any impact on the recent work
>  attinasi has been doing to deal with whitespace contained in table
>  cells?

My changes are involved with the height while attinasi's were related to the 
width -- the max-element width to be precise. (Part of having such changes, and 
for so long, means that I had to keep an eye on what is happening in the 
surrounding areas. So attinasi' changes were just part of the things that I had 
to pay attention to. I had to recover from several other changes.)

> I think that the benefit (to you) will be quicker turnaround
> on individual pieces, as well as a review of higher quality.

The reality on the terrain is not that rosy regarding this r/sr process in a 
global perspective. So as a thank you for taking the time to review _many_ of my 
stuff (not only this one), let's leave this can of worms alone for now, thanks.
. Don't want to nitpick too much over names, but StringDimensions rubs me wrong
  too (perhaps because of the baggage I associate with our multifarious string
  classes). Could I sell you on s/Dimensions/TextDimensions/? Or is that too
  close to nsTextMetrics? I take it you didn't like nsTextExtent?

  (By ``cleanup patch'', were you suggesting that you'd do this in another
  pass? That's fine with me, just want to be clear.)

. Combine seems like a fine name to replace operator+=.

. I'll volunteer to do the performance A-B testing if nobody beats me to it,
  but I won't be able to get at it for a few days (say, tomorrow earliest,
  Monday, latest).

. I'll back off on the generic font ID nomenclature, given that ``generic
  font'' is meaningful if you aren't a CSS tyro. (Which I am.)


I picked StringDimensions in analogy with DrawString() -- to get:
nsIRenderingContext::GetStringDimensions()
nsIRenderingContext::DrawString()

But I can buy nsTextDimensions() and do the substitution in this round.

Do you like the symmetry?
nsIRenderingContext::GetTextDimensions()
nsIRenderingContext::DrawText()

If you like the symmetry better I might delay the substitution and do both in 
the clean up patch.
In:

ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-bug99010/diff-gfx-gtk.txt

in nsRenderingContextGTK::GetDimensions() is aFontID ever supposed to be
assigned anything?

Also, can you add a comment describing the whole i/start/prevFont/currFont for
me?  It took a moment for me to see how they all interacted.
> is aFontID ever supposed to be assigned anything?

All these functions (GetWidth, DrawString, GetBoundingMetrics, and now 
GetDimensions) have this aFontID thing. Apparently, it was aimed to be used for 
performance reasons (tm). Say, if it is set, then the caller can send it back 
again later to indicate that the font was already found to represent the string, 
so that there is no need to repeat glyph lookup. Just go ahead and use that 
font. Sound nice in theory but it has never been implemented. So if people feel 
it can be cleaned, then I might sweep all of them in my forthcoming cleanup 
patch.

>Also, can you add a comment describing the whole i/start/prevFont/currFont for
>me?  It took a moment for me to see how they all interacted.

OK, no problem. I can add some comments.

BTW, this code is a "copy-paste" of GetWidth() in which I just added the 
sync:ing of the ascent/descent as it can be seen. If you replace the delta 
portion with DrawString(current substring) or GetBoundingMetrics(current 
substring), you have all the other functions... That's why in GfxWin, I went 
ahead and create ResolveForwards() to provide a backbone code, with a callback 
mechanism for people to hook what they want rather than "copy-pasting" the whole 
code, and adjusting the delta portion. Then, following this separation, 
ResolveForwards() could be fine-tuned and optimized separately. And as a bonus, 
it helped to remove RenderingContextWinA as I noted earlier.
It just occurred to me that you were referring to bugzilla comments -- I first
thought that you were referring to the addition of some accompanying code-level
comments.

Anyway, here they go. The crux of the matter is to find contiguous string
fragments that can be represented with the same font, while only loading the
fonts lazily, and at the same time honoring the ordering of fonts as specified
in the CSS font-family property.

Starting with <span style="font-family: font1, font2, font3"> a string... </span>
if at some point it is already determined that string[start..i] is representable
in font2, then the next character string[i + 1] will further be included only if
it can be represented in font2, yes, but with the caveat that it cannot be
represented in font1 -- otherwise font1 should be used for that character (as
per the CSS ordering). 

On the other hand, if all of the string is processed with font1 and font2, then
font3 is never loaded (lazy loading), otherwise it is loaded when none of the
already loaded fonts can represent the character at string[i + 1] and is then
searched too (if the CSS list is exhausted, the font subsystem will add some
fallbacks fonts). The rest (prevfont vs currfont, etc.) are the inevitable
technicalities that are associated to the problem.
Blocks: 76097
sr=waterson for everything except the rule tree changes. I haven't been able to
do any performance testing (stuck at home, sick; skeptical of the value of
jrgm's tests run over SERA+DSL), so I guess I'll just hope for the best.
hyatt, you've got the missing link. Note that I answered to dbaron's query about 
the rule bits too in bug 74186 (c.f. "Additional Comments From David Baron 
2001-08-19 14:39")

Marc, how should I go about your requests (carpool, etc) that you mentioned
in your comments above of 2001-09-14 11:38.
As I understand it, it normally works like this:

You need to pick a day when you have the time (from 9am onwards PST) for the
carpool. Have a look at the sheriff schedule (top of tinderbox) to see who is
sheriffing that day, and contact them to arrange the carpool. The tree will
close for verification and you will pull it and merge in your changes. After the
1st round of verification builds the tree will stay closed as you check in, so
you have it all to yourself. They will then spin another round of verif. builds
which will get tested according to defined criteria. The tree will then reopen.

Gerv

OK, I might finish up and be done with it "tomorrow" (remote time...), pending 
sr from hyatt on the last piece. I just looked at the schedule and by 
coincidence, sheriff duties will be assigned to i18n. For the record, I will 
attach an all-in-one patch of what I have so far on this first round.

    
    

    
  


  

    
    

    
  
NB: hyatt, if you prefer, I can move the rulebits from public to private since I 
already have a getter for them.

    
    

    
  
Errata, got the color code wrong - for the schedule, I instead have to contact
Performance (cathleen).

    
    

    
  
To arrange a carpool I think you're better off contacting the build team
(leaf@mozilla.org, granrose@netscape.com, etc.) than the sheriff.

    
    

    
  
Got it touch with the build team, and my carpool request was acknowledged. Also 
updated chofmann's branch landing plan with a link to a latest drop hosted on 
mozilla.org for fast access from your end (but it might take a while to be 
active as usual...)
http://komodo.mozilla.org/planning/branches.cgi

    
    

    
  
As timing wanted it, the link to the latest drop is active...
http://www.mozilla.org/projects/mathml/fonts/mozilla-win32-font-20012609.exe

    
    

    
  
Got sr=hyatt on the style code by email, this completes the r/sr clearance on 
all the bits. Carpool landing has been scheduled for tomorrow Thu, Sept 27 (PST)

    
    

    
  
I ran jrgm's tests comparing the build that rbs provided with the 2001-09-26
build from ftp.mozilla.org. The results (on a pitifully slow 166MHz machine) are
below. It looks like rbs's changes may provide a slight speedup -- yay!

Before (stock mozilla build):

Test id: 3BB1B80ABA
Avg. Median : 6342 msec		Minimum     : 1302 msec
Average     : 6451 msec		Maximum     : 20570 msec

<http://cowtools.mcom.com/page-loader/graph.pl?id=3BB1B80ABA>


After (rbs's build):

Test id: 3BB1BEE797
Avg. Median : 6227 msec		Minimum     : 1302 msec
Average     : 6337 msec		Maximum     : 20367 msec

<http://cowtools.mcom.com/page-loader/graph.pl?id=3BB1BEE797>

    
    

    
  
The changes was landed at mid-day per the carpool schedule, and they have been 
baking in the tree ever since. Resolving as FIXED.

Follow-up: bug 102088 - Cleanup of transitory font APIs...

    
    

    
  
Really marking as FIXED this time.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
Does the resolution as FIXED mean that the issue I originally reported in bug
74186 is now cleared?  I.e. will it now be possible to set different font sizes
for serif and sans-serif fonts?  It certainly seems that the backend code will
support the feature, but I'm wondering about the UI.

    
    

    
  
The UI aspect is being hooked up in bug 61883. To experiment things while
awaiting for that you can do:

user_pref("font.size.[generic].[langGroup]", integer);

For example:
user_pref("font.size.serif.x-western", 20);
user_pref("font.size.sans-serif.x-western", 30);

The UI bug has other font keys, e.g., to see the effect of the font's min-size:
user_pref("font.minimum-size.x-western", 20);

    
    

    
  
rbs, will a decent minimum font size be made default? A lot of users are scared
away from mozilla since it shows the same tiny fonts on some pages as netscape
4.x did.

    
    

    
  
That dilemma often pops up with prefs. Seems a little early to be sure what
might happen. Once the UI is in, it might perhaps be worth trying to have a
default on a milestone to see how it goes in terms of feedback. Personally, when
not testing, I have been browsing with min-size=10px (since there is no point
having text that cannot be read and/or that is tiresome to the eyes).

    
    

    
  
> will a decent minimum font size be made default? A lot of users are scared
> away from mozilla since it shows the same tiny fonts on some pages as 
> netscape 4.x did.

is there a bug open on this?


    
    

    
  
> is there a bug open on this?

Want me to open one, and write that it's spun off from this bug?



    
    

    
  
Layout on BeOS is completely hosed atm since the tinderbox bustage was "fixed"
by just implementing dummy routines.  And Qt was missed entirely.


Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
I believe that OS/2 has the most generic new versions of these routines 
possible.

We made no changes at all to support these new functions.

Please checkout my latest checkin to nsRenderingContextOS2.cpp.

I used trial and error to get the right combination - we might need to do a 
little of this on BeOS and QT.

    
    

    
  
Re-closing the bug. There is a tracker bug for hooking GetTextDimensions() on
other ports - bug 96609. That's the only elment that is missing on these
platforms and bug 96609 was opened for that purposes.


Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
re: OS/2 -- shouldn't be trial and error. What is needed is now well documented
and the minimalist intelligible fallback to have without regression is as done
on GfxMac (or GfxPS), which is what OS2 is now doing, and might have to be done
on other ports.

    
    

    
  
bug 96609 seems to cover coming up with real implementations, rather than stub
implementations.  However, the problem cls mentioned is that layout on BeOS is
completely broken, since DrawString2 does nothing.  BeOS, QT, and photon are
reasonably actively maintained, and someone should come up with a stub patch for
those platforms that will make things work as they have in the past, similar to
what was done on Mac and OS/2.

    
    

    
  
r=rbs, that's the fix-up that is needed to recover the earlier behavior in 
iterim to the proper support.

    
    

    
  
cls, please head over to bug 96609 for the remaining ports even only for fix-up 
patches as this bug is already long and weird. I have attached the proper fix 
for qt on bug 96609.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: