Closed Bug 565013 Opened 14 years ago Closed 14 years ago

Save as PDF not functional for cairo-qt builds

Categories

(Core Graveyard :: Widget: Qt, defect)

ARM
Maemo
defect
Not set
normal

Tracking

(fennec2.0+)

RESOLVED FIXED
mozilla2.0b7
Tracking Status
fennec 2.0+ ---

People

(Reporter: wolfiR, Assigned: reportbase)

References

Details

(Whiteboard: 597676)

Attachments

(2 files, 11 obsolete files)

15.85 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
141.05 KB, patch
Details | Diff | Splinter Review
browser-ui.js
    let printSettings = Cc["@mozilla.org/gfx/printsettings-service;1"]
                           .getService(Ci.nsIPrintSettingsService)

This service is not available in cairo-qt builds but I would expect the used settings to be cross platform enough.
The bug would be Core I know but the feature in Fennec exposes the issue.
There also is bug 468495 which is talking about a complete printing support apparently.
"Saving as pdf" with the printing service is mostly a hack and don't work on windows if there is no printer installed, Mac OS, and format the page using the css specified for printing.

In my opinion, the feature should be in the platform outside the print service code.
Component: Linux/Maemo → General
OS: Linux → Linux (embedded)
Hardware: All → ARM
This patch provides the necessary classes for the Save to PDF option to work under cairo-qt. They were adopted from the respective gtk2 files.
If a webpage requests printing from js, a partially functional print dialog appears. This has yet to be implemented with proper Qt dialogs.
Attachment #456407 - Flags: review?
Attachment #456407 - Flags: review? → review?(doug.turner)
Comment on attachment 456407 [details] [diff] [review]
Patch to provide save to pdf functionality

Oleg should take a first pass at this.
Attachment #456407 - Flags: review?(romaxa)
Attachment #456407 - Flags: review?(doug.turner)
Comment on attachment 456407 [details] [diff] [review]
Patch to provide save to pdf functionality

>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998

Fix license here and in other files, 2010, and original code

>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Michael Ventnor <m.ventnor@gmail.com>

probably you want to add your name somewhere here...


>+    if (format == nsIPrintSettings::kOutputFormatPDF) {
>+        surface = new gfxPDFSurface(stream, surfaceSize);
>+    } else {
>+        return NS_ERROR_NOT_IMPLEMENTED;
>+    }
>+
>+    if (!surface)
>+        return NS_ERROR_OUT_OF_MEMORY;

IIRC we are not checking new operator result...

