Status

Core Graveyard
GFX
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Zack Rusin, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

32.80 KB, application/x-tbz
Details
(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

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

13 years ago
Created attachment 158686 [details]
GFX Qt port
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*)
(Reporter)

Comment 4

13 years ago
Created attachment 159276 [details]
GFX update

Thanks a lot for the comments. iirc I fixed all the issues David had with the
patch, plus updated it to HEAD.
(Reporter)

Updated

13 years ago
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...
(Reporter)

Comment 6

13 years ago
Created attachment 159369 [details]
fixes the lack of implementation of the fontenumerator method
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

13 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

13 years ago
Created attachment 159550 [details]
GFX Qt port

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

13 years ago
Created attachment 160167 [details]
GFX Qt port

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

(Reporter)

Comment 13

13 years ago
Created attachment 161721 [details]
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
(Reporter)

Comment 14

13 years ago
Committed.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.