The default bug view has changed. See this FAQ.

Status

Core Graveyard
Ports: Qt
VERIFIED FIXED
13 years ago
9 years ago

People

(Reporter: Zack Rusin, Assigned: timeless)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.7) Gecko/20040823 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.7) Gecko/20040823 Firefox/0.9.3

In this bug I'll attach the build system patch that incorporates the changes
needed to build the Qt port.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
(Reporter)

Comment 1

13 years ago
Created attachment 158684 [details] [diff] [review]
Qt Mozilla build patch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 158684 [details] [diff] [review]
Qt Mozilla build patch

 # Add explicit X11 dependency when building against X11 toolkits
-ifneq (,$(filter gtk gtk2 xlib,$(MOZ_WIDGET_TOOLKIT)))
+ifneq (,$(filter gtk gtk2 qt xlib,$(MOZ_WIDGET_TOOLKIT)))
 LIBS += $(XLDFLAGS) $(XLIBS)

we suck... there's this nice MOZ_X11 subst in configure.in, although
autoconf.mk.in doesn't use it...

mozilla patches don't need to contain configure diffs, it is autogenerated upon
checkin


note to self: widget patch is in Bug 259036. Are the gfx changes available
anywhere?
Attachment #158684 - Flags: review?(bsmedberg)
Comment on attachment 158684 [details] [diff] [review]
Qt Mozilla build patch

>Index: configure.in

>+	dnl The following warning breaks Qt
>+	if test ! "$QTDIR"; then
>+    	_WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS}"
>+	fi

What's this about? Did you forget something?

>+qt)
>+	MOZ_ENABLE_QT=1
>+    MOZ_ENABLE_XREMOTE=1
>+	TK_CFLAGS='$(MOZ_QT_CFLAGS)'
>+	TK_LIBS='$(MOZ_QT_LDFLAGS)'
>+	AC_DEFINE(MOZ_WIDGET_QT)

Whitespace nit: follow existing conventions (use spaces here, not tabs)

>+dnl    AM_PATH_QT($QT_VERSION,,
>+dnl      AC_MSG_ERROR(Test for QT failed.))

What's up? We don't generally check in code which is commented-out.

>-if test "$MOZ_ENABLE_GTK" \
>+if test "$MOZ_ENABLE_GTK" || test "$MOZ_ENABLE_QT" \
> || test "$MOZ_ENABLE_XLIB" \
> || test "$MOZ_ENABLE_GTK2"

grr, please be consistent 

>Index: embedding/browser/Makefile.in

>-DIRS=webBrowser build chrome
>+DIRS=webBrowser build chrome qt

Do we build this dir even if qt is not the toolkit in use?

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>+#ifdef MOZ_WIDGET_QT
>+#undef CursorShape /*X.h defines it as 0,
>+                     qnamespace.h makes an enum type by that name
>+                   */
>+#endif

I would prefer "#ifdef CursorShape instead of #ifdef MOZ_WIDGET_QT

>Index: widget/src/xpwidgets/Makefile.in

> 		  uconv \
> 		  unicharutil \
>+		docshell view intl \
> 		  $(NULL)

follow existing convention, one dir per line, with consistent
tab-tab-space-space indenting

Fix those nits and attach a new patch. I also want jst to review the changes to
the plugin code.
Attachment #158684 - Flags: superreview?(jst)
Attachment #158684 - Flags: review?(bsmedberg)
Attachment #158684 - Flags: review+
Comment on attachment 158684 [details] [diff] [review]
Qt Mozilla build patch

sr=jst with bsmedbergs nits fixed.
Attachment #158684 - Flags: superreview?(jst) → superreview+
(Reporter)

Comment 5

13 years ago
Actually this baby is essentially what you used to have in the tree for the 
old Qt port. I assumed that one was already reviewed and ok. All yours truly 
did was boost the Qt requirement and add the embedding dir (btw, yeah, that 
one needs to be in ifdefs as well).  
 
I'll update it after work and will send an update tomorrow morning. And to 
answer the question the full set of patches for the Qt port is in: 259035 
(GFX), 259036 (widget), 259037 (webshell) and 259038 (embedding). I guess it'd 
be nice to have a top level bug depending on those to follow the general 
progress of the merge but without internet connection at home I can't really 
do it at the moment. 
Depends on: 259035, 259036, 259037, 259038
(Reporter)

Comment 6

13 years ago
(In reply to comment #3) 
> (From update of attachment 158684 [details] [diff] [review]) 
> >Index: configure.in 
>  
> >+      dnl The following warning breaks Qt 
> >+      if test ! "$QTDIR"; then 
> >+      _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS}" 
> >+      fi 
>  
> What's this about? Did you forget something? 
 
It was in the original patch. I removed it. 
 
> >+qt) 
> >+      MOZ_ENABLE_QT=1 
> >+    MOZ_ENABLE_XREMOTE=1 
> >+      TK_CFLAGS='$(MOZ_QT_CFLAGS)' 
> >+      TK_LIBS='$(MOZ_QT_LDFLAGS)' 
> >+      AC_DEFINE(MOZ_WIDGET_QT) 
>  
> Whitespace nit: follow existing conventions (use spaces here, not tabs) 
 
That's also something that has been orginally in your tree. BTW, you don't 
really have a convention there, xlib, photon, cairo-xlib use tabs - others 
spaces. I switched Qt to spaces, but it would make sense to make it all 
consistent if the spaces are a convention. 
 
