Closed Bug 259035 Opened 20 years ago Closed 20 years ago

GFX Qt port

Categories

(Core Graveyard :: GFX, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zack, Unassigned)

References

Details

Attachments

(1 file, 5 obsolete files)

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 This bug includes the patch with GFX Qt port. The bz2 contains the gfx/src/qt directory with all the relevant files. Reproducible: Always Steps to Reproduce:
Attached file GFX Qt port (obsolete) —
Status: UNCONFIRMED → NEW
Ever confirmed: true
Some comments on part of the patch: Makefile.in: This doesn't do anything: # If not primary toolkit, install in secondary path ifneq (qt,$(MOZ_GFX_TOOLKIT)) INACTIVE_COMPONENT = 1 endif nsDeviceContextQt.cpp: Init contains code that needs to move to SetDPI, since Observe needs it too. Also, it's possible DeviceContextImpl::Observe could listen to pref changes in the future -- so it's probably better to forward pref changes for unknown prefs too. I'm not a big fan of traceMsg for two reasons; 1) it's executing all that logging code in release builds, even if nobody's reading the log in question 2) a way for somebody to cause an unbalanced ENTER/EXIT (e.g., something that triggers an early return that doesn't have an EXIT) could lead to a buffer overflow (probably hard to exploit, though, since the control over the buffer is very limited) Only the first device context created is a preference observer for the screen resolution pref -- after that point they'll all ignore dynamic changes. It's probably better to hang that observer off the module -- although the whole business needs to be refactored since much of the code is common across platforms (and varies between them!). In CreateRenderingContext, the rv tests should be using NS_SUCCEEDED or NS_FAILED, not directly against NS_OK. Also, the error handling could be made a bit more readable by holding the rendering context in an nsCOMPtr and using early returns (although you have your |dmesg| problem). ConvertPixel doesn't seem to be used (nor in the Cairo port). nsDeviceContextQt.h: the forwarding functions should probably at least have an XXX comment that they should be replaced with |using| given an appropriate autoconf test (which we really don't currently have). A bunch of the member variables are unused (at least mWriteable and mNumCells). nsIDrawingSurfaceQt.h: It looks like you don't need the nsIDrawingSurfaceQt interface. It doesn't seem to have any callers (in this patch, anyway). (Neither does nsIDrawingSurfaceWin...) This file should be removed, and nsDrawingSurfaceQt.{h,cpp} should be changed not to implement the interface.
Blocks: 259033
this is due to a recent change I believe: In file included from /home/chb/mozilla/gfx/src/qt/nsGfxFactoryQt.cpp:49: /home/chb/mozilla/gfx/src/qt/nsDeviceContextSpecQt.h:70:2: warning: #warning is a GCC extension /home/chb/mozilla/gfx/src/qt/nsDeviceContextSpecQt.h:70:2: warning: #warning "postscript hardcore disabled" /home/chb/mozilla/gfx/src/qt/nsGfxFactoryQt.cpp: In function `nsresult nsNativeThemeQtConstructor(nsISupports*, const nsIID&, void**)': /home/chb/mozilla/gfx/src/qt/nsGfxFactoryQt.cpp:80: error: cannot allocate an object of type `nsNativeThemeQt' /home/chb/mozilla/gfx/src/qt/nsGfxFactoryQt.cpp:80: error: because the following virtual functions are abstract: ../../../dist/include/gfx/nsITheme.h:88: error: virtual PRBool nsITheme::GetWidgetPadding(nsIDeviceContext*, nsIFrame*, unsigned char, nsMargin*)
Attached file GFX update (obsolete) —
Thanks a lot for the comments. iirc I fixed all the issues David had with the patch, plus updated it to HEAD.
Attachment #158686 - Attachment is obsolete: true
nsNativeComponentLoader: SelfRegisterDll(libgfx_qt.so) Load FAILED with error: /home/chb/builds/qt-trunk/dist/bin/components/libgfx_qt.so: undefined symbol: _ZN18nsFontEnumeratorQt14GetDefaultFontEPKcS1_PPt that symbol is: nsFontEnumeratorQt::GetDefaultFont(char const*, char const*, unsigned short**) hm... this is not exactly a new method... exists since may 2003...
Attachment #159276 - Attachment is obsolete: true
attachment 159369 [details] doesn't seem to deal with these parts of comment 2: > nsDeviceContextQt.cpp: > > Init contains code that needs to move to SetDPI, since Observe needs it too. > > Also, it's possible DeviceContextImpl::Observe could listen to pref changes > in the future -- so it's probably better to forward pref changes for unknown > prefs too. > > Only the first device context created is a preference observer for the > screen resolution pref -- after that point they'll all ignore dynamic > changes. It's probably better to hang that observer off the module -- > although the whole business needs to be refactored since much of the > code is common across platforms (and varies between them!). [note that I'm not necessarily expecting you to do the latter right now, but the simple fix for the problem would be good] > In CreateRenderingContext, ... > Also, the error handling > could be made a bit more readable by holding the rendering context in > an nsCOMPtr and using early returns (although you have your |dmesg| > problem). > > ConvertPixel doesn't seem to be used (nor in the Cairo port). > > nsDeviceContextQt.h: > > the forwarding functions should probably at least have an XXX comment that > they should be replaced with |using| given an appropriate autoconf test > (which we really don't currently have). > > A bunch of the member variables are unused (at least mWriteable and > mNumCells). [also mWindowBorderWidth, mWindowBorderHeight]
(In reply to comment #7) > attachment 159369 [details] doesn't seem to deal with these parts of comment 2: > > nsDeviceContextQt.cpp: > > > > Init contains code that needs to move to SetDPI, since Observe needs it too. Fixed. > > Also, it's possible DeviceContextImpl::Observe could listen to pref changes > > in the future -- so it's probably better to forward pref changes for unknown > > prefs too. Fixed. > > Only the first device context created is a preference observer for the > > screen resolution pref -- after that point they'll all ignore dynamic > > changes. It's probably better to hang that observer off the module -- > > although the whole business needs to be refactored since much of the > > code is common across platforms (and varies between them!). > > [note that I'm not necessarily expecting you to do the latter right now, but the > simple fix for the problem would be good] I'm not yet sure how I want to handle that one. As probably everyone I'm not a big fan of "hey, look at me I'm a magical instance of this class and I do something special!" approach which currently we have in the nsDeviceContextQt. For now I'll update the code to use RegisterCallback which is a cleaner solution anyway but not optimal. > > In CreateRenderingContext, ... > > Also, the error handling > > could be made a bit more readable by holding the rendering context in > > an nsCOMPtr and using early returns (although you have your |dmesg| > > problem). Fixed. > > ConvertPixel doesn't seem to be used (nor in the Cairo port). Fixed. I think that's a method that was in the orinal Qt port and when we started we just updated all that code to a correct Qt code and never even checked whether that method was used anywhere. > > nsDeviceContextQt.h: > > > > the forwarding functions should probably at least have an XXX comment that > > they should be replaced with |using| given an appropriate autoconf test > > (which we really don't currently have). Yeah, our policy in KDE is just not to do it right now. I removed them. BTW, I'm not sure if it's the fact that I'm getting intimate with my desk here (right about now everything starts looking like a bed to me) or I messed up the CreateRenderingContext methods... The sig for CreateRenderingContext(nsIRenderingContext *&aContext) was supposed to be CreateRenderingContextInstance(nsIRenderingContext *&aContext) plus there was an error in that method (it wasn't reffing the aContext after gixing it back). I'm assuming I and Lars have hacked around that horrible bug in other places (having a not intialized context). I fixed what I could now but I'll have to look into that (preferably after it lands in CVS) > > A bunch of the member variables are unused (at least mWriteable and > > mNumCells). > > [also mWindowBorderWidth, mWindowBorderHeight] Fixed. So yeah, thanks a lot for taking the time to help me to find those things :)
Attached file GFX Qt port (obsolete) —
Fixes the problems David reported, stops world hunger, impresses my imaginary girlfriend and lets me go to bed. Or at least two out of four which is still a great percentage (in comparison to ones chances of winning the lottery for example).
Attachment #159369 - Attachment is obsolete: true
Attached file GFX Qt port (obsolete) —
Fixes the Observer issue and fixes the bug where nsImageQt thought the pixmap is always dirty.
Attachment #159550 - Attachment is obsolete: true
Also, using EXTRA_DSO_LIBS and then putting EXTRA_DSO_LIBS in the EXTRA_DSO_LDOPTS would simplify the Makefile (see the Windows gfx Makefile or the layout/build makefile; the gtk gfx one is a bit odd). You're still only registering a pref callback for the first device context (the registration is inside a test of a static variable, and the unregistration is run in all destructors). And I'm not sure how using nsIPref simplified things, although it was a switch *to* something that's deprecated. + nsresult res = gPref->GetIntPref(name.get(), &minimum); + if (NS_FAILED(res)) { + gPref->GetDefaultIntPref(name.get(), &minimum); + } I don't think GetDefaultIntPref helps you with anything.
nsFontMetricsQt.cpp: nsServiceManager::GetService(kPrefCID, NS_GET_IID(nsIPref), (nsISupports**) &gPref); could replace that with CallGetService: CallGetService(kPrefCID, &gPref); but, please use a contractid, and please don't use nsIPref. (nsIPrefBranchInternal lets you register callbacks) you might find my checkin at http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=QT_REBIRTH&branchtype=regexp&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2003-03-09+07%3A10&maxdate=2003-03-09+07%3A20&cvsroot=%2Fcvsroot useful (the UnionRect thing seems still relevant, not sure about the rest) nsRenderingContextQt: static NS_DEFINE_IID(kRenderingContextIID,NS_IRENDERING_CONTEXT_IID); this seems unused. mStateCache = new nsVoidArray(); might this be better as a direct member, rather than a pointer? QPointArray *pts = new QPointArray(aNumPoints); here too
Attached file Qt GFX dir
Fixes issues reported by Christian and David. Unless something huge will happen this is the code that I'll be committing in about four hours.
Attachment #160167 - Attachment is obsolete: true
Committed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: