99509 - Clean-up GTK+ and Xlib gfx code

Status: RESOLVED WONTFIX

(Core :: XUL, enhancement)

Description

[Roland Mainz]

I quoted this idea multiple times this month in various bugs that someone should
clean-up the GTK+ and Xlib gfx code. Xlib is mainly "done" - now it's the time
to pull over the improvements from Xlib gfx to GTK+ gfx code.

This includes:
- Use nsCOMPtr instead of manual refcounting
- Use unique class names (to avoid StaticBuild issues and make LXR search more
useable)
- Use |const char *| instead of |char *|
- Kill compiler warnings (both gcc and Sun Workshop)
* more stuff...

Comment 1

[Roland Mainz]

Accepting bug...

Comment 2

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

Could you fix bug 97969 while you're cleaning up here?

Comment 3

[Roland Mainz]

dbaron:
In theory yes... but my problem is that I do not know much about Mozilla's
string code (--> "helpwanted"... or "hintswanted")... ;-(

Comment 4

[Roland Mainz]

Attachment: Patch for 2001-09-08-08-trunk

Comment 5

[Roland Mainz]

Comments:
It may be usefull to read the reviewer/superreviewer comments in bug 90380 to
understand this monster. Most of the code already exist in Xlib gfx code - this
is only a port to GTK+ gfx code (except the parts which port from code from GTK+
--> Xlib gfx code...) ...
The cleanup is little bit incomplete... but the current patch is already ~90k in
size, other minor nits can be done later...

dbaron, wanna do the r=, please ?

Comment 6

[kill this account]

I don't really understand what this is doing:

    -NS_IMPL_ISUPPORTS1(nsDeviceContextGTK, nsIDeviceContext)
    -
     nsDeviceContextGTK::nsDeviceContextGTK()
    +  : DeviceContextImpl()

Why are we adding GTK to more places? I thought we were
moving toward Xlib. Or are you trying to make the code match line
for line. If so, to what advantage?

    -struct nsFontCharSetMap;
    -struct nsFontFamilyName;
    -struct nsFontPropertyName;
    +/* Prototype some structs */
    +struct nsFontCharSetMapGTK;
    +struct nsFontFamilyNameGTK;
    +struct nsFontPropertyNameGTK;

Minor nit: I would prefer to avoid using "my":

    -    XChar2b _16bit_space;
    -    _16bit_space.byte1 = 0;
    -    _16bit_space.byte2 = ' ';
    +    XChar2b my16bit_space;
    +    my16bit_space.byte1 = 0;
    +    my16bit_space.byte2 = ' ';

Comment 7

[kill this account]

Roland: instead of adding xxxGTK what if we add a neutral tag like xxxXMoz (or 
even xxxX) so we can work toward the files being identical and then
merged into a single file?

Does anyone know exactly what the gfs/src/gtk version really needs from
GTK?

Comment 8

[Roland Mainz]

> I don't really understand what this is doing:
> 
>     -NS_IMPL_ISUPPORTS1(nsDeviceContextGTK, nsIDeviceContext)
>     -
>      nsDeviceContextGTK::nsDeviceContextGTK()
>     +  : DeviceContextImpl()

Well... everything is already "done" by the superclass |DeviceContextImpl| - why
should we replicate this here, too ? 
BTW: That code works prefectly in Xlib and Xprint stuff since eternity...

> Why are we adding GTK to more places? I thought we were
> moving toward Xlib. Or are you trying to make the code match line
> for line. If so, to what advantage?
> 
>     -struct nsFontCharSetMap;
>     -struct nsFontFamilyName;
>     -struct nsFontPropertyName;
>     +/* Prototype some structs */
>     +struct nsFontCharSetMapGTK;
>     +struct nsFontFamilyNameGTK;
>     +struct nsFontPropertyNameGTK;

I want to have _unique_ class&struct names, even per toolkit.
I had some problems as I started to reuse Xlib gfx code in Xprint module which
screwed-up StaticBuild - I had to rename all classes and add the "Xlib" string
to the names.
This time I am doing the same for GTK+ side - to get other toolkits
(Qt/Win/Photon) to do the same. At least it's usefull to get better search
results from LXR and unstable.elemental (see
http://unstable.elemental.com/mozilla/build/latest/mozilla/gfx/dox/classes.html)

> Minor nit: I would prefer to avoid using "my":
> 
>     -    XChar2b _16bit_space;
>     -    _16bit_space.byte1 = 0;
>     -    _16bit_space.byte2 = ' ';
>     +    XChar2b my16bit_space;
>     +    my16bit_space.byte1 = 0;
>     +    my16bit_space.byte2 = ' ';

Aww... why ? :-)

> ------- Additional Comments From Brian Stell 2001-09-13 23:01 -------
> 
> Roland: instead of adding xxxGTK what if we add a neutral tag like xxxXMoz (or 
> even xxxX) so we can work toward the files being identical and then
> merged into a single file?

See comment above...

> Does anyone know exactly what the gfs/src/gtk version really needs from
> GTK?

Nothing. Only the widget code (widget/src/) needs to be ported to plain Xlib,
but thats less than a week of work (we have a working version in
widget/src/xlib/ - except that the X Input method stuff is missing).
But I won't start this discussion again... it just ends-up in a massive
flame-war from the GDK/GTK+ people here...

Comment 9

[Roland Mainz]

Marking WONTFIX.
I cannot work on Mozilla code for the next month thanks to endico.

Comment 10

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

Reopening.  Whether or not gisburn is on IRC at the moment, this is something
that should be done.  Temporarily reassigning to myself.

Comment 11

[Roland Mainz]

OKOK... got it. I can still do this thing - but I cannot give explanations via
IRC not can I guide reviewers/superreviewers through the patch via IRC... ;-(

More ToDO:
- Port architectural changes in Xlib gfx code introduced in bug 87285 to GTK+
code (incl. comments for the font change stuff requested by kin)
- Document the code. We should add more comments how the code works and use
JavaDoc-style comments for the XPCOM-style interfaces (take a look at
http://unstable.elemental.com/mozilla/build/latest/mozilla/gfx/dox/classes.html
:-)

Comment 12

[Roland Mainz]

Comment on attachment 49290 [details] [diff] [review]
Patch for 2001-09-08-08-trunk

I am going to file a new patch after all my builds recompiled (~18-24h from now) ...

Comment 13

[Roland Mainz]

Taking again... but I doubt I can "fix" this in 0.9.5 ...

Comment 14

[Christopher Blizzard (:blizzard)]

OK, I'm holding off reviewing until roland says "go."  Just drop me some email
when you are ready again.

Comment 15

[Roland Mainz]

blizzard:
thanks!

----

Adding dependicy to bug 94327 because it touches huge piles of
nsFontMetrics{Xlib,GTK}.* code. I prefer to wait with this one until the noise
from these high-priority bugs (is there any other bug which touches
gfx/src/{gtk,xlib}/ in the  next three weeks ?) is gone before I go and file
another 100KB patch for this one... =:-)

----

More ToDo:
- Add _BIG_ link to the (hopefully soon) updated docs in bug 90384 at the top
of  mozilla/gfx/src/{gtk,xlib}/nsFontMetrics{GTK,Xlib}.{cpp,h} ...

Comment 16

[Roland Mainz]

blizzard:
When do you have time to give me a quick r=/sr= for the new (upcoming patch)
patch ? I'd like to file a patch when you have time and discuss/review it via
IRC chat if possible - I'd like to get it "in" quick quick quick because the
patch is xx@@!!-_huge_) ...

