80224 - Xlib-toolkit's nsFontMetricsXlib.cpp is missing tons of nsFontCharSetMap mappings...

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Roland Mainz]

Xlib-toolkit has problems finding matching fonts on pages which need
non-ISO-Latin-1 fonts...

Looking at mozilla/gfx/src/gtk/nsFontmetricsGTK.cpp and comparing it with
nsFontMetricsXlib.cpp - it looks like there are tons of nsFontCharSetMap
mappings missing...

Note that a patch should also fix mozilla/gfx/src/xprint/nsFontmetricsXP.cpp as
this is a 1:1 copy+s/Xlib/Xp/ from nsFontMetricsXlib.cpp (after checkin of bug
78548)...

Comment 1

[Roland Mainz]

Patch is simple (setting milestone to 0.9.1) - but requires checkin of bug 78548
first...

Accepting bug...

Comment 2

[Roland Mainz]

nsDeviceContextXlib::GetMetricsFor()/nsDeviceContextXp::GetMetricsFor() needs to
be implemented, too.

Example (from my test build - needs further testing!!):
-- snip --
NS_IMETHODIMP nsDeviceContextXp::GetMetricsFor(const nsFont& aFont,
nsIFontMetrics  *&aMetrics)
{
    GetLocaleLangGroup();
    return GetMetricsFor(aFont, mLocaleLangGroup, aMetrics);
}
 
NS_IMETHODIMP nsDeviceContextXp::GetMetricsFor(const nsFont& aFont, 
                        nsIAtom* aLangGroup, nsIFontMetrics  *&aMetrics)
{
PRInt32         n,cnt;
nsresult        rv;

  // First check our cache
  n = mFontMetrics.Count();

  for (cnt = 0; cnt < n; cnt++)
  {
    aMetrics = (nsIFontMetrics*) mFontMetrics.ElementAt(cnt);

    const nsFont* font;
    aMetrics->GetFont(font);
    nsCOMPtr<nsIAtom> langGroup;
    aMetrics->GetLangGroup(getter_AddRefs(langGroup));
    if (aFont.Equals(*font) && (aLangGroup == langGroup.get()))
    {
      NS_ADDREF(aMetrics);
      return NS_OK;
    }
  }

  // It's not in the cache. Get font metrics and then cache them.
  nsIFontMetrics* fm = new nsFontMetricsXp();
  if (nsnull == fm) {
    aMetrics = nsnull;
    return NS_ERROR_FAILURE;
  }

  rv = fm->Init(aFont, aLangGroup, this);

  if (NS_OK != rv) {
    aMetrics = nsnull;
    return rv;
  }

  mFontMetrics.AppendElement(fm);
  NS_ADDREF(fm);     // this is for the cache


  for (cnt = 0; cnt < n; cnt++){
    aMetrics = (nsIFontMetrics*) mFontMetrics.ElementAt(cnt);
    const nsFont  *font;
    aMetrics->GetFont(font);
  }

  NS_ADDREF(fm);      // this is for the routine that needs this font
  aMetrics = fm;
  return NS_OK;
}
-- snip --

I'll wait for checkin of patches in bug 78548 and then fix this issue...

Comment 3

[Roland Mainz]

Found testcase (http://people.netscape.com/ftang/demo/utf8form.html).

ftang: can you have a quickish look at the Xlib-toolkit code
(mozilla/gfx/src/xlib/) and check if you can see the issue why
http://people.netscape.com/ftang/demo/utf8form.html is not viewed correctly
(I'll add a screenshot). I still cannot get it working... ;-((

Comment 4

[Roland Mainz]

Attachment: Screenshot from Xlib-toolkit Mozilla viewing http://people.netscape.com/ftang/demo/utf8form.html

Comment 5

[Roland Mainz]

Attachment: Screenshot from GTK+-toolkit Mozilla viewing http://people.netscape.com/ftang/demo/utf8form.html

Comment 6

[Roland Mainz]

Added screenshot from GTK+ toolkit viewing the same URL - this one looks OK so
far...

Comment 7

[Roland Mainz]

Retargeting, new milestone is 0.9.2 - first bug 81311 has to be fixed...

Comment 8

[pocemit]

Attachment: patch to nsFontMetricsXlib.cpp,h to sync with GTK version

Comment 9

[pocemit]

This patch includes FontMetrics changes from 66082 (very little if I remember
correctly) plus syncs the source with the GTK version (I assume that one works
the best).
Xlib font output now matches GTK font output pixel by pixel, at least on my
system.

tim

Comment 10

[Roland Mainz]

Patch looks nice - but it relies on bug 66082... adding dependicy...

Comment 11

[Roland Mainz]

Requesting r=/sr= for that patch... tor/blizzard ?

Comment 12

[pocemit]

Attachment: New patch, applies cleanly against mozilla/, etc

Comment 13

[pocemit]

Let's wait on bug 16688 until its accepted - it has a macro for one of the files
in the unicode encoder directory, and I'd rather wait for that to be in than
duplicate it in xlib code. The patch will be pretty straightforward to adapt to
Xlib though.

Comment 14

[pocemit]

By the way it does not rely on bug 66082 - I just pulled orig versions of
nsFontMetricsXlib from cvs and made a patch against them.
So with approval this should go in. The code is almost line-by-line copy of the
GTK version, and its organized in the same way, so it should be possible to
track GTK changes in here easily.

There are still issues with GTK font metrics though, but I am sure there are
other bugs open on that.

Comment 15

[Roland Mainz]

Welcome to patch _hell_... ;-(((
I cannot apply the patches, both old and new after I patched my tree with
patches in bug 66082...
Uhm... pocemit, you do not have a patch which fits into patched tree (e.g.
appiles cleanly after patches in bug 66082 have been appiled) ?
I am currently working on a new patch for bug 66082 to get sr=...

Comment 16

[pocemit]

Okay, I fucked up. This DOES indeed depend on 66082.
The patch sequence should be like this:

1. Clean tree
2. Patch all of 66082
3. Get clean nsFontMetricsXlib.*
4. patch this

I am sorry. I totally missed that font metrics were converted to gc-cache as well.
This is why it is so critical these patches go in immediately :)

Comment 17

[Roland Mainz]

yes... but it looks bug 66082 will be "in" the tree at the end of this week
(hopefully... hopefully...)... :-))

pocemit:
Wanna file a new patch for the tree after checkin of latest patch in bug 66082,
please ? I'd like to get this r=/sr='ed as _soon_ as possible...

Comment 18

[Roland Mainz]

pocemit:
Once bug 66082 is "in" the tree:
Please file a new patch incl. solution for bug 16688 (just copy the macros from
intl/uconv/public/nsIUnicodeEncoder.h and wrap them with #ifndef
ENCODER_BUFFER_ALLOC_IF_NEEDED // then define our own macros - for now until
patch in bug 16688 get's checked-in) and request r=/sr= for it. I'll be back on
sunday/monday...

Comment 19

[pocemit]

Okay, as requested by gisburn, here is the latest patch, ready for review.

As mentioned earlier in the comments, this patch brings Xlib port
nsFontMetricsXlib.cpp into sync with the GTK version, adding things like
language groups, proper font matching, etc.
This is a cut & paste + adapt from the GTK port, so there shouldn't be any bugs
here. This also includes the patch from bug 16688 which removes the 512
character limit for things like DrawString/GetWidth/etc.

Reviews are welcome.

Comment 20

[pocemit]

Attachment: newest patch for Xlib font metrics

Comment 21

[Roland Mainz]

Requesting r=/sr= for that patch, please...

Comment 22

[Roland Mainz]

Lates patch looks good and works OK (even with ISO-8859-2, japanese and thai
pages). 

r=gisburn ([e-mail])

Comment 23

[Roland Mainz]

Oh... yes... I forgot... reassining to [e-mail] - he's doing
the main work... :-)

Comment 24

[Roland Mainz]

Requesting sr= tor/blizzard ?

Comment 25

[Roland Mainz]

pocemit:
Can you view http://www.walla.co.il/ correctly (hebrew) with your Xlib-toolkit
build ?

Comment 26

[tor]

sr=tor

Comment 27

[Roland Mainz]

Requesting a= ...
This patch is simple and the risk is minor as only Xlib-tookit code is patched -
not part of default build...

Comment 28

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for the trunk

Comment 29

[Roland Mainz]

[as discussed in IRC #mozilla]
CC:'ing mkaply for checkin...

Comment 30

[Mike Kaply [:mkaply]]

Fix checked in

Comment 31

[Teruko Kobayashi]

[e-mail], please verify this bug.

Comment 32

[Roland Mainz]

Verifying. Works perfectly... :-)))