>+    surface.swap(*aSurface);
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP nsDeviceContextSpecQt::Init(nsIWidget* aWidget,
>+        nsIPrintSettings* aPS,

would be nice to have correct indentation here, and in other functions
should be lice this
+NS_IMETHODIMP
+nsDeviceContextSpecQt::Init(nsIWidget* aWidget,
+                            nsIPrintSettings* aPS,
+                            PRBool aIsPrintPreview)
+{


>+
>+NS_IMETHODIMP nsDeviceContextSpecQt::GetPath(const char** aPath)      
                                                                  ^^^^^ - don't introduce white-spaces here and in other places.... 

>+
>+NS_IMETHODIMP nsDeviceContextSpecQt::BeginDocument(PRUnichar * aTitle, PRUnichar * aPrintToFileName,
>+        PRInt32 aStartPage, PRInt32 aEndPage)
>+{
>+    if (mToPrinter) {
>+        return NS_ERROR_NOT_IMPLEMENTED;    
                                           ^^^^^

e.t.c.

>+
>+//return something so silent pdf saving is happy. maybe implement an actual pdf printer some time so printing from content js works

try to fit sources content into 80 chr width

>+
>+NS_IMETHODIMP nsPrinterEnumeratorQt::InitPrintSettingsFromPrinter(const PRUnichar* aPrinterName, nsIPrintSettings* aPrintSettings)

same here, break retval, func, params into column



>+    *_retval = nsnull;
>+    nsPrintSettingsQt* printSettings = new nsPrintSettingsQt(); // does not initially ref count

no new check here
>+    NS_ENSURE_TRUE(printSettings, NS_ERROR_OUT_OF_MEMORY);


>+    NS_ADDREF(*_retval = printSettings); // ref count
>+    return NS_OK;

r+ with comments fixed
Attachment #456407 - Flags: review?(romaxa) → review+
I updated this patch so the new files are actually hg copies of the gtk versions, so the license can be fixed in both.
Attachment #456407 - Attachment is obsolete: true
Attachment #457797 - Flags: review?
Attachment #456407 - Flags: review?(doug.turner)
Attachment #457797 - Flags: review? → review?(timeless)
Comment on attachment 457797 [details] [diff] [review]
files are now copies of the gtk2 version


>+    nsRefPtr<nsPrintSettingsQt> printSettingsQt(do_QueryInterface(aPS));

I think here should be:
      nsCOMPtr<nsPrintSettingsQt> printSettingsQt(do_QueryInterface(aPS));

>+    if (!printSettingsQt)
>+        return NS_ERROR_NO_INTERFACE;
>+    return NS_OK;
> }
Comment on attachment 457797 [details] [diff] [review]
files are now copies of the gtk2 version


+#define NS_PRINTSETTINGSQt_IID \
+{0x758df520, 0xc7c3, 0x11dc, {0x95, 0xff, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x66}}
 
-#define NS_PRINTSETTINGSGTK_IID \
-{ 0x758df520, 0xc7c3, 0x11dc, { 0x95, 0xff, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x66 } 

NS_PRINTSETTINGSQt_IID should be in all caps
please use uuidgen to generate a replacement iid for this fake interface, since yours isn't necessarily compatible w/ the gtk one.

sorry for misleading you, the problem is that the code cheats.

it should have an nsIPrintSettingsGtk and an nsIPrintSettingsQt from which nsPrintSettingsGtk and nsPrintSettingsQt inherit respectively.

anyway, for now just use nsCOMPtr<> -- my fault, eventually someone should really fix the code (and also get rid of the evil _Assign).

-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

if you're switching to 4 spaces, please add:

+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

i suspect you shouldn't call this until right before return NS_OK;
+    file.setAutoRemove(false);
+
+    rv = NS_NewNativeLocalFile(
+            nsDependentCString(file.fileName().toAscii().constData()),
+            PR_FALSE,
+            getter_AddRefs(mSpoolFile));
+    if (NS_FAILED(rv)) {
+        file.remove();
+        return NS_ERROR_GFX_PRINTER_COULD_NOT_OPEN_FILE;
+    }
+
..
+    if (!surface)
+        return NS_ERROR_OUT_OF_MEMORY;
+    surface.swap(*aSurface);
+    return NS_OK;

set the next review to roc or someone who officially reviews qt
Attachment #457797 - Flags: review?(timeless) → review-
Also it would be nice to fix this warning:

nsPrintOptionsQt.cpp:38:
widget/src/xpwidgets/nsPrintSettingsImpl.h:65: warning: 'virtual nsPrintSettings& nsPrintSettings::operator=(const nsPrintSettings&)' was hidden
widget/src/qt/nsPrintSettingsQt.h:110: warning:   by 'nsPrintSettingsQt& nsPrintSettingsQt::operator=(const nsPrintSettingsQt&)'
Attached patch Updated Patch Version (obsolete) — Splinter Review
Attachment #460677 - Flags: review?(mark.finkle)
Attached patch Updated Patch Version (obsolete) — Splinter Review
Sorry uploaded the same version as before. Now the right one. @Oleg this is the same as in GTK.
Attachment #460677 - Attachment is obsolete: true
Attachment #460679 - Flags: review?(mark.finkle)
Attachment #460677 - Flags: review?(mark.finkle)
Attached patch Updated Version 2 (obsolete) — Splinter Review
Sorry found a minor issue with that change i did. Fixed it
Attachment #460679 - Attachment is obsolete: true
Attachment #460680 - Flags: review?(mark.finkle)
Attachment #460679 - Flags: review?(mark.finkle)
Attachment #460680 - Flags: review?(mark.finkle) → review?(doug.turner)
tracking-fennec: --- → ?
Attachment #460680 - Flags: review?(doug.turner) → review?(dholbert)
I won't have time to review this in the next couple of days, and I'll be traveling to France for SVG Open next week, but I'll try to look at it on the plane.
tracking-fennec: ? → 2.0b2+
Comment on attachment 460680 [details] [diff] [review]
Updated Version 2

adding oleg for review.
Attachment #460680 - Flags: review?(romaxa)
Blocks: 584225
Comment on attachment 460680 [details] [diff] [review]
Updated Version 2

Sorry for the delay on this.  I'm about to get on a plane, on which I'll finish this review, but here's what I've got so far.  Looks really good, overall! My feedback is mostly nits.

diff --git a/configure.in b/configure.in
>     NS_PRINTING=1)
> 
>-if test "$MOZ_WIDGET_TOOLKIT" = "qt"; then
>-    AC_MSG_WARN([Printing does not work with Qt at this time. Omitting printing support.])
>-    NS_PRINTING=
>-fi
> 
> if test "$NS_PRINTING"; then
>     AC_DEFINE(NS_PRINTING)

Delete one of the blank lines flanking your deleted chunk here. (no need to leave 2 blank lines between logically-connected NS_PRINTING blocks.)

> copy to widget/src/qt/nsDeviceContextSpecQt.h
> -/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Change "c-basic-offset" to 4 in nsDeviceContextSpecQt.h and nsPrintOptionsQt.h, to correctly reflect the indentation-level you've used in these files.

Looks like nsPrintDialogQt.cpp, nsPrintDialogQt.h, nsPrintOptionsQt.cpp, and nsPrintSettingsQt.cpp lack this modeline altogether -- could you add the same modeline there, too? (with c-basic-offset: 4, assuming you use 4-space indentation in those files)

Also: nsPrintSettingsQt.h's modeline has 2 broken things:
 - "Mode: IDL" should be "Mode: C++"  (It's just a normal .h file)
 - c-basic-offset should be 4

I think that might broken in the GTK version, too -- we should fix that there at some point -- but for now, I'd just prefer that we not duplicate that brokenness. :)

>+nsresult nsPrintSettingsQt::_Clone(nsIPrintSettings** _retval)
[...]
>+    NS_ENSURE_ARG_POINTER(_retval);
>+    *_retval = nsnull;
>+
>+    nsPrintSettingsQt* newSettings = new nsPrintSettingsQt(*this);
>+    *_retval = newSettings;

The line
  *_retval = nsnull;
doesn't do anything useful there - remove that.

>+NS_IMETHODIMP
>+nsPrintSettingsQt::_Assign(nsIPrintSettings* aPS)
[...]
>+    return NS_OK;
>+
> }

Remove the newline before the closing brace here.

Also:

This file has a mixture of return-type-and-signature-on-same-line (e.g. _Assign) vs. return-type-and-signature-on-different-lines. (e.g. _Clone)

Could you standardize to the different-lines version (matching _Assign), for consistency & to atch the more general mozilla style?
Comment on attachment 460680 [details] [diff] [review]
Updated Version 2

Further more notes on nsPrintSettingsQt:
=======================================
>+static const char* const PaperSizeToPaperName[QPrinter::NPageSize] =
>+{ "A4", "B5", "Letter", "Legal", "Executive",

A few notes here:
Firstly, variable names ("PaperSizeToPaperName") shouldn't be capitalized.

Secondly, this seems like it'd be better-named something like "indexToPaperName", and the array below would be something like "indexToQtPaperEnum".  Both arrays map the same sort of index to an indicator of page size -- the only difference is that the first array maps to string names, and the second one maps to Qt's enumerated values. So the arrays' names should mostly be the same, aside from specifying the thing that they map to.

Thirdly, what happens if some version of Qt has a larger value for NPageSize than the number of entries we have here? (e.g. what if they add more page types in the future)?  This would still compile, I think, but if we somehow end up using one of the "new" paper names, we'll ultimately reference an uninitialized entry in this array, in calls to GetPaperName. That'd be bad.

To address that issue, I think we should declare this array without an explicit size, and then include a PR_STATIC_ASSERT somewhere (maybe in GetPaperName) to check that its size matches NPageSize.

>+static const QPrinter::PageSize indexToPageSize[QPrinter::NPageSize] =
>+{

The above points apply here, too -- rename to "indexToQtPaperEnum" or something like it, and don't specify the array-size up-front, and add a PR_STATIC_ASSERT somewhere to verify the array-size is what we expect.

>+NS_IMETHODIMP
>+nsPrintSettingsQt::GetPaperName(PRUnichar** aPaperName)
>+{
>+    QPrinter::PaperSize size = mQPrinter->paperSize();
>+    QString name(PaperSizeToPaperName[size]);

Is it possible that the variable |size| could be out-of-bounds for our array?  e.g. could paperSize() return some weird error code if mQPrinter is in a bad state?

If it's at all possible for |size| to be out of bounds, you should add a range-check-and-return.

Or, if that absolutely can't happen, you should add a NS_ABORT_IF_FALSE() to check the range in debug builds, so that your assumption about the range is clear.

>+    *aPaperName = ToNewUnicode(nsDependentString((const PRUnichar*)name.constData()));
This line is too long -- maybe add a newline before nsDependentString, to match your line-wrapping in GetToFileName.

>+nsPrintSettingsQt::SetPaperName(const PRUnichar* aPaperName)
>+{
>+    QString ref((QChar*)aPaperName, NS_strlen(aPaperName));
>+    for(unsigned int i = 0;i<QPrinter::NPageSize; i++)

s/unsigned int/PRUint32/
Also: add a space after "for", after "i = 0;", and on either side of "<".

>+        if(ref == QString(PaperSizeToPaperName[i])){

Add a space after "if".

>+QPrinter::Unit GetQtUnit(PRInt16 aGeckoUnit)
>+{
>+    if (aGeckoUnit == /*kPaperSizeMillimeters*/ 1) {

Why not just use kPaperSizeMillimeters directly here?  Magic numbers (even "1") are bad.

Also, this method (GetQtUnit) should just be folded into SetPaperSizeUnit.  You only call this method for converting mPaperSizeUnit, and you call it whenever you read mPaperSizeUnit, so it'd be better to only convert once, inside of SetPaperSizeUnit.

>+NS_IMETHODIMP
>+nsPrintSettingsQt::SetUnwriteableMarginInTwips(nsIntMargin& aUnwriteableMargin)
>+{
>+    nsPrintSettings::SetUnwriteableMarginInTwips(aUnwriteableMargin);
>+    mQPrinter->setPageMargins(
>+            NS_TWIPS_TO_INCHES(mUnwriteableMargin.left),
>+            NS_TWIPS_TO_INCHES(mUnwriteableMargin.top),
>+            NS_TWIPS_TO_INCHES(mUnwriteableMargin.right),
>+            NS_TWIPS_TO_INCHES(mUnwriteableMargin.bottom),
>+            QPrinter::Inch);
>+    return NS_OK;

Hmm, it looks like that six-line "mQPrinter->setPageMargins(" call is duplicated in all five SetUnwriteableMargin* function-bodies here.  Could you split that out into a private no-argument helper method?  Something like "UnwriteableMarginUpdated()", perhaps.

>+    return NS_OK;
>+}
>+NS_IMETHODIMP
>+nsPrintSettingsQt::SetPaperHeight(double aPaperHeight)

Add a newline before NS_IMETHODIMP there.

>+NS_IMETHODIMP
>+nsPrintSettingsQt::SetupSilentPrinting()
>+{
>+    return NS_OK;
>+}

You can get rid of this chunk.  You already inherit a do-nothing stub from nsPrintSettings, so there's no need to define a do-nothing stub here.
Comment on attachment 460680 [details] [diff] [review]
Updated Version 2

Notes on nsDeviceSpecQt.*, nsPrintDialogQt.*, and nsPrintOptionsGTK.*:
======================================================================
>diff --git a/widget/src/gtk2/nsDeviceContextSpecG.h b/widget/src/qt/nsDeviceContextSpecQt.cpp
>+#ifdef USE_POSTSCRIPT
> #include "nsPSPrinters.h"
> #include "nsPaperPS.h"  /* Paper size list */
>+#endif /* USE_POSTSCRIPT */

I don't think this chunk does anything -- it doesn't look like USE_POSTSCRIPT is mentioned at all anywhere else in the tree or in your patch, aside from this one spot.

>+NS_IMETHODIMP nsDeviceContextSpecQt::GetSurfaceForPrinter(
>+        gfxASurface** aSurface)
[...]
>+    const char* path;
>+    GetPath(&path);
[...]
>+    DO_PR_DEBUG_LOG(("\"%s\", %f, %f\n", path, width, height));

There's no need for the |path| variable -- it's only used for a logging line, and we can just directly print |mPath| rather than copying it to |path| and printing that.

>+    nsresult rv;
[...10 lines later...]
>+    rv = NS_NewNativeLocalFile(

Just declare rv inline with its assignment, i.e.
  nsresult rv = NS_NewNativeLocalFile(...

>+    if (!surface)
>+        return NS_ERROR_OUT_OF_MEMORY;

I don't think we can actually reach that line with a null |surface| value.  But if we can, NS_ERROR_OUT_OF_MEMORY wouldn't be the right thing to return, since we've got infallible-new().

If it's actually possible for |surface| to be null, choose a different return type (ERROR_FAILURE maybe). If not, replace this chunk with a NS_ABORT_IF_FALSE() statement (to be foolproof against a future where this method gets more complicated and handles more output types).

>+NS_IMETHODIMP nsDeviceContextSpecQt::BeginDocument(
>+        PRUnichar * aTitle,
>+        PRUnichar * aPrintToFileName,

Delete the space before the "*" characters there, for consistency with the style you're using elsewhere.

>+// return empty list, so silent save to pdf is happy
>+NS_IMETHODIMP nsPrinterEnumeratorQt::GetPrinterNameList(

I don't understand this comment - it looks like we're returning a non-empty list to me.

>+    QList<QPrinterInfo> qprinters = QPrinterInfo::availablePrinters();
>+    nsTArray<nsString>* printers =
>+        new nsTArray<nsString>(qprinters.size() + 1);

Why +1 there? (Assuming we need it, add a comment to explain why.)

d>+    for(int i = 0; i< qprinters.size(); ++i){

s/int/PRInt32/ and add a space after "for" and before "<"

>+//return something so silent pdf saving is happy. maybe implement an
>+//actual pdf printer some time so printing from content js works

Again, I don't understand this. RE "return something" - does this method not do the right thing?

>+    *aDefaultPrinterName = ToNewUnicode(nsDependentString((const PRUnichar*)defprinter.constData()));
>+    DO_PR_DEBUG_LOG(("GetDefaultPrinterName(): default printer='%s'.\n", NS_ConvertUTF16toUTF8(*aDefaultPrinterName).get()));

These lines are too long.

>+NS_IMETHODIMP nsPrinterEnumeratorQt::InitPrintSettingsFromPrinter(
[...]
>+    DO_PR_DEBUG_LOG(("nsPrinterEnumeratorQt::InitPrintSettingsFromPrinter()"));
>+    return NS_OK;
> }

This method does nothing right now (as compared to the version in nsPrinterEnumeratorG, which does a ton of stuff). Perhaps this should return NS_ERROR_NOT_IMPLEMENTED?  (Also, does it break anything for this to be unimplemented?)

>diff --git a/widget/src/gtk2/nsDeviceContextSpecG.h b/widget/src/qt/nsDeviceContextSpecQt.h
> #define NS_PORTRAIT  0
> #define NS_LANDSCAPE 1

I don't think you need these.

>+        NS_IMETHOD GetSurfaceForPrinter(gfxASurface** surface);

This line is indented too much.

>+    NS_IMETHOD Init(
>+            nsIWidget* aWidget,
>+            nsIPrintSettings* aPS,
>+            PRBool aIsPrintPreview);
>+    NS_IMETHOD BeginDocument(
>+            PRUnichar* aTitle,

The first param should be up on the same line as the method-name for both these methods.

>+    virtual ~nsDeviceContextSpecQt();

Right now, you declare the destructor after everything else -- that doesn't match the ordering in the .cpp file, and it's a bit odd stylistically.  Could you move that declaration up higher, to be right after the constructor?

>diff --git a/widget/src/gtk2/nsPrintDialogGTK.h b/widget/src/qt/nsPrintDialogQt.cpp

It looks like this file just has stub "return NS_ERROR_NOT_IMPLEMENTED" method-bodies for now.  Can you add a comment towards the top, to explain that?  Something like:
  // For Qt, we only support printing to PDF, and that doesn't need a
  // print dialog at this point.  So, this class's methods are left
  // un-implemented for now.

> #include "nsIPrintSettings.h"
> #include "nsIWidget.h"

These two #includes can be removed.  (Note that even if this class had non-stub method implementations, it still wouldn't need nsIPrintSettings.h, because it already gets that via nsPrintSettingsQt.h)

>+nsPrintDialogServiceQt::ShowPageSetup(nsIDOMWindow* aParent,
>+        nsIPrintSettings* aNSSettings)

Fix indentation on the second line there.

>diff --git a/widget/src/gtk2/nsPrintDialogGTK.h b/widget/src/qt/nsPrintDialogQt.h
[...]
>+class nsPrintDialogServiceQt : public nsIPrintDialogService
> {
[...]
>+        NS_IMETHODIMP Init();
>+    NS_IMETHODIMP Show(nsIDOMWindow* aParent, nsIPrintSettings* aSettings,
>+            nsIWebBrowserPrint* aWebBrowserPrint);
>+    NS_IMETHODIMP ShowPageSetup(nsIDOMWindow* aParent,
>+            nsIPrintSettings* aSettings);
> };

Fix indentation here. (Init is indented too far, and Show & ShowPageSetup need more indentation on their second line.)

>diff --git a/widget/src/gtk2/nsPrintOptionsGTK.cpp b/widget/src/qt/nsPrintOptionsQt.cpp
>  * Contributor(s):
>+ *          Florian Hänel <heeen@gmx.de>

Why the huge indentation for your name there? :)  Just increase indent by 2 spaces for that. (to match the other files.)

>+nsresult nsPrintOptionsQt::_CreatePrintSettings(nsIPrintSettings** _retval)
>+{
>+    *_retval = nsnull;
>+    nsPrintSettingsQt* printSettings = 
>+        new nsPrintSettingsQt(); // does not initially ref count
>+    NS_ADDREF(*_retval = printSettings); // ref count

Remove the first line of this method (setting *_retval to null).

Also, this file has some extra blank lines at the end -- get rid of those, please.

>diff --git a/widget/src/gtk2/nsPrintOptionsGTK.h b/widget/src/qt/nsPrintOptionsQt.h
>+#define nsPrintOptionsQt_h__
[...]
> };
> 
> 
> 
> #endif /* nsPrintOptions_h__ */

s/nsPrintOptions/nsPrintOptionsQt/ in the #endif comment.

Also, delete the extra newlines before #endif. (There should just be one blank line there.)

=============
General notes:
 - I don't know the Qt API at all, so I'm trusting you & romaxa to catch anything broken there. :)
 - I'm marking this r- just because there's a lot of stuff to fix in these last 3 comments.  But on the bright side, it's all relatively minor stuff - this is almost there, I think! :)
Attachment #460680 - Flags: review?(dholbert) → review-
Comment on attachment 460680 [details] [diff] [review]
Updated Version 2

Also this patch need to be updated to latest m-c, because there are some change happen in http://hg.mozilla.org/mozilla-central/log/e523182f9639/widget/src/gtk2/nsPrintSettingsGTK.cpp

I think Tom will try to address all comments, and attach new patch
Attachment #460680 - Flags: review?(romaxa)
Here's the same patch with bitrot fixed, so it now appplies to current trunk. (It was just a s/INCHES_TO_TWIPS/INCHES_TO_INT_TWIPS/ in the patch file.)
Assignee: nobody → reportbase
Jeremias on vacation, and Tom will take care about review comment fixes and attaching new version of patch
Attached patch Fix review comments (obsolete) — Splinter Review
Fix review comments by "Daniel Holbert".
Attachment #475926 - Flags: review?(dholbert)
Comment on attachment 475926 [details] [diff] [review]
Fix review comments

Hi Tom -- thanks for fixing this stuff! Comments below...  Also, it looks like this just addresses Comment 17 -- see also Comment 15 & Comment 16 for more stuff that needs fixing. :)

>@@ -126,27 +119,25 @@ NS_IMETHODIMP nsDeviceContextSpecQt::Get
[...]
>-    DO_PR_DEBUG_LOG(("\"%s\", %f, %f\n", path, width, height));
>-    nsresult rv;
>-
>+    DO_PR_DEBUG_LOG(("%f, %f\n", width, height));

In Comment 17, I was actually suggesting removing the |path| variable entirely, and using mPath *instead* in this logging line.  Can you change this chunk to do that?  (#1, remove the declaration of "path", which I don't think you've done yet -- #2, add "mPath" to the logging line, where "path" was)

>@@ -177,18 +168,16 @@ NS_IMETHODIMP nsDeviceContextSpecQt::Get
>-    if (!surface)
>-        return NS_ERROR_OUT_OF_MEMORY;

If we're not checking this anymore, then this needs a NS_ABORT_IF_FALSE to assert that surface is non-null, as I'd mentioned in Comment 17.

>-// return empty list, so silent save to pdf is happy
>+// Return printer name list  
> NS_IMETHODIMP nsPrinterEnumeratorQt::GetPrinterNameList(

I'd just remove that comment entirely -- "Return printer name list" isn't really a useful header-comment for a method called GetPrinterNameList. :)

(Also, watch out for end-of-line whitespace -- you have a few blank spaces at the end of this comment)

>+    //+1 is added so that in the case where there is no printer drivers,
>+    //the write to pdf functionality will work.  This could probably be
>+    //handeled better.  
>     nsTArray<nsString>* printers =
>-        new nsTArray<nsString>(qprinters.size() + 1);
>+        new nsTArray<nsString>(qprinters.size() + 1); 

A few things:
 - You added end-of-line whitespace after the "+ 1); " -- don't do that. :)
 - Need a space between "//" and the comment
 - s/handeled/handled/

So, on to the content of the comment: so, is the problem here that the caller of this method requires a nonempty list, in order to go ahead with printing (even to PDF)?  I think we end up leaving that last "+1" entry as an empty string -- does that cause any weirdness?  Perhaps we should initialize that string to something (e.g. "FakePdfDriver").  I'm not sure where that'd show up, but it might be better than an empty string.

> >+    for(int i = 0; i< qprinters.size(); ++i){
> s/int/PRInt32/ and add a space after "for" and before "<"

This review-comment from comment 17 still needs to be addressed.  We also need a space between ")" and "{".

>-//return something so silent pdf saving is happy. maybe implement an
>-//actual pdf printer some time so printing from content js works
>+// Return default printer name
> NS_IMETHODIMP nsPrinterEnumeratorQt::GetDefaultPrinterName(

As above with "GetPrinterNameList", I'd prefer no comment at all, rather than a comment that exactly matches what the method name already says. :)

>-    DO_PR_DEBUG_LOG(("GetDefaultPrinterName(): default printer='%s'.\n", NS_ConvertUTF16toUTF8(*aDefaultPrinterName).get()));
>+    DO_PR_DEBUG_LOG(("GetDefaultPrinterName(): default printer='%s'.\n", 
Remove end-line-space-character after |\n",|

>+		NS_ConvertUTF16toUTF8(*aDefaultPrinterName).get()));
>+		

Don't use tab characters for indentation! Spaces only, please.
Also, remove whitespace-on-blank-line on the second line there.

>+	//Always return NS_OK.  Save to pdf does not require a default printer.
>     return NS_OK;

Add a space after "//", and replace the tab with spaces.

> NS_IMETHODIMP nsPrinterEnumeratorQt::InitPrintSettingsFromPrinter(
>+    //Leave NS_OK for now
>+    //Probably should use NS_ERROR_NOT_IMPLEMENTED
>     return NS_OK;

Add a space after "//", and flag this with "XXX" since it's something that's TODO / to-be-investigated.

> NS_IMETHODIMP nsPrinterEnumeratorQt::DisplayPropertiesDlg(
>         const PRUnichar* aPrinter,
>         nsIPrintSettings* aPrintSettings)
> {
>     return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
>+
>+

Looks like you added two blank lines there -- remove those.

>diff --git a/widget/src/qt/nsDeviceContextSpecQt.h b/widget/src/qt/nsDeviceContextSpecQt.h
>-    NS_IMETHOD Init(
>-            nsIWidget* aWidget,
>+    NS_IMETHOD Init(nsIWidget* aWidget,
>             nsIPrintSettings* aPS,
>             PRBool aIsPrintPreview);

Increase indentation on the last two lines above, to make those arguments line up with the first one.

>-    NS_IMETHOD BeginDocument(
>-            PRUnichar* aTitle,
>+    NS_IMETHOD BeginDocument(PRUnichar* aTitle,
>             PRUnichar* aPrintToFileName,
>             PRInt32 aStartPage,
>             PRInt32 aEndPage);

Same here.

> nsPrintDialogServiceQt::ShowPageSetup(nsIDOMWindow* aParent,
>-        nsIPrintSettings* aNSSettings)
>+							nsIPrintSettings* aNSSettings)

s/tabs/spaces/

>diff --git a/widget/src/qt/nsPrintDialogQt.h b/widget/src/qt/nsPrintDialogQt.h
>-        NS_IMETHODIMP Init();
>-    NS_IMETHODIMP Show(nsIDOMWindow* aParent, nsIPrintSettings* aSettings,
>+    NS_IMETHODIMP Init();
>+    NS_IMETHODIMP Show(nsIDOMWindow* aParent, 
>+			nsIPrintSettings* aSettings,
>             nsIWebBrowserPrint* aWebBrowserPrint);

Tabs again -- fix those. Also, "nsIWebBrowserPrint* aWebBrowserPrint" isn't indented correctly right now.

>     NS_IMETHODIMP ShowPageSetup(nsIDOMWindow* aParent,
>             nsIPrintSettings* aSettings);

Indentation of "nsIPrintSettings* aSettings" is still incorrect here, as I mentioned in Comment 17.

>diff --git a/widget/src/qt/nsPrintSettingsQt.cpp b/widget/src/qt/nsPrintSettingsQt.cpp
> }
>-
>+ 
> NS_IMETHODIMP
> nsPrintSettingsQt::SetPaperName(const PRUnichar* aPaperName)

Looks like you accidentally added whitespace to a blank line there -- revert that.
Attachment #475926 - Flags: review?(dholbert) → review-
(In reply to comment #22)
> >+    //+1 is added so that in the case where there is no printer drivers,
> >+    //the write to pdf functionality will work.  This could probably be
> >+    //handeled better.  
> >     nsTArray<nsString>* printers =
> >-        new nsTArray<nsString>(qprinters.size() + 1);
> >+        new nsTArray<nsString>(qprinters.size() + 1); 

Was good to speak with you earlier today -- so it sounds like the "+ 1" in this code might not be required or useful after all -- hopefully we'll know more once you've got a test environment set up & can see what (if anything) breaks without the + 1. :)

(FWIW, I just double-checked the GTK version of this function, and there's no such "+ 1" there:
> 709 NS_IMETHODIMP nsPrinterEnumeratorGTK::GetPrinterNameList(nsIStringEnumerator **aPrinterNameList)
[...]
> 719   PRInt32 numPrinters = GlobalPrinters::GetInstance()->GetNumPrinters();
> 720   nsTArray<nsString> *printers = new nsTArray<nsString>(numPrinters);
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsDeviceContextSpecG.cpp#719
Adding myself to cc.
Fixes review comments from 22/23.  Removed +1 discussed in comment 23.  

Note: The QT version still creates an empty pdf file.  This will be fixed in the next patch.
Attachment #475926 - Attachment is obsolete: true
Attachment #477624 - Flags: review?(dholbert)
Comment on attachment 477624 [details] [diff] [review]
Main Patch: Fixed review comments from 22/23

>diff --git a/widget/src/gtk2/nsDeviceContextSpecG.cpp b/widget/src/qt/nsDeviceContextSpecQt.cpp
>+#include "gfxPDFSurface.h"
>+ 
  ^ looks like you added whitespace-on-a-blank-line there

>+    for(PRInt32 i = 0; i < qprinters.size(); ++i){
         ^                                         ^
Still needs spaces as indicated above -- before and after the (...)

>diff --git a/widget/src/gtk2/nsPrintDialogGTK.h b/widget/src/qt/nsPrintDialogQt.h
>+    NS_IMETHODIMP ShowPageSetup(nsIDOMWindow* aParent,
>+                       nsIPrintSettings* aSettings);

Still needs indentation before "nsIPrintSettings* aSettings", as noted in Comment 17 & 22.

Aside from that, I think this finished addressing Comment 17/22 - yay! :)  I'm not marking r+, though, since there are still other things to address that I brought up in comment 15 & comment 16. (as noted at beginning of comment 22)
Attachment #477624 - Flags: review?(dholbert)
Whiteboard: 476521
Whiteboard: 476521 → 597676
PDF printing now works.  This path fixes comments from 15,16,26.  See comments below.

>> Thirdly, what happens if some version of Qt has a larger value for NPageSize
>> than the number of entries we have here? (e.g. what if they add more page types
>> in the future)?  This would still compile, I think, but if we somehow end up
>> using one of the "new" paper names, we'll ultimately reference an uninitialized
>> entry in this array, in calls to GetPaperName. That'd be bad.

>> To address that issue, I think we should declare this array without an explicit
>> size, and then include a PR_STATIC_ASSERT somewhere (maybe in GetPaperName) to
>> check that its size matches NPageSize.

The reason is that the heap is a scarce resource.   String sizes 
known at compile time should use static or stack memory.  Using the global heap
for memory allocations of things we know the size of at compile 
time is inefficent.   

Malloc is the number one cause of performance bottlenecks.  Applications should use malloc as
sparingly as possible.  


>>Is it possible that the variable |size| could be out-of-bounds for our array? 
>>e.g. could paperSize() return some weird error code if mQPrinter is in a bad
>>state? 

I looked at the enums.  Papersize will always be less than NPageSize. 
No assert is necessary.   The compiler will catch out-of-bound issues.

>> Also, this method (GetQtUnit) should just be folded into SetPaperSizeUnit.  You
>> only call this method for converting mPaperSizeUnit, and you call it whenever
>> you read mPaperSizeUnit, so it'd be better to only convert once, inside of
>> SetPaperSizeUnit

Not changed for now.  The compiler will easily optimize this 
extra function call to oblivian.

>> Hmm, it looks like that six-line "mQPrinter->setPageMargins(call is
>> duplicated in all five SetUnwriteableMargin* function-bodies here.  Could you.

The compiler complains when I add this helper member function. I'll create a 
simple macro instead.
Attachment #477944 - Flags: review?(dholbert)
Attachment #477944 - Attachment is patch: true
Attachment #477944 - Attachment mime type: application/octet-stream → text/plain
this isn't blocking fennec 2.0.  However, we'd love to see this patch land soon!
tracking-fennec: 2.0b2+ → 2.0-
tracking-fennec: 2.0- → 2.0+
after discussing a bit more... 2.0+
(In reply to comment #27)
> >> Thirdly, what happens if some version of Qt has a larger value for NPageSize
> >> than the number of entries we have here? (e.g. what if they add more page types
> >> in the future)?  This would still compile, I think, but if we somehow end up
> >> using one of the "new" paper names, we'll ultimately reference an uninitialized
> >> entry in this array, in calls to GetPaperName. That'd be bad.
> 
> >> To address that issue, I think we should declare this array without an explicit
> >> size, and then include a PR_STATIC_ASSERT somewhere (maybe in GetPaperName) to
> >> check that its size matches NPageSize.
> 
> The reason is that the heap is a scarce resource.   String sizes 
> known at compile time should use static or stack memory.  Using the global heap
> for memory allocations of things we know the size of at compile 
> time is inefficent.   
> 
> Malloc is the number one cause of performance bottlenecks.  Applications should
> use malloc as
> sparingly as possible.  

I wasn't suggesting using malloc -- I was suggesting that we simply remove the text "QPrinter::NPageSize" from this line:

>+static const char* const indexToPaperName[QPrinter::NPageSize] =
> { "A4", "B5", "Letter", "Legal", "Executive",
[...]
> };

Unless I'm mistaken, the compiler can still calculate the size of the array just fine (and no malloc's are needed), since we're specifying the contents up-front -- right?

More importantly, we can then make sure we've got the right number of strings in that array, using a (compile-time) PR_STATIC_ASSERT to check sizeof() the array vs. NPageSize. This would catch bugs (preventing out-of-bounds / uninitialized array accesses) if we accidentally forgot an entry here, or removed an entry in the array, and it'd also catch bugs from future Qt releases increasing the value of NPageSize if they ever add more paper-types.

> I looked at the enums.  Papersize will always be less than NPageSize. 
> No assert is necessary.

Ok.  Yeah -- since it's an enum, I guess that chunk should be fine.

>> Also, this method (GetQtUnit) should just be folded into SetPaperSizeUnit.  You
>> only call this method for converting mPaperSizeUnit, and you call it whenever
>> you read mPaperSizeUnit, so it'd be better to only convert once, inside of
>> SetPaperSizeUnit
>
> Not changed for now.  The compiler will easily optimize this 
> extra function call to oblivian.

I'm not so much worried about performance impact as just unnecessary complexity in new code (& resulting reduced readability).

Right now the patch uses a helper-function to convert a variable *every single time the variable is used*.  There's absolutely no reason for that, when you can just convert it up-front.  Why not just simplify it?

> >> Hmm, it looks like that six-line "mQPrinter->setPageMargins(call is
> >> duplicated in all five SetUnwriteableMargin* function-bodies here.  Could you.
> 
> The compiler complains when I add this helper member function. I'll create a 
> simple macro instead.

Ok.  I'd still prefer a member function (I'm not a big fan of macros), but not enough to mandate it, in a component where I'm not an owner/peer. :)
Nit: You added a modeline to the top of nsPrintDialogQt.* and nsPrintOptions.cpp (as I'd asked -- thanks!), but those added modelines all have:
> Mode: IDL;

nsPrintOptionsQt.h has this problem, too.

That should be "Mode: C++" in all those cases.  This patch has no IDL files, so there shouldn't be any "Mode: IDL" anywhere.
Comment on attachment 477944 [details] [diff] [review]
Addresses issues from comment 26.

r=dholbert on this patch & the one before it, with comment 30 & 31 addressed.

Thanks for all the great work here!
Attachment #477944 - Flags: review?(dholbert) → review+
Attached patch Address comments 30/31 (obsolete) — Splinter Review
Address comment 30 and comment 31

>> I'm not so much worried about performance impact as just unnecessary complexity
>> in new code (& resulting reduced readability).

Yeah.  I see what you mean.  However, the compiler complains about a bad conversion if
you dont do use helper function.   I bet he tried to pull it out, because of all 
the compiler errors and ugly casts, and then just deceided to use the helper function instead.
I couldn't come up with a cleaner way fix this with out making it even uglier than it already is.
Attachment #477944 - Attachment is obsolete: true
Attachment #478313 - Flags: review?(dholbert)
Addresses issues form comment 26, comment 30 and comment 31

>> I wasn't suggesting using malloc -- I was suggesting that we simply remove the
>> text "QPrinter::NPageSize" from this line:

Sorry. I Misunderstood.  Done.

>> I'm not so much worried about performance impact as just unnecessary complexity
>> in new code (& resulting reduced readability).

Yeah.  I see what you mean.  However, the compiler complains about a bad conversion if
you dont do use helper function.   I bet he tried to pull it out, because of all 
the compiler errors and ugly casts, and just decieded to use the helper function instead.  
I couldn't come up with a cleaner way fix this with out making it even uglier than it already is.
Attachment #478313 - Attachment is obsolete: true
Attachment #478316 - Flags: review?(dholbert)
Attachment #478313 - Flags: review?(dholbert)
(In reply to comment #34)
> >> I wasn't suggesting using malloc -- I was suggesting that we simply remove the
> >> text "QPrinter::NPageSize" from this line:
> 
> Sorry. I Misunderstood.  Done.

I still don't see a PR_STATIC_ASSERT anywhere.  Can you add that, to verify that both indexTo*** arrays have sizes equal to NPageSize? (PR_STATIC_ASSERT can catch bugs *at compile time*, which is awesome)

See this for sample usage:
http://mxr.mozilla.org/mozilla-central/search?string=PR_STATIC_ASSERT

Also, the NS_ASSERTION that you added...
>   NS_ASSERTION(size < sizeof(indexToPaperName)/sizeof(QPrinter::PageSize),"Out of bounds");
...is incorrect.  indexToPaperName is an array of char*'s, not an array of PageSize's, so I think you should be dividing by sizeof(char*), not sizeof(QPrinter::PageSize) there.

But in any case, you should just get rid of that assertion entirely and just use a (compile-time) PR_STATIC_ASSERT to check the array sizes against NPageSize.  (As you said in comment 27, |size| is an enum and can be assumed to be in-bounds, so we don't need to assertion-check its range as long as we've verified that the array has the right size.)

> Yeah.  I see what you mean.  However, the compiler complains about a bad
> conversion if
> you dont do use helper function.

Perhaps that's because you forgot to change the type of mPaperSizeUnit to be "QPrinter::Unit"?  That would cause type-conversion errors.  There shouldn't be anything inherent to this change that would cause compiler errors/warnings, though.

You should be fine if you do this:
 - Make mPaperSizeUnit a QPrinter::Unit
 - Remove the GetQtUnit() method, and its declaration, and all of its calls.
 - Update SetPaperSizeUnit to look like this:
NS_IMETHODIMP
nsPrintSettingsQt::SetPaperSizeUnit(PRInt16 aPaperSizeUnit)
{
    mPaperSizeUnit =
      (aPaperSizeUnit == nsIPrintSettings::kPaperSizeMillimeters) ?
      QPrinter::Millimeter : QPrinter::Inch;
}
Addresses issues from comment 26/30/31/35

>> I still don't see a PR_STATIC_ASSERT anywhere.
Fixed

>> Perhaps that's because you forgot to change the type of mPaperSizeUnit to be
>> "QPrinter::Unit"?  That would cause type-conversion errors.  There shouldn't >> be anything inherent to this change that would cause compiler errors/warnings, though.

mPaperSizeUnit is a member variable of a class that is common with gtk.  Not changed for now.
Attachment #478316 - Attachment is obsolete: true
Attachment #478378 - Flags: review?(dholbert)
Attachment #478316 - Flags: review?(dholbert)
Comment on attachment 478378 [details] [diff] [review]
Followup Patch: Addresses issues from comment 26/30/31/35

(In reply to comment #36)
> mPaperSizeUnit is a member variable of a class that is common with gtk.  Not
> changed for now.

Yup, that's what I was missing -- sorry for missing that, and thanks for the clarification.

r=dholbert
Attachment #478378 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Attachment #477624 - Attachment description: Fixed review comments from 22/23 → Main Patch: Fixed review comments from 22/23
Attachment #478378 - Attachment description: Addresses issues from comment 26/30/31/35 → Followup Patch: Addresses issues from comment 26/30/31/35
Here's the main patch again (from attachment 477624 [details] [diff] [review]), with bitrot from bug 597249 fixed so that it applies cleanly.

Later today, I'm planning on folding this together with the followup patch (attachment 478378 [details] [diff] [review]) and landing on Tom's behalf.
Attachment #477624 - Attachment is obsolete: true
Attachment #475186 - Attachment is obsolete: true
Attachment #457797 - Attachment is obsolete: true
Attachment #460680 - Attachment is obsolete: true
Landed:
 http://hg.mozilla.org/mozilla-central/rev/40ea53a130a3

Reclassifying as Core / Widget:Qt, since that's where the changed code here actually lives.
Status: NEW → RESOLVED
Closed: 14 years ago
Component: General → Widget: Qt
Keywords: checkin-needed
Product: Fennec → Core
QA Contact: maemo-linux → qt
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.