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•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•