Closed Bug 107562 Opened 23 years ago Closed 23 years ago

Implement Print Preview

Categories

(Core :: Printing: Output, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

VERIFIED WORKSFORME
mozilla1.2alpha

People

(Reporter: rods, Assigned: rods)

References

Details

Attachments

(3 files)

 
Blocks: 103890
Status: NEW → ASSIGNED
Shouldn't it be a All/All bug as we do not have a Windows/Linux OS choice? It
may become important when verifying.
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)?

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.


Target Milestone: --- → mozilla0.9.6
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

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 on attachment 56282 [details] [diff] [review]
Revised Initial patch

r=kmcclusk@netscape.com. Looks good!
Attachment #56282 - Flags: review+
*** Bug 108375 has been marked as a duplicate of this bug. ***
Is this also going to be implemented/turned on on classic Mac? We do have the
menu item, but nothing happens...
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.
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!
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Depends on: 111213
rods:
Wanna update the Xlib toolkit like you did it for GTK+ code (it's simple, both
tookkit work ~~98% the same way), please ?
Need to have this done by then
Target Milestone: mozilla0.9.7 → mozilla1.0
Keywords: mozilla1.0
lib gfx port of GTK+ changes in CVS
Requesting rs= for this port ...
r=rods for XLib patch
it is impl'ed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → WORKSFORME
Target Milestone: mozilla1.0 → mozilla1.2
verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: