50998 - XP impl of GetAveCharWidth and remove ifdef _win32 in GfxText

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[rods (gone)]

I need to remove the ifdef _WIN32 in nsGfxTextControlFrame2.cpp and implement a 
cross platform GetAveCharWidth

Comment 1

[ckritzer (gone)]

Engineering reminder.  Reassigning to rods.

Comment 2

[rods (gone)]

This really needs to be done for this release

Comment 3

[Mike Kaply [:mkaply]]

Attachment: OS/2 implemented GetAveCharWidth - can we get an r= for enabling it?

Comment 4

[rods (gone)]

I am all for that r=rods

Comment 5

[buster]

looks ok to me.  sr=buster.  rod, are there other places that need this, or is
this sufficient?

Comment 6

[rods (gone)]

Other than the OS/2 impl that is it, it is good to go.

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Per buster's comment, I found another place to change

Comment 8

[rods (gone)]

Opps, good catch. r=rods

Comment 9

[Mike Kaply [:mkaply]]

Attachment: It would help if I changed the header - I went ahead and checked this in to fix our breakage

Comment 10

[John Keiser (jkeiser)]

OK, so the way to fix this is to make GetAveCharWidth work on all our ports
using a cross-platform GetAveCharWidth.  People can replace them with Real Ones
at a later date if the platform supports them (I'll put big comments in there
saying it's fake and that they should use a platform-specific one if available).

Taking.

Comment 11

[John Keiser (jkeiser)]

Er, taking again

Comment 12

[Rick Ju]

jkeiser,
it's diffcult for us to implement all GetAve for beos/QT/mac/photon.  i'd better
to add it on nsIFontMetrics first, then ask people who know the platform
implement it. 
i know only qt, so i have implent the one for qt. 

what do you think?

Comment 13

[Rick Ju]

Attachment: qt implementation

in all 10 toolkit:

implemented already:  windows os/2 xlib gtk

not implemented yet: mac, photon, beos, QT

the patch is for QT. And we still need *****Mac, BeOS, Photon*******.

if you can do it, we need ur help. Thanks.
when we get patchs for all 3, we can get to change the nsIFontMetrics ...

Comment 14

[John Keiser (jkeiser)]

Rick: for the rest of them, you can just do the same thing we do for GTK, namely
measure the character 'x'.  I think that is a better XP implementation (and
faster) than the one in nsTextControlFrame anyway.  And it will keep our
platforms relatively similar to each other.  If there is a real AveCharWidth
call on the platform of course, it is better to use that.

Comment 15

[Rick Ju]

Attachment: Mac impl

using 'x'

Comment 16

[Rick Ju]

Attachment: photon impl

there was a mAveCharWidth = mSpaceWidth already. 

adding GetAveCharWidth only

Comment 17

[Rick Ju]

Attachment: beos impl

using 'x'

Comment 18

[Rick Ju]

Attachment: nsIFontMetrics modification

removed the #if (_WIN32) || ...

Comment 19

[Rick Ju]

terribly sorry that i can't find all the build env for these platforms to test
these patch .

add some masters of gfx.

HELP needed:
  1. pls check the code and check whether better impl exists.
      i am not familiar with most these platforms.
  2. i dont have env to test them.
      pls kindly test them if u do.

thanks!

Comment 20

[rbs]

These patches look alright to me, sr=rbs on all.

There is a remaining cleanup of #if defined(_WIN32) || defined(XP_OS2) in 
mozilla/layout/html/forms/src/nsGfxTextControlFrame.cpp

While there, also saw a code that computes 'NSToCoordRound(p2t)' too many times
(caming from the patch in bug 99948):

    // round to a multiple of p2t
    nscoord rest = internalPadding%NSToCoordRound(p2t);
    if( rest < NSToCoordRound(p2t) - rest) {
      internalPadding = internalPadding - rest;
    } else {
      internalPadding = internalPadding + NSToCoordRound(p2t) - rest;
    }

While you are there, mind cleaning up that code a little bit, e.g., use:
    nscoord t = NSToCoordRound(p2t);
    nscoord rest = internalPadding % t;
    if( rest < t - rest) { // same as rest < half-of-pixel
      internalPadding -= rest; // round down
    } else {
      internalPadding += t - rest; // round up
    }

Comment 21

[Rick Ju]

thanks, rbs.
About nsGfxTextControlFrame.cpp:
   Do u mean nsTextControlFrame.cpp?
   The file is why i fix this bug. pls refer to #92980.
   Tnanks for ur comments about NSToCoordRound. i will do it in #92980's patch.

Comment 22

[rbs]

>  Do u mean nsTextControlFrame.cpp?

Yes. I typed one of the old names.

>   The file is why i fix this bug. pls refer to #92980.
>   Tnanks for ur comments about NSToCoordRound. i will do it in #92980's patch.

OK.

Comment 23

[John Keiser (jkeiser)]

Comment on attachment 98566 [details] [diff] [review]
qt implementation

fm.widht won't compile--fm.width is better :)

Comment 24

[John Keiser (jkeiser)]

Attachment: Fix nsTextFrame

This makes nsTextFrame use GetAveCharWidth() always instead of guessing that
the average width is 10 :)  nsTextControlFrame will get its update when Rick
checks in his patch to that bug.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 99462 [details] [diff] [review]
photon impl

