Closed
Bug 259035
Opened 20 years ago
Closed 20 years ago
GFX Qt port
Categories
(Core Graveyard :: GFX, defect)
Core Graveyard
GFX
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zack, Unassigned)
References
Details
Attachments
(1 file, 5 obsolete files)
32.80 KB,
application/x-tbz
|
Details |
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:
Reporter | ||
Comment 1•20 years ago
|
||
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.
Comment 3•20 years ago
|
||
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*)
Reporter | ||
Comment 4•20 years ago
|
||
Thanks a lot for the comments. iirc I fixed all the issues David had with the patch, plus updated it to HEAD.
Reporter | ||
Updated•20 years ago
|
Attachment #158686 -
Attachment is obsolete: true
Comment 5•20 years ago
|
||
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...
Reporter | ||
Comment 6•20 years ago
|
||
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]
Reporter | ||
Comment 8•20 years ago
|
||
(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 :)
Reporter | ||
Comment 9•20 years ago
|
||
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
Reporter | ||
Comment 10•20 years ago
|
||
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.
Comment 12•20 years ago
|
||
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
Reporter | ||
Comment 13•20 years ago
|
||
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
Reporter | ||
Comment 14•20 years ago
|
||
Committed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•