107562 - Implement Print Preview

Status: VERIFIED WORKSFORME

(Core :: Printing: Output, defect)

Description

[rods (gone)]

Comment 1

[rods (gone)]

Attachment: Initial patch for Win & Linux

Comment 2

[Jacek Piskozub]

Shouldn't it be a All/All bug as we do not have a Windows/Linux OS choice? It
may become important when verifying.

Comment 3

[Marc Attinasi]

Ah, a nice small patch ;)

Comments:

mozilla/gfx/public/nsDeviceContext.h
+  NS_IMETHOD GetAltDevice - Cannot make a virtual method inline, sorry :(
+  nsIDeviceContext* mAltDC; - why not use a COMPtr?


mozilla/gfx/public/nsIDeviceContext.h
/* comments need to be provided for the new methods and constants */

mozilla/gfx/src/windows/nsDeviceContextWin.cpp
+  - empty line is unnecessary... please remove

mozilla/content/base/src/nsDocumentViewer.cpp
+  - empty line is unnecessary... please remove

+  // Destroy the old Presentation
+  mPresShell    = do_QueryInterface(nsnull);
+  mPresContext  = do_QueryInterface(nsnull);
+  mViewManager  = do_QueryInterface(nsnull);
+  mWindow       = do_QueryInterface(nsnull);
Is this necessary? Can't you just assign to nsnull? What does the QI(null) call
do here anyway?

mozilla/layout/base/src/nsPresContext.cpp
+nsPresContext::GetTwipsToPixelsForFonts - comments about the use of the altDC 
                                           would be nice here...
mozilla/layout/base/src/nsPrintPreviewContext.cpp
+PrintPreviewContext::QueryInterface(REFNSIID aIID, void** aInstancePtr)
+{
+ 
Empty line is unnecessary, and you could assert for null out-param (as you did
in many other places)

mozilla/layout/html/base/src/nsPageFrame.cpp
+    //nscoord quarterInch = NS_INCHES_TO_TWIPS(0.25);
+    //rect.Deflate(quarterInch,0);
why not remove them?

+#ifdef DEBUG_PRINTING
   if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer) {
why is there this big difference here? Please comment as I won't be the only one
confused by it.

mozilla/layout/html/base/src/nsSimplePageSequence.cpp
+    mPageData->mHeadFootFont = new nsFont("serif",
NS_FONT_STYLE_NORMAL,NS_FONT_VARIANT_NORMAL,
+                               NS_FONT_WEIGHT_NORMAL,0,NSIntPointsToTwips(10));
+    NS_ASSERTION(mPageData->mHeadFootFont != nsnull, "mHeadFootFont cannot be
null!");
you cannot assert that new succeeds, can you? You need to handle this potential
error.

+    //vm->Display(view, mOffsetX+mMargin.left, mOffsetY+mMargin.top);
 why not remove this?

html.css
+  *|*:-moz-any-link img, img[usemap], object[usemap] {
+    -moz-user-focus: none !important;
+  }
+
+  *|*:-moz-any-link {
+    -moz-user-focus: none !important;
+  }
+
+  img[usemap], object[usemap], object, embed, applet, iframe {
+    -moz-user-focus: none !important;
+  }
these can be combined into one rule.

mozilla/view/src/nsViewManager2.cpp
mozilla/view/src/nsViewManager2.h
These files are no longer part of any build - no need to mess with them at all.


So, overall I say it looks good! The only really critical aspects of my comments
above are the internal documentation ones - this stuff needs to be commented so
others have a hope of understanding it. Also, you might want to reference your
design doc somewhere in the PrintPreview code (and publish it, of course) - that
will help people understand it. Also, it would be very nice to #ifdef this since
some platforms and/or build will not want it. No need to make the code bloat the
app if it won't be used!

Also, what have you done to confirm that there are no leaks (or impact on
general performance when NOT using print preview)?

Comment 4

[Kevin McCluskey (gone)]

Wrap all print-preview related code in NS_PRINT_PREVIEW so we can build without
print-preview code form platforms such as Mac OSX which has its own built-in
print-preview.

nsDeviceContextWin::GetDeviceSurfaceDimensions

Is it correct that you only want to use the mAltDC if the mSpec is nsnull? Seems
like you would always want to use the mAltDC if the flag UseAltDCFor_SURFACE_DIM
is set.

nsDocumentViewer

Need big comment which expains how print preview works or at least redirect them
to your design document which should be added as an attachment to a bug that can
be referenced from a comment in nsDocumentViewer.

Individual members like mParentWidget should have comments explaining their purpose.


nsViewManger.h:
DiscardEvent method is not used. You need to remove it from the nsViewManager.h


nsViewManager2:
Do not need to make changes to nsViewManager2. It is obsolete and is no longer
part of the build.

Comment 5

[rods (gone)]

Attachment: Revised Initial patch

Comment 6

[Marc Attinasi]

There looks to be more stuff to #ifdef in there: 
+  static PRBool mIsDoingPrintPreview;
for example (there are more too). Also, I'm assuming you compiled with and
without the flag set... sr=attinasi

Comment 7

[rods (gone)]

Actually, that is now a data member instead of a global static. And yes I have
been compiling both ways, but I will do one extra check before checking it in.
(thanks for the reminder)

Comment 8

[Kevin McCluskey (gone)]

Comment on attachment 56282 [details] [diff] [review]
Revised Initial patch

r=[e-mail]. Looks good!

Comment 9

[Matthias Versen [:Matti]]

*** Bug 108375 has been marked as a duplicate of this bug. ***

Comment 10

[Peter Van der Beken [:peterv]]

Is this also going to be implemented/turned on on classic Mac? We do have the
menu item, but nothing happens...

Comment 11

[rods (gone)]

The idea is to have it working for non-OSX Mac, but I will need some help with 
that. The menu appears because I wasn't able to get the platform xul stuff 
working correctly so it would only show up on Windows and Linux.

Comment 12

[Brian King [:kinger]]

A few comments:

1) Bugs 17006 and 20943 look like dupes. I'll leave it to rods to decide.

2) It needs to be added to the print toolbar menubutton.

3) Need a way to break out of print preview mode.

Looks great by the way!

Comment 13

[Roland Mainz]

rods:
Wanna update the Xlib toolkit like you did it for GTK+ code (it's simple, both
tookkit work ~~98% the same way), please ?

Comment 14

[rods (gone)]

Need to have this done by then

Comment 15

[Roland Mainz]

Attachment: gfx/src/xlib patch for 2001-11-30-08-trunk

lib gfx port of GTK+ changes in CVS
Requesting rs= for this port ...

Comment 16

[rods (gone)]

r=rods for XLib patch

Comment 17

[rods (gone)]

it is impl'ed

Comment 18

[sujay]

verified.