Closed
Bug 107562
Opened 23 years ago
Closed 23 years ago
Implement Print Preview
Categories
(Core :: Printing: Output, defect)
Tracking
()
VERIFIED
WORKSFORME
mozilla1.2alpha
People
(Reporter: rods, Assigned: rods)
References
Details
Attachments
(3 files)
112.39 KB,
patch
|
Details | Diff | Splinter Review | |
138.03 KB,
patch
|
kmcclusk
:
review+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Comment 2•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Target Milestone: --- → mozilla0.9.6
Assignee | ||
Comment 5•23 years ago
|
||
Comment 6•23 years ago
|
||
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
Assignee | ||
Comment 7•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 56282 [details] [diff] [review] Revised Initial patch r=kmcclusk@netscape.com. Looks good!
Attachment #56282 -
Flags: review+
Comment 9•23 years ago
|
||
*** Bug 108375 has been marked as a duplicate of this bug. ***
Comment 10•23 years ago
|
||
Is this also going to be implemented/turned on on classic Mac? We do have the menu item, but nothing happens...
Assignee | ||
Comment 11•23 years ago
|
||
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•23 years ago
|
||
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!
Updated•23 years ago
|
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Comment 13•23 years ago
|
||
rods: Wanna update the Xlib toolkit like you did it for GTK+ code (it's simple, both tookkit work ~~98% the same way), please ?
Assignee | ||
Comment 14•23 years ago
|
||
Need to have this done by then
Target Milestone: mozilla0.9.7 → mozilla1.0
Assignee | ||
Updated•23 years ago
|
Keywords: mozilla1.0
Comment 15•23 years ago
|
||
lib gfx port of GTK+ changes in CVS Requesting rs= for this port ...
Assignee | ||
Comment 16•23 years ago
|
||
r=rods for XLib patch
Assignee | ||
Comment 17•23 years ago
|
||
it is impl'ed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → WORKSFORME
Target Milestone: mozilla1.0 → mozilla1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•