331415 - Some printing code cleanup

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Eli Friedman]

I have a sort of random set of cleanups in the printing code that I came up with by going through it.  There's still much more to be done, but this is a start.

It's really amazing how low the quality is of the printing code in layout, with lots of code making false assumptions, doing things that don't have any effect, and recalulating various aspects of a document through multiple code paths.  Overall, I would say it's some of the worst code in Gecko.  I'm not suprised it hasn't been touched in so long, since it's really hard to follow.

Patch coming up.

Comment 1

[Eli Friedman]

Attachment: Patch v1

This conflicts slightly with Bug 244055, but I should be able to resolve the conflicts pretty easily.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Overall, I would say it's some of the worst code in Gecko.

I wouldn't disagree with you there!

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you land the patch in bug 244055, BTW? I'm not sure what's up with Alexandre. Before or after this one, I don't mind.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 215966 [details] [diff] [review]
Patch v1

+   * Get/set whether this document should be treated as having real pages

Can you document what this means? I think this means that this document contains a page sequence, and when it's not set but the document is paginated, we're dealing with a paginated subdocument like an IFRAME inside a paginated document (which kinda makes sense since it's not inconceivable we might eventually want to break IFRAMEs across pages). Perhaps you should rename this to HasPageSequence()?

-      aPO->mWindow->Show(aShow);

Why don't we need this, or something like it?

I admit I don't know the function of a lot of stuff you're removing, but I don't care to find out. rubberstamp=me

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW ... one idea that's been floating around is to having printing clone the document tree to a new document tree before printing it. Then print selection, print frames to separate pages, etc, could just be tweaks to the cloning process. We could strip script and remove the hacks that currently prevent scripting in printed/print-previewed documents. But more importantly we wouldn't then need any support for multiple presentations per document. We could have print preview come up in a new window and let the user continue to work with the old document. Etc...

Comment 6

[Eli Friedman]

(In reply to comment #4)
> (From update of attachment 215966 [details] [diff] [review] [edit])
> +   * Get/set whether this document should be treated as having real pages
> 
> Can you document what this means? I think this means that this document
> contains a page sequence, and when it's not set but the document is paginated,
> we're dealing with a paginated subdocument like an IFRAME inside a paginated
> document (which kinda makes sense since it's not inconceivable we might
> eventually want to break IFRAMEs across pages). Perhaps you should rename this
> to HasPageSequence()?

Okay, it means that we have a subdocument for a paginated document, like you correctly figured out.  However, all printed documents and subdocuments have a page sequence frame; this isn't noticable because only the first page of the subdocuments is actually printed.  Stop assuming the code is logical! :)  I'm leaving fixing that to another bug.  IsRealPages was created to be a much cleaner way to express that completely stupid situation than the various completely backward and potentially buggy ways it was being noted.

The sort of pagination of subdocuments you seem to be thinking of is completely nonexistent at the moment.  I don't see how paginating IFRAMEs is useful, though, since they are a fixed size anyway.  Paginating frames might be useful, but that doesn't seem like it's worth worrying about.  If there's some use case I'm messing, please tell me.

I'll put a better XXX comment there. As for the name, I'll change it to IsRootPaginatedDocument would be more self-explanatory.

> -      aPO->mWindow->Show(aShow);
> 
> Why don't we need this, or something like it?
> 
> I admit I don't know the function of a lot of stuff you're removing, but I
> don't care to find out. rubberstamp=me
> 

The way the print preview code creates a widget, it's visible from creation; therefore, there's no need to show it.

(In reply to comment #5)
> BTW ... one idea that's been floating around is to having printing clone the
> document tree to a new document tree before printing it. Then print selection,
> print frames to separate pages, etc, could just be tweaks to the cloning
> process. We could strip script and remove the hacks that currently prevent
> scripting in printed/print-previewed documents. But more importantly we
> wouldn't then need any support for multiple presentations per document. We
> could have print preview come up in a new window and let the user continue to
> work with the old document. Etc...

Yeah, that would be a good idea; copying the document wouldn't be too hardto change in the nsPrintEngine code, and would protect against interference from other windows during print preview.  Messing around with the document tree would be more work, although possibly necessary for print previewing some outputs.  Redoing how printing selection works needs more discussion, though; we want to consider carefully any changes that would result in a dfferent user experience.

Changing the code to create a new window would be more difficult at this point because it would require some UI code changes, and a new API.  The UI needs a makeover anyway, but it would probably have to wait until after Firefox development moves back onto the trunk (I doubt it would work out to get a printing mechanism rewrite into 1.8.1 without the Firefox peers making it a priority).

One annoying part of all this would be the frozen nsIWebBrowserPrint interface.  Maybe we'll make an nsIWebBrowserPrint2 and stick a couple extra methods onto that; that would also allow the UI changes in the XUL apps to go in whenever (once they have switched to 1.9).  It wouldn't be too much extra code to have multiple print preview APIs, so that option could work out.

Also, we probably want to consider other possible UI changes before making these changes, so that we can consider what backend changes are necessary.  Where would be a good place to start a "how printing should work" discussion?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> Okay, it means that we have a subdocument for a paginated document, like you
> correctly figured out.  However, all printed documents and subdocuments have
> a page sequence frame; this isn't noticable because only the first page of
> the subdocuments is actually printed.  Stop assuming the code is logical! :)
> I'm leaving fixing that to another bug.

That might not be a bad thing if we ever want to break subdocuments across pages.

> IsRealPages was created to be a much
> cleaner way to express that completely stupid situation than the various
> completely backward and potentially buggy ways it was being noted.

Right.

> The sort of pagination of subdocuments you seem to be thinking of is
> completely nonexistent at the moment.

Yep

> I don't see how paginating IFRAMEs is useful, though, since they are a fixed
> size anyway.

Yes, but that fixed size could be arbitrarily large. It would be nice to be able to print very tall IFRAMEs in a reasonable way. It should not be a priority to get this working, of course.

> I'll put a better XXX comment there. As for the name, I'll change it to
> IsRootPaginatedDocument would be more self-explanatory.

Sounds good.

> (In reply to comment #5)
> Yeah, that would be a good idea; copying the document wouldn't be too hard to
> change in the nsPrintEngine code, and would protect against interference from
> other windows during print preview.  Messing around with the document tree
> would be more work, although possibly necessary for print previewing some
> outputs.  Redoing how printing selection works needs more discussion, though;
> we want to consider carefully any changes that would result in a dfferent
> user experience.

Yes, but I suspect the current code's user experience is "horrible" in a large range of situations :-).

> Changing the code to create a new window would be more difficult at this
> point because it would require some UI code changes, and a new API.  The UI
> needs a makeover anyway, but it would probably have to wait until after
> Firefox development moves back onto the trunk (I doubt it would work out to
> get a printing mechanism rewrite into 1.8.1 without the Firefox peers making
> it a priority).

Definitely. I'm not suggesting we dive into that now. But if you have time and the will to do it, going to a document cloning approach ASAP might let us avoid having to fix some of the kinks in the existing code. One thing that I forgot to mention is that I think using document cloning to achieve effects like putting frames on separate pages, we can then get rid of the whole printobject tree, and just allow printed documents to display their subdocuments directly like we do on the screen. This would fix a multitude of issues, for example, currently z-ordering of subdocuments in print/print preview doesn't work because we just render the base document and then paste on the subdocuments.

> Also, we probably want to consider other possible UI changes before making
> these changes, so that we can consider what backend changes are necessary. 
> Where would be a good place to start a "how printing should work" discussion?

On a wiki page, I guess.

You should look at the printing features that will be in IE7 --- not that we have to clone them, but they'll give us a good set of requirements for the back end. Things like dynamic margin changes -> incremental reflow of print-previewed documents. Dynamic text-zoom and scale changing.

One thing I think would be cool is to not display a print-previewed document directly, but leave it hidden, and expose the geometry of its pages through a XUL API. Then XUL could construct the print preview window, draw its own page boundaries etc, and use drawWindow to just copy an image of each page into the print preview window. This would make it easy to display multiple pages in the window with different arrangements, add per-page UI to the window, and so on.

Another small thing we could do which I think wouldn't be very hard once we've done the above, is to give the user per-page control over landscape vs portrait. So if you print-preview a document in portrait, and content on one of the pages is too wide, double click on it (or whatever) and that page turns into landscape.

Comment 8

[Eli Friedman]

(In reply to comment #7)
> > Redoing how printing selection works needs more discussion, though;
> > we want to consider carefully any changes that would result in a dfferent
> > user experience.
> 
> Yes, but I suspect the current code's user experience is "horrible" in a large
> range of situations :-).
> 
> > Changing the code to create a new window would be more difficult at this
> > point because it would require some UI code changes, and a new API.  The UI
> > needs a makeover anyway, but it would probably have to wait until after
> > Firefox development moves back onto the trunk (I doubt it would work out to
> > get a printing mechanism rewrite into 1.8.1 without the Firefox peers making
> > it a priority).
> 
> Definitely. I'm not suggesting we dive into that now. But if you have time and
> the will to do it, going to a document cloning approach ASAP might let us avoid
> having to fix some of the kinks in the existing code. One thing that I forgot
> to mention is that I think using document cloning to achieve effects like
> putting frames on separate pages, we can then get rid of the whole printobject
> tree, and just allow printed documents to display their subdocuments directly
> like we do on the screen. This would fix a multitude of issues, for example,
> currently z-ordering of subdocuments in print/print preview doesn't work
> because we just render the base document and then paste on the subdocuments.

I think that converting subdocuments to display with their parent document is independent of cloning the document tree; it's just an issue of attaching to the parent document like we do in print preview during reflow and the resulting code cleanup.

Also, how could you construct a document tree that would separate frames properly?  I don't see how that would work without paginated subdocuments; I guess that's a motivation for doing that.

> > Also, we probably want to consider other possible UI changes before making
> > these changes, so that we can consider what backend changes are necessary. 
> > Where would be a good place to start a "how printing should work" discussion?
> 
> On a wiki page, I guess.

OK.

> You should look at the printing features that will be in IE7 --- not that we
> have to clone them, but they'll give us a good set of requirements for the back
> end. Things like dynamic margin changes -> incremental reflow of
> print-previewed documents. Dynamic text-zoom and scale changing.

I will check it out.

BTW, Alexandre said he's planning on checking in Bug 244055 tomorrow.  Thought you might like to know.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> I think that converting subdocuments to display with their parent document is
> independent of cloning the document tree; it's just an issue of attaching to
> the parent document like we do in print preview during reflow and the
> resulting code cleanup.

It mostly is, but it might be harder to get rid of the printobject tree and print frames separately if we don't clone the document.

> Also, how could you construct a document tree that would separate frames
> properly?  I don't see how that would work without paginated subdocuments; I
> guess that's a motivation for doing that.

Hmm. Yeah :-).

> BTW, Alexandre said he's planning on checking in Bug 244055 tomorrow.
> Thought you might like to know.

Great!

Comment 10

[Eli Friedman]

Just rebooted to try out the new IE.  The printing is definitely not polished, and the performance sucks, but I'll give them the benefit of the doubt because it's a beta.

I really don't see the point of changing the default .5 inch margins in most cases; the best way to stuff more onto a page is with scaling, and I don't see wanting to put less on a page.  So, interesting, but not too useful.  Hmm, I thought it actually continuously reflowed from all the descriptions I've heard.

IE's different page views are different, although I'm not sure about better.  Some discussion would be good.

This stuff definitely requires more flexibility from the print code.  If we're not trying to implement continuous margin reflow, drawWindow would be fast enough.  The other posssibility is a specialized XUL element to display pages, which would paint directly to the screen; I guess since stuff on paper can't animate, the performance difference would be negligible, although the memory usage for a zoomed in page could be overwhelming.  Either way would provide the flexibility to have an IE-like print preview.

Page setup dialog: I'm curious why our page setup dialog doesn't have a paper size setting.  We have a better UI for header configuration.

The enable-disable headers option on the toolbar seems useful; probably worth having.

We *have* to copy IE's handling of selection printing.  They got it right. (Try selecting something and going into print preview if you haven't seen it).  This is subtle, but so much more useful than the other stuff; I wonder why nobody mentions it.

On a more technical note, it looks like the unselected stuff has no impact on the display of the selection for IE's implementation; I think we should follow their lead.  I'm thinking reflow or frame construction will have to collapse the non-selected stuff.

On the subject of frames: the frame handling is provided through the same mechanism as the selection handling; they provide the options we do, but in print preview as well as the print dialog.

Having a help icon is also nice.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #10)
> This stuff definitely requires more flexibility from the print code.  If
> we're not trying to implement continuous margin reflow, drawWindow would be
> fast enough.  The other posssibility is a specialized XUL element to display
> pages, which would paint directly to the screen; I guess since stuff on paper
> can't animate, the performance difference would be negligible, although the
> memory usage for a zoomed in page could be overwhelming.  Either way would
> provide the flexibility to have an IE-like print preview.

I've been thinking we might want a new element + frame type that just renders a view of some other document, a sort of live drawWindow. That would be useful here, and useful for all sorts of other things too. Pretty easy to implement.

> On a more technical note, it looks like the unselected stuff has no impact on
> the display of the selection for IE's implementation; I think we should
> follow their lead.  I'm thinking reflow or frame construction will have to
> collapse the non-selected stuff.

I think the easiest way to handle this would be to do document cloning and only clone the selected content, plus some carefully chosen context. This would ultimately be a lot simpler and easier to understand than messing with reflow, and more powerful. For example we could clone table headers if part of a table is selected. In fact we could remove the support in paint code for hiding unselected content.

Comment 12

[Eli Friedman]

Attachment: Final version

This is the version I just checked in.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One goal to think about when cleaning up this code is making it easy to put print preview in its own window...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See comment #5 :-)