Closed Bug 144216 Opened 18 years ago Closed 18 years ago

mozilla 1.0rc2 doesn't build with --enable-toolkit-qt

Categories

(Core Graveyard :: GFX, defect, P3)

x86
Linux
defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 178987

People

(Reporter: bero, Assigned: bero)

References

Details

Attachments

(1 file)

SSIA - Attaching a patch.
This makes it compile with Qt 2.3.x; it's still a no-go with Qt 3.x because
Mozilla hasn't been adapted to the new charset yet.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch
wow this is long

these are things i'd like changed if it weren't for the fact that you're fixing 
build bustage by cloning code.

[re MAKE_PR_BOOL] i'm pretty sure you can use PRBool() to cast things, but 
in general you should just be using if (cond) or if (!cond) instead of 
comparisons to PR_TRUE/PR_FALSE so it shouldn't matter. if you're going to use 
something, might i suggest !!expr instead of (expr)?PR_TRUE:PR_FALSE?

+// then it will never be delete, so this class takes care of that.
be delete_d_
+  /* Does this device allow to set/change the paper size ? */
allow _??_ to/g
+  /* Set number of orientation records and the records itself */
grm: records <-> itself. choose from 'record itself' or 'records themselves'
+  mPrefs->SetCharPref(nsPrintfCString(256, PRINTERFEATURES_PREF ".%s.%s", 
mPrinterName.get(), tagname).get(), value);
what happens if you have a 300 character long printer name?
-  NS_ASSERTION(aPS, "Must have a PrintSettings!");
+  DO_PR_DEBUG_LOG(("nsDeviceContextSpecQT::DisplayXPDialog()\n"));
grm: +  NS_ASSERTION(aPS, "Must have a print settings!");
PrintSettings was ok because it was a type, 'print settings' isn't because it 
isn't correct English.
+  /* printer names for the PostScript module alwas start with 
sp: alwas
+  if (aCount) 
+    *aCount = 0;
+  else 
+    return NS_ERROR_NULL_POINTER;
else before return (personal nit)/g
if you had written:
+  if (!aCount) 
+    return NS_ERROR_NULL_POINTER;
+  else 
+    *aCount = 0;
then an sr would have complained about else after return.
consider this instead:
+  if (!aCount) 
+    return NS_ERROR_NULL_POINTER;
+  *aCount = 0;

+  if (NS_FAILED(rv)) {
+    return rv;
+  }
note:i think people are leaning towards NS_ENSURE_TRUE(NS_SUCCEEDED(rv), rv);/g
+  nsresult rv = GlobalPrinters::GetInstance()->InitializeGlobalPrinters();
...
+  PRInt32 numPrinters = GlobalPrinters::GetInstance()->GetNumPrinters();
is there some important reason not to cache GetInstance()?/g

+        nsMemory::Free(array[i]);
+      nsMemory::Free(array);
warning: msvc6 free (and i have don't want to think about what nsMemory::Free 
actually does) will complain if you don't cast its param to void*.

+        XpuOrientationRec *curr = &olist[i];
+        printerFeatures.SetOrientationRecord(i, curr->orientation);
why not:
+        printerFeatures.SetOrientationRecord(i, 
((XpuOrientationRec)olist[i]).orientation);

your code uses both
+      }
+      else {
and
+      } else {
i prefer the latter or consistency.

+    return NS_OK;    
+  }
+  else
else after return. lose it :)

+  /* fixme: We simply ignore the |aPrinter| argument here
mozilla style includes prefixing things like this with XXX

+      mGlobalPrinterList->AppendString(
+        nsString(NS_ConvertASCIItoUCS2(NS_POSTSCRIPT_DRIVER_NAME)) + 
+        nsString(NS_ConvertASCIItoUCS2(name)));
this looks like bad ns*String foo, but it's too late for me to spend time 
checking.

+GlobalPrinters::GetDefaultPrinterName(PRUnichar **aDefaultPrinterName)
..
+    if (NS_FAILED(rv)) {
+      return;
+    }
..
+  if (GlobalPrinters::GetInstance()->GetNumPrinters() == 0)
+    return;
personal nit, be consistent about wrapping single block returns. presonally I 
prefer the latter (or consistency)

notations /g = global nit. grm: = grammar. sp: = spelling

anyways, i'm willing to check this in [to trunk] (if you want) if cls/blizzard 
take a moment to comment. for branch you'd need to send an additional email to 
drivers@mozilla.org explaining that this lets people build mozilla against qt.
Assignee: kmcclusk → bero
Blocks: qt
Nice... I wish we would get bug 130857 ("Create unified version of
nsDeviceContextSpec{GTK|Qt|Xlib}.{cpp|h} stuff in gfx/src/unixshared/")
implemented instead but this one is enougth to get the Qt port back to live...
:)

Thanks for the work...

----

timeless wrote:
> these are things i'd like changed if it weren't for the fact that you're 
> fixing build bustage by cloning code.

That's what makes the difference between nsDeviceContextGTK <-->
nsDeviceContextQt <--> nsDeviceContextXlib - always the same code but different 
class names.
That's why I filed bug 130857 to get a unified version of the code (for
GTK/Qt/Xlib/BeOS) to avoid that one platform get's patched without updating the
others, too...
Blocks: 130857
Unfortunately the patch isn't sufficient to make it actually work yet... It compiles (that's when I sent the patch - the rest of the build took some more hours), but then barfs on startup:  GFX: dpi=75 t2p=0.0526316 p2t=19 depth=16 JCG: eBorderStyle_close isn't handled yet... please fix me  
fwiw eBorderStyle_close isn't handled in gtk either.
my bet is we core or exit, use your favorite debugger, good luck.

note that getting it to build satisfies 'fixing build bustage' and would enable 
others (eg hwaara) to try to help out, so it's still worth looking into putting 
on trunk. (i probably wouldn't ask drivers to put it on branch)
Correction: It doesn't crash, it just hangs forever after displaying the 
eBorderStyle_close message. 
 
My first guess is it waits for user input without bothering to show the dialog 
it's waiting for first, probably it forgets to enter the Qt event loop. 
 
Last couple of lines from ltrace: 
 
_ZNK24nsASingleFragmentCString19GetReadableFragmentER18nsReadableFragmentIcE17nsFragmentRequestj(0xbfffe160, 
0xbfffe060, 1, 0, 0) = 0x4040e8e0 
_ZNK24nsASingleFragmentCString19GetReadableFragmentER18nsReadableFragmentIcE17nsFragmentRequestj(0xbfffe160, 
0xbfffe060, 3, 0, 0xbfffe160) = 0 
_ZNK24nsASingleFragmentCString19GetReadableFragmentER18nsReadableFragmentIcE17nsFragmentRequestj(0xbfffe150, 
0xbfffe060, 1, 0, 0) = 0x4040e8e5 
_ZNK24nsASingleFragmentCString19GetReadableFragmentER18nsReadableFragmentIcE17nsFragmentRequestj(0xbfffe150, 
0xbfffe060, 3, 0, 0) = 0 
--- 0 (Real-time signal 0) --- 
breakpointed at 0x42028828 (?) 
breakpointed at 0x40444f57 (?) 
 
_ZNK24nsASingleFragmentCString19GetReadableFragmentER18nsReadableFragmentIcE17nsFragmentRequestj 
== nsASingleFragmentCString::GetReadableFragment(nsReadableFragment<char>&, 
nsFragmentRequest, unsigned) const 
 
 
Don't have the time to debug this any further ATM, if nobody else picks it up 
I'll take a closer look later. 
 
much better, i don't have time not but that bug's filed (component:string, 
assignee: jag/scc) you should be able to find it.
The code in widget/src/qt/ may need an update, maybe the XP timer changes were
never ported to Qt.

Please get this bug fixed first (build bustage) and file a new bug for the new
issue, assign it to me and CC: timeless...
done. Bug 144428. 
Blocks: 144469
Comment on attachment 83375 [details] [diff] [review]
Fix build problems with --enable-toolkit-qt

r=Roland.Mainz@informatik.med.uni-giessen.de - let's get it "in" into trunk
first
Attachment #83375 - Flags: review+
Priority: -- → P3
*** Bug 130725 has been marked as a duplicate of this bug. ***
timeless, can you check this in? I don't want to think about your comment 2...

this is ports code, so rs=blizzard, and we have an r=
This patch fails for the current mozilla CVS as well as 1.1b. Bug 144469 also
contains this comment. This should have a re-look to fix up for the current CVS,
quite a large chunk fails.

*** This bug has been marked as a duplicate of 178987 ***
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → DUPLICATE
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.