Comment 17

[Jonadab the Unsightly One (Nathan Eady)]

Looks like this won't make it in for 0.9.6.

Comment 18

[Asa Dotzler [:asa]]

0.9.6 is long gone. -> 0.9.7

Comment 19

[Mike]

Attachment: Cleaned up signed/unsigned compiler warnings

cleaned by recasting variables on lines complier complained.  Split warning
where Image size <=0 in separate <0 and =0.

Comment 20

[kill this account]

This appear to be a c++ file. Could it be attached as a patch (ie: cvs diff -u 
and also not mark it octet stream).

Comment 21

[Mike]

Attachment: Cleaned up Signed/unsigned complier warnings

reattached as a patch.	My changes are in between comments //my code   //my
code ends.  Src file from ftp.	My connection too slow for CVS.
I don't have authority to remove my old attachment.  Anyone who does feel free
to blow away.

Comment 22

[kill this account]

> I don't have authority to remove my old attachment.  Anyone who does feel
> free to blow away.

Are you able to use the "Edit" "Actions" and mark it as obsolete?

Attachment 72002 [details] [diff] does not look like a patch to me. Did you make it by
doing a 'cvs diff <filepath/filename>' ?

Comment 23

[Mike]

having problems connecting to cvs.  will try again. sorry.

Comment 24

[Mike]

Attachment: recast variables to clean up compile warnings

produced with cvs diff.  Pulled src code from 2002030407_0.9.9-0_rh7.  Tried to
mark both my other attachments as obsolete by receive this error message "You
must be authorized to make changes to attachments to make attachments obsolete
when creating a new attachment."

Comment 25

[kill this account]

To the senior staff: Am I missing something here?
Is there some reason why we are doing this without consulting the author of 
this code, tor?

(Did I cc: the right tor?)

Comment 26

[Mike]

could someone please review my patch?

Comment 27

[kill this account]

attachment 72527 [details] [diff] [review]:

-  if (destX==mWidth)
+  if (destX== (unsigned int) mWidth)

Spacing seems odd.

-  for (PRInt32 y=0; y<readHeight; y+=compY) {
+  for (PRInt32 y=0; (unsigned int)y < (unsigned int)readHeight; (unsigned 
int)y+=compY) {
     if (y==0) {
-      compY = PR_MIN(mHeight-destY, readHeight-y);
+      compY = PR_MIN((unsigned int)mHeight-destY, (unsigned int)readHeight-y);

Do we know what type these should be?

Is there a less clumsy way to do this?

Comment 28

[Mike]

I will try and find a less clumsy way to fix.  The compiler had been complaining
about comparing signed and unsigned int so I would assume they are signed ints.

Comment 29

[Mike]

Attachment: 2nd version of nsImageGTK.cpp warning clean up

Hope this better.

Comment 30

[Peter Weilbacher]

gtk is gone, we now only use Gtk2 in Thebes --> WONTFIX