332927 - Clean up iframe/frame printing code

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Eli Friedman]

This patch implements a bunch of cleanups for printing iframes: not using page sequences for subdocuments on a page, hooking subdocuments into the parent document in both print and print preview, and hooking subdocuments into the parent document's view tree.  Also some other misc code cleanup.  Note that this changes the code to create scrollframes for subdocuments in print preview; not doing that led to clipping problems.

Blocks 332806 because the fix for that is in this patch; mPresContext wasn't always a valid pointer.  It was always rather confusing, and unneeded, so I got rid of it.

Patch coming up.

Comment 1

[Eli Friedman]

Attachment: Patch

Comment 2

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

Just as an aside, do you know why we need nsPageFrame's child (nsPageContentFrame?) to have a widget?

Comment 3

[Eli Friedman]

I just tried it; I guess my changes make it possible.  I'll get rid of it.  New patch coming up soon.

Comment 4

[Eli Friedman]

Attachment: Patch v2

By the way, this patch makes it so that iframes can be scrolled in print preview; do you think that's fine?

Comment 5

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

I think we don't want anything to be scrollable in print preview. Should be easy to fix that.

Comment 6

[Eli Friedman]

Attachment: Patch v3

Okay, I think this takes care of all the issues.

Comment 7

[Eli Friedman]

Attachment: Patch v4

Sorry about all the patches; apparently not having BeginObservingDocument and EndObservingDocument around the initial reflow causes problems for textareas.

Comment 8

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

Comment on attachment 217746 [details] [diff] [review]
Patch v4

+    if (aPO->mParent->mPresShell) {
+      frame = aPO->mParent->mPresShell->FrameManager()->
+                  GetPrimaryFrameFor(aPO->mContent);
+    }

It would be better to call nsLayoutUtils::GetCrossDocParentFrame on aPO's root frame, then things won't break if we ever allow subdocuments to break.

Why do we need ReflowDocList? Can't we reflow child documents through nsSubDocumentFrame::Reflow like we normally do? If the answer is "yes", don't bother putting it in this patch, just land this and do another patch.

Comment 9

[Eli Friedman]

(In reply to comment #8)
> (From update of attachment 217746 [details] [diff] [review] [edit])
> +    if (aPO->mParent->mPresShell) {
> +      frame = aPO->mParent->mPresShell->FrameManager()->
> +                  GetPrimaryFrameFor(aPO->mContent);
> +    }
> 
> It would be better to call nsLayoutUtils::GetCrossDocParentFrame on aPO's root
> frame, then things won't break if we ever allow subdocuments to break.

That's not really doable.  At this point, the subdocument has no frames; that content object is obtained by going through the parent document's content tree.  What do you mean by allowing subdocuments to break?  The approach is already pretty broken.  For example, it will completely miss anonymous content, because it's not in the content tree.  Anyway, the worst that can happen is that the subdocument won't print.  This will all go away after we get rid of ReflowDocList (see below).
 
> Why do we need ReflowDocList? Can't we reflow child documents through
> nsSubDocumentFrame::Reflow like we normally do? If the answer is "yes", don't
> bother putting it in this patch, just land this and do another patch.

Yes, but it's not a simple patch; one thing that requires new code is the separated frames case, which currently requires the frames to be reflowed and printed separately.  I'm not exactly sure how that will work, but we'll figure it out.  I'll file a bug on that.

Also, on a side note, how do we clone a document with subdocuments?  I don't really understand how docshells work.

Comment 10

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

Sounds reasonable.

Yes, cloning a document with subdocuments is going to be tricky. The docshell/contentviewer/globalwindow mess is nasty. We'll need to create a new docshell as normal and then instead of loading a document, we give it a recursively cloned document.

Comment 11

[Eli Friedman]

Checked in.

Comment 12

[Steve England [:stevee]]

Could this have caused bug 333590?