hm... currently, mAveCharWidth of the photon port contains the width of a space
rather than an 'x' like the other ports. as that variable is currently unused,
I would change it to match the others.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 98566 [details] [diff] [review]
qt implementation

+  PRUnichar averageX = (PRUnichar)'x';

use QChar here, because that's what fm.width takes.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 99461 [details] [diff] [review]
Mac impl

r=biesi

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

r=biesi on all patches if my comments are addressed (and the width typo gets fixed)
however, you might want to get r=[e-mail] or
r=[e-mail] for the BeOS patch instead, as they are the ports
maintainers, afaik

Comment 29

[John Keiser (jkeiser)]

rbs, could you sr the "Fix nsTextFrame" patch as well?  It seems best to get it
checked in now or it will be one of those "yeah, we need to do that but we won't
until Gecko 3.0" things :)

Comment 30

[rbs]

Comment on attachment 99540 [details] [diff] [review]
Fix nsTextFrame

sr=rbs

Comment 31

[Rick Ju]

Comment on attachment 98566 [details] [diff] [review]
qt implementation

>Index: nsFontMetricsQT.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/qt/nsFontMetricsQT.h,v
>retrieving revision 1.8
>diff -u -r1.8 nsFontMetricsQT.h
>--- nsFontMetricsQT.h	30 Oct 2001 22:57:49 -0000	1.8
>+++ nsFontMetricsQT.h	10 Sep 2002 09:17:04 -0000
>@@ -138,6 +138,7 @@
>     NS_IMETHOD  GetMaxAscent(nscoord &aAscent);
>     NS_IMETHOD  GetMaxDescent(nscoord &aDescent);
>     NS_IMETHOD  GetMaxAdvance(nscoord &aAdvance);
>+    NS_IMETHOD  GetAveCharWidth(nscoord &aAveCharWidth);
>     NS_IMETHOD  GetFont(const nsFont *&aFont);
>     NS_IMETHOD  GetLangGroup(nsIAtom** aLangGroup);
>     NS_IMETHOD  GetFontHandle(nsFontHandle &aHandle);
>@@ -201,6 +202,7 @@
>     nscoord          mMaxAscent;
>     nscoord          mMaxDescent;
>     nscoord          mMaxAdvance;
>+    nscoord          mAveCharWidth;
>     nscoord          mXHeight;
>     nscoord          mSuperscriptOffset;
>     nscoord          mSubscriptOffset;
>Index: nsFontMetricsQT.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/qt/nsFontMetricsQT.cpp,v
>retrieving revision 1.25
>diff -u -r1.25 nsFontMetricsQT.cpp
>--- nsFontMetricsQT.cpp	7 Sep 2002 17:10:09 -0000	1.25
>+++ nsFontMetricsQT.cpp	10 Sep 2002 09:17:05 -0000
>@@ -644,6 +644,7 @@
>   mMaxAscent = 0;
>   mMaxDescent = 0;
>   mMaxAdvance = 0;
>+  mAveCharWidth = 0;
>   mXHeight = 0;
>   mSuperscriptOffset = 0;
>   mSubscriptOffset = 0;
>@@ -913,6 +914,9 @@
>   mEmHeight = nscoord(fm.height() * f);
>   mMaxAdvance = nscoord(fm.maxWidth() * f);
> 
>+  PRUnichar averageX = (PRUnichar)'x';
>+  mAveCharWidth = nscoord(fm.width(averageX) * f);
>+
>   mEmAscent = mMaxAscent;  
>   mEmDescent = mMaxDescent;
>  
>@@ -1022,6 +1026,12 @@
> NS_IMETHODIMP nsFontMetricsQT::GetMaxAdvance(nscoord &aAdvance)
> {
>   aAdvance = mMaxAdvance;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP nsFontMetricsQT::GetAveCharWidth(nscoord &aAveCharWidth)
>+{
>+  aAveCharWidth = mAveCharWidth;
>   return NS_OK;
> }
>

Comment 32

[Rick Ju]

Attachment: new qt impl ( widht -> width )

Comment 33

[Rick Ju]

Attachment: new qt impl ( widht typo and using QChar )

Comment 34

[Rick Ju]

Attachment: New photon patch ( using 'x' instead of ' ' )

Comment 35

[Sergei Dolgov]

Comment on attachment 99463 [details] [diff] [review]
beos impl

seems ok. Ohh, and i missed when AveCharWidth changed to "x" width instead of
Finnish developer name-based :)

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, new patches look good to me. r=biesi on all non-obsolete patches here.

looks like you're ready to checkin.

Comment 37

[John Keiser (jkeiser)]

I will check in.  I don't think Rick has cvs access yet.

Comment 38

[John Keiser (jkeiser)]

Fix checked in.  Let's see what this does to ports before resolving ;)

Comment 39

[John Keiser (jkeiser)]

Well, that went alarmingly smoothly.  Yippee, fix checked in to stay!