> >+dnl    AM_PATH_QT($QT_VERSION,, 
> >+dnl      AC_MSG_ERROR(Test for QT failed.)) 
>  
> What's up? We don't generally check in code which is commented-out. 
 
Also something that was orginally in your tree. Fixed that. 
 
> >-if test "$MOZ_ENABLE_GTK" \ 
> >+if test "$MOZ_ENABLE_GTK" || test "$MOZ_ENABLE_QT" \ 
> > || test "$MOZ_ENABLE_XLIB" \ 
> > || test "$MOZ_ENABLE_GTK2" 
>  
> grr, please be consistent  
 
Also not me. Fixed :) 
 
> >Index: embedding/browser/Makefile.in 
>  
> >-DIRS=webBrowser build chrome 
> >+DIRS=webBrowser build chrome qt 
>  
> Do we build this dir even if qt is not the toolkit in use? 
 
Yap, that was wrong. I just added it for us and forgot to remove it. 
 
> >Index: modules/plugin/base/src/nsPluginHostImpl.cpp 
>  
> >+#ifdef MOZ_WIDGET_QT 
> >+#undef CursorShape /*X.h defines it as 0, 
> >+                     qnamespace.h makes an enum type by that name 
> >+                   */ 
> >+#endif 
>  
> I would prefer "#ifdef CursorShape instead of #ifdef MOZ_WIDGET_QT 
 
Makes sense. Moved it. 
 
> >Index: widget/src/xpwidgets/Makefile.in 
>  
> >                 uconv \ 
> >                 unicharutil \ 
> >+              docshell view intl \ 
> >                 $(NULL) 
>  
> follow existing convention, one dir per line, with consistent 
> tab-tab-space-space indenting 
 
That wasn't really a problem. This part was just plain wrong. I did it because 
we had some problems regenerating the Makefile because of some funky things I 
did and since I didn't feel like messing with it at the time I just made the 
whole thing link against it. Fixed it. 
Thanks for taking a look at it. 
(Reporter)

Comment 7

13 years ago
Created attachment 158877 [details] [diff] [review]
Qt Mozilla build patch 2
(Reporter)

Updated

13 years ago
Attachment #158684 - Attachment is obsolete: true
hm...
aclocal: configure.in: 3343: macro `AM_PATH_QT' not found in library
(Reporter)

Comment 9

13 years ago
(In reply to comment #8) 
> aclocal: configure.in: 3343: macro `AM_PATH_QT' not found in library 
Oh, yeah, the patch should of course not have the : 
+    AM_PATH_QT($QT_VERSION,, 
+      AC_MSG_ERROR(Test for Qt failed.)) 
lines. Sorry about that. 
 
 
 
in allmakefiles.sh:
widget/src/qt

that should be widget/src/qt/Makefile

otherwise you'll have a file called qt in the objdir, which causes the build to
fail...
(Reporter)

Comment 11

13 years ago
Created attachment 159279 [details] [diff] [review]
Qt build patch

Fixes the allmakefiles issue plus updates to cleanly apply with HEAD
(Reporter)

Updated

13 years ago
Attachment #158877 - Attachment is obsolete: true
Comment on attachment 159279 [details] [diff] [review]
Qt build patch

+    MOZ_ENABLE_XREMOTE=1

so does qt actually support xremote? because if it doesn't, this will do bad
things, the user-visible one being that bookmarks aren't loaded in browser
windows (well, due to sucky error handling, but still)
(Reporter)

Comment 13

13 years ago
Created attachment 159370 [details] [diff] [review]
build patch 4

No, it doesn't I was entertaining the thought of supporting it and didn't think
that adding something in will cause such problems.
Attachment #159279 - Attachment is obsolete: true
(Reporter)

Comment 14

13 years ago
I just updated gfx, widget and embedding patches. Could someone please either
let me know whether there's anything wrong with them or commit them?
so, it seems that currently embedding/browser/qt is never built. shouldn't it be
built, similar to embedding/browser/gtk, by mozilla/Makefile.in? (maybe it
shouldn't, I don't know :) )

(Reporter)

Comment 16

13 years ago
I disabled it for a bit. Now it's actually working fine so we can enable it by
default. I'm not 100% committed to this api yet so I don't want to have people
jumping on it yet. I can enable it though if that's easier.
(Reporter)

Comment 17

13 years ago
Qt Mozilla is now in CVS.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 18

13 years ago
I just tried to build a Qt Seamonkey and it seems the file qlist.h is missing.
Or am I doing something wrong? I used the following .mozconfig:

ac_add_options --enable-freetype2
ac_add_options --enable-crypto
ac_add_options
--enable-extensions=wallet,irc,help,inspector,typeaheadfind,xml-rpc,xmlextras,venkman,cookie,content-packs,transformiix,universalchardet,webservices,spellcheck,gnomevfs,negotiateauth,sroaming
ac_add_options --enable-svg
ac_add_options --enable-svg-renderer-libart
ac_add_options --disable-installer
ac_add_options --enable-debug
ac_add_options --disable-optimize
mk_add_options MOZ_INTERNAL_LIBART_LGPL=1
export MOZ_INTERNAL_LIBART_LGPL=1
ac_add_options --enable-default-toolkit=qt
ac_add_options --with-qt
ac_add_options --with-qtdir=/usr/share/qt3
ac_add_options --enable-chrome-format=symlink
(Reporter)

Comment 19

13 years ago
qlist.h is a deprecated header which I simply forgot to removed and apperently 
some distros stopped including it. Either way I just fixed it. 
vrfy fixed. qt is now in the tree.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.