169695 - move gdk hack from nsRenderingContextGTK.cpp

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[kill this account]

nsRenderingContextGTK::my_gdk_draw_text() has nothing to do with the
rendering context and blizzard sees this as an issue so lets move it.

Comment 1

[Christopher Blizzard (:blizzard)]

Patch in 126919 will fix this.

Comment 2

[kill this account]

there is no reason to wait for a 16,000+ line patch to fix this

Comment 3

[Rui Xu]

code issue, QA to [e-mail] for now.

Comment 4

[kill this account]

Attachment: patch to move my_gdk_draw_text out of nsRenderingContextGTK

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 100050 [details] [diff] [review]
patch to move my_gdk_draw_text out of nsRenderingContextGTK

r=smontagu, if you tidy up the indentation in nsXFontNormal.cpp, here:

>-  nsRenderingContextGTK::my_gdk_draw_text(aDrawable, mGdkFont, aGC,
>+  my_gdk_draw_text(aDrawable, mGdkFont, aGC,
>                                           aX, aY, aString, aLength);
		      ^^^^^^^^^^^^^^^^^^^^^^

and here:

>-  nsRenderingContextGTK::my_gdk_draw_text(aDrawable, mGdkFont, aGC,
>+  my_gdk_draw_text(aDrawable, mGdkFont, aGC,
>                                           aX, aY,
>                                           (const char *)aString, aLength*2);
		     ^^^^^^^^^^^^^^^^^^^^^^^

Comment 6

[kinmoz]

Comment on attachment 100050 [details] [diff] [review]
patch to move my_gdk_draw_text out of nsRenderingContextGTK

Just fix the indentation problem that Simon pointed out.

sr=[e-mail]

Comment 7

[kill this account]

checked in

Comment 8

[Christopher Blizzard (:blizzard)]

Hang on a second.  I can't believe this got checked in as is.  There is exactly
one caller of my_gdk_draw_text and that's in nsXFontNormal.cpp  my_gdk_draw_text
is static, that is, it does not require anything from the rendering context or
the font metrics.

Is there any reason why that should not have just been moved directly into
nsXFontNormal.cpp.  I mean, this is the original reason that I brought the
problem up in the first place.  Now they are just in some random "utils" file. 
A junk bin.

Comment 9

[kinmoz]

sigh ... blizzard you are correct. When looking at the patch initially, I
honestly thought I saw it used in nsRenderingContextGTK.cpp() too.

I didn't like the fact that we were creating a new .h and .cpp file for a single
function, but I was going under the assumption that perhaps other shared
routines would be moved into the new file at some point.

Comment 10

[Bolian Yin]

This patch make Mozilla gtk2 build fail.  

Error Message on my Linux:
========================
c++  -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion
-Wpointer-arith -Wbad-function-cast -Wcast-align -Woverloaded-virtual -Wsynth
-Wno-ctor-dtor-privacy -pedantic -Wno-long-long -fshort-wchar -pthread -pipe 
-DDEBUG -D_DEBUG -DDEBUG_mozilla -DTRACING -g -fno-inline -o DumpColors
DumpColors.o          -L../../dist/bin -L../../dist/lib -lgkgfx -L../../dist/bin
-lxpcom  -liberty -L../../dist/bin -lmozjs -L/home/gnome2/gnome-cvs/dist/lib
-lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lgdk_pixbuf-2.0 -lm -lpangox-1.0
-lpango-1.0 -lgobject-2.0 -lgmodule-2.0 -ldl -lglib-2.0  
-L/home/mozilla/mai/src/mozilla/dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl 
 -ldl -lm     
TestColorNames.cpp
c++ -o TestColorNames.o -c -DOSTYPE=\"Linux2.4\" -DOSARCH=\"Linux\" -DOJI 
-I../../dist/include/xpcom -I../../dist/include/string -I../../dist/include/gfx
-I../../dist/include -I../../dist/include
-I/home/mozilla/mai/src/mozilla/dist/include/nspr      -I./../src
-I/usr/X11R6/include   -fPIC  -I/usr/X11R6/include -fno-rtti -fno-exceptions
-Wall -Wconversion -Wpointer-arith -Wbad-function-cast -Wcast-align
-Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -pedantic -Wno-long-function)
nsGdkUtils.cpp:99: `GDK_DRAWABLE_XID' undeclared (first use this
function)
nsGdkUtils.cpp:100: `GDK_GC_XGC' undeclared (first use this function)
nsGdkUtils.cpp:100: `XDrawString' undeclared (first use this function)
nsGdkUtils.cpp:112: `XChar2b' undeclared (first use this function)
nsGdkUtils.cpp:112: parse error before `)'
nsGdkUtils.cpp:125: `XFontSet' undeclared (first use this function)
nsGdkUtils.cpp:125: parse error before `='
nsGdkUtils.cpp:127: `fontset' undeclared (first use this function)
nsGdkUtils.cpp:127: `XmbDrawString' undeclared (first use this
function)
make[5]: *** [nsGdkUtils.o] Error 1
make[5]: Leaving directory `/home/mozilla/mai/src/mozilla/gfx/src/gtk'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/home/mozilla/mai/src/mozilla/gfx/src'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/home/mozilla/mai/src/mozilla/gfx'
make[2]: *** [tier_9] Error 2
make[2]: Leaving directory `/home/mozilla/mai/src/mozilla'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/home/mozilla/mai/src/mozilla'
make: *** [build] Error 2
========================

Comment 11

[Roland Mainz]

Christopher Blizzard wrote
> Is there any reason why that should not have just been moved directly into
> nsXFontNormal.cpp.

Erm...
... and I have to pull it out from gfx/src/x11shared/nsXFontNormal.cpp when we
wnat to share that part between Xlib gfx and GTK+ gfx...
De facto no code in gfx/src/x11shared/ should depend on any GTK+/GDK library
functions.

Comment 12

[Jay Yan]

I would like to close this bug for default build with gtk1 and file a new bug to
track the gtk2 building and will attach the patch soon.

Comment 13

[Jay Yan]

new bug filed at bug 171461, asking review and sr for patch there. thanks

Comment 14

[Jay Yan]

Blizzard already fixed bug 171461, thanks all.

Comment 15

[Christopher Blizzard (:blizzard)]

That has nothing to do with this problem.  The function declarations contain gtk
objects so you've got other things to do first anyway.

Comment 16

[kill this account]

> Is there any reason why that should not have just been moved directly into
> nsXFontNormal.cpp.  

Yes, there is.

The intent of nsXFontNormal.cpp is to factor out the common code int
gfx/src/gtk and gfx/src/xlib so nsXFontNormal.cpp should not have
any gtk/gdb specific code. I am working on this and it will come in
a following patch.

Comment 17

[kill this account]

Factoring the common code between gfx/src/gtk and gfx/src/xlib is a project
Roland and I have been working on slowly for a while now.

Is there any a issue with factoring out the common code between gfx/src/gtk
and gfx/src/xlib?