Last Comment Bug 487667 - Clone documents for printing
: Clone documents for printing
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Print Preview (show other bugs)
: Trunk
: x86 All
-- normal with 2 votes (vote)
: ---
Assigned To: Olli Pettay [:smaug] (pto-ish for couple of days)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 534316 573537 517543 534407 534502 534720 535343 535851 539645 544018 545330 560235 651028
Blocks: 110641 229080 435136 468568 500882 520501 526403
  Show dependency treegraph
 
Reported: 2009-04-09 12:22 PDT by Olli Pettay [:smaug] (pto-ish for couple of days)
Modified: 2016-09-19 16:19 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
an example for dynamic print styles (340 bytes, text/html)
2009-04-09 12:22 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details
initial WIP (48.30 KB, patch)
2009-04-10 04:39 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
WIP (73.82 KB, patch)
2009-04-29 15:32 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v7 (107.69 KB, patch)
2009-07-10 15:17 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v14 (168.86 KB, patch)
2009-09-08 04:58 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v15 (173.95 KB, patch)
2009-09-08 09:05 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v15.1 (173.96 KB, patch)
2009-09-08 09:51 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v19 (164.31 KB, patch)
2009-09-16 12:50 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
anim image tests (1.30 KB, text/html)
2009-09-17 03:28 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details
+animated image cloning (190.80 KB, patch)
2009-09-17 04:21 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
roc: review+
Details | Diff | Splinter Review
v20.1 (190.96 KB, patch)
2009-09-18 05:44 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
jst: review+
dbaron: review+
bobbyholley: review-
Details | Diff | Splinter Review
v21 (191.22 KB, patch)
2009-11-13 11:44 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v22 (195.86 KB, patch)
2009-11-13 14:29 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v23 (199.43 KB, patch)
2009-11-13 15:59 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v24 (198.45 KB, patch)
2009-11-14 14:15 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
roc: review+
Details | Diff | Splinter Review
v24.1 (199.78 KB, patch)
2009-11-17 04:45 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
joe: review-
Details | Diff | Splinter Review
v25 (195.98 KB, patch)
2009-11-17 14:52 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
joe: review+
Details | Diff | Splinter Review
v29 (126.53 KB, patch)
2009-12-11 09:20 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review

Description User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-09 12:22:06 PDT
So we want to clone documents for printing.

There are few things to think about, for example
1) How to handle XBL
2) How to clone stylesheets/rules
3) and how to clone document, but use the cloned stylesheets/rules
...
Comment 1 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-09 12:22:37 PDT
Created attachment 371905 [details]
an example for dynamic print styles
Comment 2 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-09 16:08:33 PDT
I think we could ignore XBL until XBL2 arrives at least. I can't imagine people seriously use XBL with printed documents.

When cloning, how about serializing all style sheets and inline styles to inline <style> blocks?
Comment 3 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-04-09 16:31:41 PDT
I agree regarding XBL. It'll mean that <video>s won't have controls, and <marquee>s won't scroll, but in both cases that sounds like what we want anyway.

Doesn't the CSS code have a copy-on-write powers already? Can we use those?
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-09 17:32:21 PDT
What's the concern with cloning stylesheets, exactly?
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-09 17:33:20 PDT
And to be clear, I'm not quite sure what the second paragraph of comment 2 is suggesting...
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-09 17:54:28 PDT
I was just suggesting that if it's difficult to clone stylesheets and ensure the clones are properly attached to the cloned document, we could serialize the stylesheets of the original document and paste those serializations as inline style sheets in the cloned document.
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-09 18:10:40 PDT
I don't think there should be any issues with cloning sheets.  We do it all the type for chrome, for example: all the chrome sheets are clones of the sheets living in the prototype cache.
Comment 8 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-10 01:52:36 PDT
(In reply to comment #4)
> What's the concern with cloning stylesheets, exactly?
Maybe nothing. I found some usable API after filing this bug :)
Going to test how it works...

4) who should own the cloned document,
5) and should there be a separate docshell for it,
6) or is re-using document viewer ok. (though global object handling might get tricky)
Comment 9 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-10 02:02:23 PDT
For print preview docshell could have a method like
cloneAndLoadAsPrintPreview(nsIDOMDocument* aDoc)
That would initialize things like for normal pages, but just use the cloned
document and its stylesheets. That way, depending on the UI requirements, print preview could be opened in a new tab or window. Well, even using the same
tab should work, but that would push the original page to bfcache (if bfcachable).

I'm not sure about printing, but I think document viewer could handle that, pretty much like it is handled now.
Comment 10 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-10 03:05:18 PDT
The cloned document should behave much like an external document IMHO. That is, it should have its own documentviewer but no associated docshell. The cloned document will never be viewed directly. After it has reflowed, the print engine can just print it. For print preview I want to create new XUL API that lets chrome create "page elements" and render the print document into them. This will let chrome and themes control the look and behaviour of the decorations around each page and other aspects of print preview. For example, chrome could display pages "N-up", allow the user to visually select pages to print, etc.

This approach means that layout will no longer need to distinguish between "print" and "print preview" presentations. Some code can be removed from layout. We can be 100% sure that the print layout matches the print preview layout exactly, since they're the same layout. We no longer have to worry about the pesky "print preview" scrollbar. The strange print preview frame tree (where there's one nsViewportFrame at the root and each page is *also* an nsViewportFrame in the same frame tree) will go away.

For the XUL API, we'll need a way to query the print document for the number of pages it has. To render the pages, I think the best approach would be to create a <portal> element with a setPage(doc,pageNumber) method. Its paint method would adjust the clipping and transform of the passed-in gfxContext and pass it off to the print document for painting.

But I think the first step should be to get the cloned document approach working with printing.
Comment 11 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-10 04:39:08 PDT
Created attachment 372035 [details] [diff] [review]
initial WIP

Doesn't yet take care of iframes/frames nor catalogsheets etc.
Removes the textfragment cloning.
Comment 12 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-10 04:47:35 PDT
..and script handling is a nice hack. Will replace with something else.
Comment 13 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-10 05:06:41 PDT
Handling plugins will be interesting. One approach would be to render the plugin into a canvas and replace the plugin with the canvas in the cloned document. This should work for windowless plugins but will probably fail hopelessly on windowed Linux and Windows plugins. Then again, I'm not sure printing works for them anyway.
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-10 09:36:10 PDT
We used to have cases where plugins generated their own postscript, at least on Linux.  Has that gotten removed anyway with the various linux printing and cairo work in the 1.9 timeframe?

That is, some plugins (java comes to mind) basically did vector, not bitmap, printing.  The canvas approach would be a regression from that.
Comment 15 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-10 10:00:03 PDT
iframes/frames working, using an ugly hack.
Will cleanup while trying to get <img> working.
Comment 16 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-10 11:44:03 PDT
So as a first step I'm aiming to provide similar printing behavior what we
have now, but using cloned documents. Already that allows us to replace
nsTObserverArray<nsIPresShell*> with nsIPresShell* in nsIDocument and
nsPresShellIterator.h can be removed.

And if wanted, nsIContent objects could have a pointer to their primary frame
(if we really want that). nsIContent could probably have nsIFrame* member variable which is set lazily when primary frame is first time needed.
And when destroying an nsIFrame, that could clear the relevant nsIContent::mFrame.
Comment 17 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-10 12:05:28 PDT
That last bit wouldn't work well given our current <area> mess, so let's not worry about it quite yet.
Comment 18 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-10 13:17:44 PDT
(In reply to comment #14)
> We used to have cases where plugins generated their own postscript, at least on
> Linux.  Has that gotten removed anyway with the various linux printing and
> cairo work in the 1.9 timeframe?

Yes.
Comment 19 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-10 13:19:07 PDT
(In reply to comment #16)
> And if wanted, nsIContent objects could have a pointer to their primary frame
> (if we really want that). nsIContent could probably have nsIFrame* member
> variable which is set lazily when primary frame is first time needed.

There's no reason to not set it eagerly as far as I know.
Comment 20 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-04-29 15:32:13 PDT
Created attachment 375093 [details] [diff] [review]
WIP

This contains printfs and leaks and is full of hacks, but
printing images/form elements/frames etc work.

Apparently printing plugins doesn't work on OSX, so have to wait until
I have access to linux again.
Comment 21 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-26 20:52:29 PDT
I've spun off the second half of comment 16 into bug 500882.
Comment 22 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-07-10 15:17:32 PDT
Created attachment 387958 [details] [diff] [review]
v7

Uploading the patch so that I have the backup.

Printing <video> doesn't work yet (it doesn't work in 1.9.1/trunk, well, not in any reasonable way).
Plugin printing isn't there yet either - it doesn't work on my main dev. environment, linux, anyway.

Frame printing should work, and selection/multiple selections printing.

I'm not quite happy how print preview setup works. It basically overrides the
current presentation of some content viewer. Probably the presentation should
be moved to cache and a new content viewer should be created for the print preview.

For printing an artificial docshell is created. It is basically just a container
for the document tree.

Misses automated tests.

Will update the patch and ask reviews hopefully on Monday or so.
Comment 23 User image Boris Zbarsky [:bz] (still a bit busy) 2009-07-10 15:23:56 PDT
Do you need the artificial docshell?  Can you get away with just a document viewer?
Comment 24 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-07-10 15:28:00 PDT
The docshell makes the setup pretty easy.
(I)Frames add their docshell to the parent docshell and print engine can then
iterate them and so on.
Comment 25 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-10 18:19:29 PDT
(In reply to comment #22)
> I'm not quite happy how print preview setup works. It basically overrides the
> current presentation of some content viewer. Probably the presentation should
> be moved to cache and a new content viewer should be created for the print
> preview.

The master plan makes the print preview presentation go away anyway, so don't worry about it.
Comment 26 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-07-15 12:16:24 PDT
(In reply to comment #22)
> Will update the patch and ask reviews hopefully on Monday or so.
Bah.
<video> is easy, but plugins are not, at least not for someone not too familiar
with our gfx :(
Comment 27 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-15 17:24:31 PDT
Can't you call nsObjectFrame::PaintPlugin directly or indirectly, passing in a context pointing to a temporary surface? That should paint the plugin if we possibly can.
Comment 28 User image Johnny Stenback (:jst, jst@mozilla.com) 2009-07-15 17:33:11 PDT
Some plugins expect to be able to do high resolution printing, Java in particular from what I hear. I know very little about how that's done on either end, but I hear they do vector drawing to somewhere for print output, printing to a lowres bitmap isn't good enough for them I don't think.

Hao, do you know anything about what the Java plugin does for printing?
Comment 29 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-15 19:43:35 PDT
You mean on Windows? Perhaps we can capture the print output to a Windows metafile and play it back...
Comment 30 User image Johnny Stenback (:jst, jst@mozilla.com) 2009-07-15 23:43:18 PDT
Yeah, on Windows. And that sounds like what they likely do today IIRC. No idea what happens on other platforms...
Comment 31 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-08 04:58:47 PDT
Created attachment 399186 [details] [diff] [review]
v14

Contains fix for Bug 514487.

External documents (svg effects etc) don't work, not sure how important and
this shouldn't be too difficult to fix.

Otherwise works pretty well, IMO.
I'd like to make the docshell setup a bit better still.
And more automatic tests. Currently we have tests only for very few cases like
form control handling.

- Plugin printing works like today.

- image animation as well, though without the bugs which cause animations to
  restart even if the image is being printed/previewed.

- SMIL animations aren't started when printing. (so at least this doesn't have
  the current "paint something random depending on the invalidation"
  behavior)

- <video> prints the current frame or poster image.
  (So the current random-painting bug is fixed.)

- selection printing works

- (i)frame printing works, also when the document is included using <object>

- internal svg effects work

- styles are cloned

Am I missing still something crucial?
Comment 32 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-08 09:05:27 PDT
Created attachment 399221 [details] [diff] [review]
v15

Fixes a small problem in media element handling and adds a helper method
to ensure reasonable document viewer for print preview.
Comment 33 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-08 09:29:30 PDT
Comment on attachment 399221 [details] [diff] [review]
v15

Asking first a sort of "what you think about this" review :)
Comment 34 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-08 09:51:23 PDT
Created attachment 399229 [details] [diff] [review]
v15.1
Comment 35 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-08 15:50:50 PDT
This is awesome.

+  PRBool IsPrintDocument() { return mIsPrintDocument; }
+
+  virtual already_AddRefed<nsIDocument>
+  CloneForPrint(nsISupports* aCloneContainer);
+
+  nsIDocument* GetOriginalDocument() { return mOriginalDocument; }

We'll need documentation for these.

+  PRPackedBool mIsPrintDocument;
+
+  PRPackedBool mCloningForPrint;
+
+  nsCOMPtr<nsIDocument> mOriginalDocument;

And these.

mCloningForPrint is kind of nasty, why can't we have a CloneNodeInternal which takes it as a parameter? Or do the "if (mCloningForPrint)" stuff in CloneForPrint after we've cloned the document?

+   * Returns the nsIObjectFrame object which should be used when
+   * printing the plugin.
+   */
+  [noscript] nsIFrame getPrintFrame();

It's not clear from the comment what this means or why you'd use it.

+  if (!doc || !(doc->IsPrintDocument() || mOwnerContent->IsInDoc())) {

Unnecessary parens

Is there a reason we don't do the stylesheet cloning in nsIDocument::CloneForPrint for all cloned documents?

Similar question for nsHTMLOptionElement and nsHTMLTextAreaElement, can't we just always clone the selection and the value? What about the input values for other input types?

Can you write a test for the ID-registration in nsNodeUtils::CloneAndAdopt? I guess that's an existing bug that script could encounter.

+    nsIFrame* nsiframe = do_QueryFrame(frame);
+    aDest->mPrintFrame = nsiframe;

nsiframe isn't a good name, I'd just call it 'f'

This way of handling plugins isn't ideal --- I really do think we should snapshot the plugin's current pixels if we can --- but I guess this is good enough for now.

+  if (doc && (aForceInDataDoc ||
+              (!doc->IsLoadedAsData() || doc->IsPrintDocument()))) {

Unnecessary parens

+		test_bug514487.html \

Looks like this file is missing?

How can we test image cloning, video cloning, etc? I wonder if we can just clone the document for printing into an iframe which is the same size as the viewport and use that for the test, somehow.

+  virtual PRBool IsInitializedForPrintPreview() = 0;
+
+  virtual void InitializeForPrintPreview() = 0;
+
+  virtual void SetPrintPreviewPresentation(nsIWidget* aWidget,
+                                           nsIViewManager* aViewManager,
+                                           nsPresContext* aPresContext,

Better document these

+      if (IsDynamic()) {
+        return;
+      } else {
+        ++gPrintImageRefs->Elements()[index];
+      }

Avoid else after return

nsPresContext::ResetPrintImageAnimation is yucky. Could we avoid this by having nsImageLoadingContent::CloneImageForPrint replace the image request with a snapshot of the current frame of the image?

It would be really nice if we could just remove nsPresContext::IsDynamic completely...

I'm not sure what the changes in nsVideoFrame.cpp are...

-        po->mContent  = aContent;

Where did this go?

I have some great ideas about improving the printing of selection by removing everything but the selected content from the cloned doc --- which would let us get rid of the "print selection" hacks we currently have in layout --- but that can wait I guess.
Comment 36 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 02:31:04 PDT
(In reply to comment #35)
> mCloningForPrint is kind of nasty, why can't we have a CloneNodeInternal which
> takes it as a parameter? Or do the "if (mCloningForPrint)" stuff in
> CloneForPrint after we've cloned the document?
I don't like mCloningForPrint either, but it was the simplest way to achieve whatever I needed.
It is needed that some things happens before child nodes of the document are cloned. I still wanted to use the normal cloneNode method, so that
I get all the functionality nsNodeUtils::CloneAndAdopt provides.

> 
> +   * Returns the nsIObjectFrame object which should be used when
> +   * printing the plugin.
> +   */
> +  [noscript] nsIFrame getPrintFrame();
> 
> It's not clear from the comment what this means or why you'd use it.
OK, will improve the comment.
The idea is the same what we have now - use the original plugin instance
for printing.

> Is there a reason we don't do the stylesheet cloning in
> nsIDocument::CloneForPrint for all cloned documents?
Why should we clone stylesheets in normal case?

> Similar question for nsHTMLOptionElement and nsHTMLTextAreaElement, can't we
> just always clone the selection and the value? What about the input values for
> other input types?
Input elements do clone the values.
These are question not strictly related to this bug, but to related to
generic "what should be cloned when a node is cloned".

> Can you write a test for the ID-registration in nsNodeUtils::CloneAndAdopt? I
> guess that's an existing bug that script could encounter.
That change is part of Bug 514487, and the patch contains few tests for it.
 
> This way of handling plugins isn't ideal --- I really do think we should
> snapshot the plugin's current pixels if we can --- but I guess this is good
> enough for now.
It spent quite a few days trying to figure out how to get a reasonable
snapshot (never got it quite right), and even then I wasn't sure if that
is the right thing - mainly because of comment 28. 

> +        test_bug514487.html \
> 
> Looks like this file is missing?
It is there

> nsPresContext::ResetPrintImageAnimation is yucky. Could we avoid this by having
> nsImageLoadingContent::CloneImageForPrint replace the image request with a
> snapshot of the current frame of the image?
The problem is that there are lots of other images than just <img>.
All the borders and background etc and all those can be animated.
At some point I did take the snapshot in CloneImageForPrint, but making
that work nicely with other images was getting quite ugly.

> I'm not sure what the changes in nsVideoFrame.cpp are...
Hmm, windows newlines or something. I'll check that.

> -        po->mContent  = aContent;
> 
> Where did this go?
nsPrintObject::Init
Comment 37 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-09 02:59:15 PDT
(In reply to comment #36)
> (In reply to comment #35)
> > mCloningForPrint is kind of nasty, why can't we have a CloneNodeInternal which
> > takes it as a parameter? Or do the "if (mCloningForPrint)" stuff in
> > CloneForPrint after we've cloned the document?
> I don't like mCloningForPrint either, but it was the simplest way to achieve
> whatever I needed.
> It is needed that some things happens before child nodes of the document are
> cloned. I still wanted to use the normal cloneNode method, so that
> I get all the functionality nsNodeUtils::CloneAndAdopt provides.

Why can't you pass 'aCloningForPrint' down through nsNodeUtils::Clone and nsNodeUtils::CloneAndAdopt?

> > Is there a reason we don't do the stylesheet cloning in
> > nsIDocument::CloneForPrint for all cloned documents?
> Why should we clone stylesheets in normal case?

It just seems to me that generally the clone should be as much like the original as possible.

> > Similar question for nsHTMLOptionElement and nsHTMLTextAreaElement, can't we
> > just always clone the selection and the value? What about the input values for
> > other input types?
> Input elements do clone the values.
> These are question not strictly related to this bug, but to related to
> generic "what should be cloned when a node is cloned".

Same as above. I guess this is a spec question.

> It spent quite a few days trying to figure out how to get a reasonable
> snapshot (never got it quite right), and even then I wasn't sure if that
> is the right thing - mainly because of comment 28.

I think we could actually fix that, at least on Windows, by having plugins "print" on Windows to an enhanced metafile which we can then play back later. But let's deal with that elsewhere.

> > nsPresContext::ResetPrintImageAnimation is yucky. Could we avoid this by having
> > nsImageLoadingContent::CloneImageForPrint replace the image request with a
> > snapshot of the current frame of the image?
> The problem is that there are lots of other images than just <img>.
> All the borders and background etc and all those can be animated.
> At some point I did take the snapshot in CloneImageForPrint, but making
> that work nicely with other images was getting quite ugly.

I see. Hmm, that is hard. I guess one problem is that matching images in styles in the print presentation to images in styles in the main presentation is difficult, and maybe there isn't even a match due to media style rules.

Right now you're actually showing the first frame of all style images, right? Let's suppose we carry on with that, so we only show the "current frame" for nsImageLoadingContent images. Why then do we need to manage style images via mPrintImages and gPrintImageContainers? Wouldn't it be enough to snapshot nsImageLoadingContent images and load all other images normally?
Comment 38 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-09 03:03:14 PDT
(In reply to comment #37)
> Right now you're actually showing the first frame of all style images, right?

As usual I'm confused. Correct me if I'm wrong again, but what's actually happening is that the style system's request for an image gets mapped to the existing image in the main presentation, and you're temporarily pausing the animation of that image, right? So we display the right frame of the image in the print presentation, but we stop the image animating in the main presentation...

Then it seems we could fix this by adding code where we construct image requests. We want to be able to specify a flag which means "if there is an existing image for this URI, take a snapshot of its current frame and give me that snapshot instead of the real image" ... and pass that flag whenever we construct an image request for a print document.
Comment 39 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 03:31:19 PDT
(In reply to comment #38)
> So we display the right frame of the image in
> the print presentation, but we stop the image animating in the main
> presentation...

Right. So basically keeping our current behavior (with bug fixes).
Comment 40 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-09 03:38:23 PDT
True, but the code is ugly and the behaviour is going to be more surprising than before since now the document can be running normally except that its images (perhaps only some of its images) are not animated.
Comment 41 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 03:50:28 PDT
Well even now if you have same page loaded or same images in two windows,
the animations in both windows are stopped.
Comment 42 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 03:51:13 PDT
...but sure, cloning images for printing would be the better.
Comment 43 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 09:06:13 PDT
I was hoping that keeping the current behavior with plugins and images would be
enough for now.
Comment 44 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-09 13:09:20 PDT
Plugins yes, because we're not adding any significant new code for them. But for images you're adding these global arrays and mPrintImages...
Comment 45 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 13:16:38 PDT
(In reply to comment #44)
>  But
> for images you're adding these global arrays and mPrintImages...
because I want to fix the current problems we have.
We may start animating images in print preview sort of randomly, although
painting them depends on whether they are invalidated.
Comment 46 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 13:17:27 PDT
The patch does trigger those problems more often than what happens currently, but
the problems are there even now.
Comment 47 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-09 16:06:54 PDT
If we snapshotted images for !IsDynamic presentations, wouldn't that fix the current problems too?
Comment 48 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 16:17:32 PDT
(In reply to comment #47)
> If we snapshotted images for !IsDynamic presentations, wouldn't that fix the
> current problems too?
Yeah, that is what I'd like to do, sure, but fixing the current animation handling
was just significantly easier, at least for me.
No need to change layout/style or anything in like that.

Certainly something to fix, but perhaps in a followup.
Though, if wanted, I could hack that change to this bug too.
Comment 49 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 16:20:18 PDT
I'll try something a bit different what I thought earlier.
I may need to push some bit up to imgloader.
Comment 50 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-09 16:58:14 PDT
(In reply to comment #49)
> I'll try something a bit different what I thought earlier.
> I may need to push some bit up to imgloader.
Ok, the idea I had wouldn't work.
Need something else. Have to hack stylesheet cloning to get that right.
Comment 51 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-10 06:31:55 PDT
(In reply to comment #37)
> Why can't you pass 'aCloningForPrint' down through nsNodeUtils::Clone and
> nsNodeUtils::CloneAndAdopt?
That doesn't quite work. nsNodeUtils::CloneAndAdopt relies on DOM's Clone method
and I can't add new parameters to that. So I think the current setup is
(unfortunately) the simplest way to handle this.
Comment 52 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-10 15:57:30 PDT
(In reply to comment #51)
> That doesn't quite work. nsNodeUtils::CloneAndAdopt relies on DOM's Clone
> method
> and I can't add new parameters to that. So I think the current setup is
> (unfortunately) the simplest way to handle this.

Oh well...

> (In reply to comment #47)
> Certainly something to fix, but perhaps in a followup.
> Though, if wanted, I could hack that change to this bug too.

I just don't like adding new code and later taking it out again.

How about in this patch we don't try to handle image animation at all, then you do another patch on top of this patch to snapshot images, and then we land them together?
Comment 53 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-11 03:11:57 PDT
(In reply to comment #52)
> How about in this patch we don't try to handle image animation at all, then you
> do another patch on top of this patch to snapshot images, and then we land them
> together?
Well, I don't want to land something which breaks things badly (as I said the image animation problem is there already now, but the patch makes it even
more visible).
So I guess I need to change stylesheet cloning or something to take
snapshots. And do that in this bug.
Comment 54 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-11 14:48:16 PDT
OK, but it can still be a separate patch, which would be helpful.

One thing that might be a good idea is have the code use the term "CloneStatic" or "CloneStaticSnapshot" instead of "CloneForPrint", since that's more precisely what we're doing and might be useful for other things.
Comment 55 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-16 11:04:15 PDT
(In reply to comment #54)
> OK, but it can still be a separate patch, which would be helpful.
So you want to remove the imageanimation handling in this bug or in a followup?


> One thing that might be a good idea is have the code use the term "CloneStatic"
> or "CloneStaticSnapshot" instead of "CloneForPrint", since that's more
> precisely what we're doing and might be useful for other things.
OK
Comment 56 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-16 12:50:41 PDT
Created attachment 401078 [details] [diff] [review]
v19

Just small changes in this one.
Comment 57 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-16 14:57:09 PDT
(In reply to comment #55)
> (In reply to comment #54)
> > OK, but it can still be a separate patch, which would be helpful.
> So you want to remove the imageanimation handling in this bug or in a followup?

I think: take the image animation handling out of this patch. Implement image snapshotting in an additional patch, in this bug.
Comment 58 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-16 15:24:24 PDT
... so I guess I'll take out the image animation and update the patch to whatever the solution will be.
stylesheet cloning isn't too helpful here unfortunately.
Comment 59 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-16 15:31:58 PDT
At style resolution time for the print document, when we create the imgRequest objects, can't we test if the document is static and if it is, and we already have an image frame (because we got a reference to an existing image), snapshot at that point?
Comment 60 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-16 15:38:58 PDT
Ah, perhaps at a bit later than what I thought. Stylesheet would still use the original image request.
Comment 61 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-16 15:39:47 PDT
But I still wonder who keeps the image request alive...
Comment 62 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-16 15:42:31 PDT
Or I guess, basically how to pass the context info down to nsStyleImage
Comment 63 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-17 03:28:51 PDT
Created attachment 401186 [details]
anim image tests
Comment 64 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-17 04:21:47 PDT
Created attachment 401193 [details] [diff] [review]
+animated image cloning

This clones the current frame of animated images.
Comment 65 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-17 19:09:57 PDT
+  PRPackedBool mIsStaticDocument;
+
+  PRPackedBool mCloningForStatic;
+
+  nsCOMPtr<nsIDocument> mOriginalDocument;

Add comments here

+  // Make document to use different container during cloning.

"Make document use"

+
+  } else if (mDecoder)
     mDecoder->Paint(aContext, aFilter, aRect);

Lose blank line and add {}

+      if (frame && frame->GetType() == nsGkAtoms::HTMLVideoFrame &&
+          static_cast<nsVideoFrame*>(frame)->ShouldDisplayPoster()) {
+        elem = do_QueryInterface(static_cast<nsVideoFrame*>(frame)->
+                                 GetPosterImage());
+      } else {
+        elem = do_QueryInterface(const_cast<nsHTMLMediaElement*>(this));
+      }
+
+      nsLayoutUtils::SurfaceFromElementResult res =
+        nsLayoutUtils::SurfaceFromElement(elem,
+                                          nsLayoutUtils::SFE_WANT_NEW_SURFACE);

This happens to have the effect that in a print presentation, the poster image will be scaled to preserve aspect ratio. (Whereas currently the poster image is scaled to fill the frame.) But that's OK because we really want the former behaviour anyway, and we'll make that happen in bug 517363.
Comment 66 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-17 20:20:39 PDT
Comment on attachment 401193 [details] [diff] [review]
+animated image cloning

+  if (GetIsPrintPreview() ||
+      (mDocument && mDocument->IsStaticDocument()))

How can we be in print preview with a non-static document?

+#define NS_SET_IMAGE_REQUEST(method_, context_, request_)                   \
+  if (context_->PresContext()->IsDynamic()) {                               \
+    method_(request_);                                                      \
+  } else {                                                                  \
+    nsCOMPtr<imgIRequest> req = nsContentUtils::GetStaticRequest(request_); \
+    method_(req);                                                           \
+  }

Why not just have a helper function
  imgIRequest* MakeStaticIfNeeded(nsStyleContext* aContext, imgIRequest* aRequest);
?

dbaron should review the style system changes. Someone else will have to review the content changes too (jst?) and someone else again will have to review the imglib changes (joe?).

I like this very much. Thank you.

I wonder if we can simplify the printing of subdocuments in the future so we don't need a tree of nsPrintObjects. Something to think about.

On trunk we have a bug where printing doesn't work properly with @font-face font loading --- bug 468568. This will help us fix that bug because we'll be able to reconstruct frames for a print document.
Comment 67 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-18 01:45:47 PDT
(In reply to comment #66)
> Why not just have a helper function
>   imgIRequest* MakeStaticIfNeeded(nsStyleContext* aContext, imgIRequest*
> aRequest);
> ?
Because using the macro I can easily have the nsCOMPtr to keep the already_addrefed object alive while setting the value.

> (From update of attachment 401193 [details] [diff] [review])
> +  if (GetIsPrintPreview() ||
> +      (mDocument && mDocument->IsStaticDocument()))
> 
> How can we be in print preview with a non-static document?
I'll check this, but IIRC this might happen just *before* we enter print preview.
Comment 68 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-18 05:44:12 PDT
Created attachment 401420 [details] [diff] [review]
v20.1
Comment 69 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-18 05:46:05 PDT
Comment on attachment 401420 [details] [diff] [review]
v20.1

jst, could you look at the content changes?
dbaron, could you check the style handling changes?
Bholley, this has the dummy request we talked about on IRC.
Comment 70 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-18 13:02:26 PDT
Seems like I need to change 
+    img->GetAnimated(&animated);
+    if (!animated) {
+      NS_ADDREF(*aReturn = aRequest);
+      return NS_OK;
+    }
to
    nsresult rv = nsimg->GetAnimated(&animated);
    if (NS_SUCCEED(rv) && !animated) {
      NS_ADDREF(*aReturn = aRequest);
      return NS_OK;
    }

once bug 517543 is fixed.
Comment 71 User image Bobby Holley (:bholley) (busy with Stylo) 2009-09-18 22:23:30 PDT
I'm liking the imgDummyRequest name less and less, but I don't really have any other good ideas. Joe - thoughts?

+  nsCOMPtr<imgIContainer> img, currentFrame;
+  aRequest->GetImage(getter_AddRefs(img));
+  if (img) {
+    PRBool animated = PR_FALSE;
+    img->GetAnimated(&animated);
+    if (!animated) {
+      NS_ADDREF(*aReturn = aRequest);
+      return NS_OK;
+    }

Per our discussion on IRC, you can't rely on this as is without bug 517543.

+  // Lock dummy requests for now, since they can't trigger onDiscard.
+  req->LockImage();

This isn't necessary assuming the conditional approach. Assuming that the image was indeed animated, extracting the frame definitely creates a new imgContainer, and that imgContainer has no source data (so it can't discard).

+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip2(aObserver);
why "2"?

+      aObserver->OnStartRequest(req);

OnStartRequest should fire unconditionally.

+    if (req->mImageStatus & STATUS_LOAD_COMPLETE) {
+      aObserver->OnStartDecode(req);
+      aObserver->OnStartFrame(req, 0);
+      nsIntRect r;
+      mImage->GetCurrentFrameRect(r);
+      aObserver->OnDataAvailable(req, 0, &r);
+      aObserver->OnStopFrame(req, 0);
+    }
+    if (req->mImageStatus & STATUS_FRAME_COMPLETE) {
+      aObserver->OnStopContainer(req, req->mImage);
+      aObserver->OnStopDecode(req, NS_OK, nsnull);
+      aObserver->OnStopRequest(req, PR_TRUE);
+    }

These don't look right to me. See the IDL docs in imgIDecoderObserver for the distinction between load notifications and decode notifications (note that, at present, OnStopDecode should be treated as a load notification and should be fired right before OnStopRequest). OnStartFrame, OnDataAvailable, and OnStopFrame should be conditional on STATUS_FRAME_COMPLETE, and OnStopContainer (pooor man's OnStopDecode for the moment) should be contingent on STATUS_DECODE_COMPLETE (introduced in bug 517543).

+NS_IMETHODIMP
+imgDummyRequest::RequestDecode()
+{
+  return mImage ? mImage->RequestDecode() : NS_OK;
+}
+NS_IMETHODIMP
+imgDummyRequest::LockImage()
+{
+  if (mImage) {
+    ++mLocksHeld;
+    return mImage->LockImage();
+  }
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+imgDummyRequest::UnlockImage()
+{
+  if (mImage) {
+    NS_ABORT_IF_FALSE(mLocksHeld > 0, "calling unlock but no locks!");
+    --mLocksHeld;
+    return mImage->UnlockImage();
+  }
+  return NS_OK;
+}
+
+imgDummyRequest::~imgDummyRequest()
+{
+  if (mImage) {
+    while (mLocksHeld) {
+      UnlockImage();
+    }
+  }
+}

I don't like the mImage checks here. Can't we just mandate that mImage is non-null and assert it in the constructor?
Comment 72 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-19 01:32:05 PDT
(In reply to comment #71)
> +    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip2(aObserver);
> why "2"?
Leftover

> 
> +      aObserver->OnStartRequest(req);
> 
> OnStartRequest should fire unconditionally.
ok.


> 
> +    if (req->mImageStatus & STATUS_LOAD_COMPLETE) {
> +      aObserver->OnStartDecode(req);
> +      aObserver->OnStartFrame(req, 0);
> +      nsIntRect r;
> +      mImage->GetCurrentFrameRect(r);
> +      aObserver->OnDataAvailable(req, 0, &r);
> +      aObserver->OnStopFrame(req, 0);
> +    }
> +    if (req->mImageStatus & STATUS_FRAME_COMPLETE) {
> +      aObserver->OnStopContainer(req, req->mImage);
> +      aObserver->OnStopDecode(req, NS_OK, nsnull);
> +      aObserver->OnStopRequest(req, PR_TRUE);
> +    }
> 
> These don't look right to me. See the IDL docs in imgIDecoderObserver for the
> distinction between load notifications and decode notifications (note that, at
> present, OnStopDecode should be treated as a load notification and should be
> fired right before OnStopRequest).
imgIDecoderObserver doesn't explain these too well. I was *trying* to do whatever imgRequest does :)

> (pooor man's OnStopDecode for the moment) should be contingent on
> STATUS_DECODE_COMPLETE (introduced in bug 517543).
OK

> I don't like the mImage checks here.
Why not?

> Can't we just mandate that mImage is
> non-null and assert it in the constructor?
How can I mandate that. I need a request which will never point to an
animated image. If a request is cloned to a dummy request before
imgRequest has mImage, I still need something to return. And that something
shouldn't be imgRequestProxy, since imgRequest may get the mImage once
OnDataAvailable is called. So I'd rather return a dummy request without mImage.
Comment 73 User image Bobby Holley (:bholley) (busy with Stylo) 2009-09-19 13:11:10 PDT
(In reply to comment #72)
> > I don't like the mImage checks here.
> Why not?
> 
> > Can't we just mandate that mImage is
> > non-null and assert it in the constructor?
> How can I mandate that. I need a request which will never point to an
> animated image. If a request is cloned to a dummy request before
> imgRequest has mImage, I still need something to return. And that something
> shouldn't be imgRequestProxy, since imgRequest may get the mImage once
> OnDataAvailable is called. So I'd rather return a dummy request without mImage.

My general gripe here (and following) is that imgIRequests are supposed to be dynamic. They may not have everything _now_, but they'll probably get it sometime soon. imgIRequest guarantees, for example, that locks applied now will be deferred and applied later if the image doesn't exist yet. But in this case, the image is never going to exist.

Nonetheless, I don't really see a way around it. To mitigate the issues, and make things as close to consistent as possible, you assert in the constructor that _either_ mImage is null _or_ the number of frames is exactly equal to 1, and then set up state and notifications this way:

OnStartRequest
if (!mImage) {
  OnStopDecode(error);
  OnStopRequest();
  mImageStatus = STATUS_ERROR;
  return;
}
mImageStatus = 0;
if (SIZE_AVAILABLE) {
  mImageStatus |= STATUS_SIZE_AVAILABLE;
  OnStartContainer()
}
OnStartDecode()
OnStartFrame()
OnDataAvailable()
mImageStatus |= STATUS_FRAME_COMPLETE
OnStopFrame()
mImage |= STATUS_DECODE_COMPLETE
OnStopContainer()
mImage |= STATUS_LOAD_COMPLETE
OnStopDecode()
OnStopRequest()
 
And of course, joe's going to need to take a look at this as well.
Comment 74 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-09-20 11:10:29 PDT
(In reply to comment #73)
> My general gripe here (and following) is that imgIRequests are supposed to be
> dynamic. 
But what I need is static request :)

> imgIRequest guarantees, for example, that locks applied now will
> be deferred and applied later if the image doesn't exist yet. But in this case,
> the image is never going to exist.
Well, nothing guarantees that normal imgRequest gets the mImage either. 

> assert in the constructor
> that _either_ mImage is null _or_ the number of frames is exactly equal to 1
Makes sense.

> and then set up state and notifications this way:
You mean in Clone()?
Why do I need to change image status between OnXXX calls?
That is not what happens when normal imgIRequest is cloned.
Comment 75 User image Bobby Holley (:bholley) (busy with Stylo) 2009-09-20 11:30:49 PDT
(In reply to comment #74)
> (In reply to comment #73)

> You mean in Clone()?
> Why do I need to change image status between OnXXX calls?
> That is not what happens when normal imgIRequest is cloned.

You don't really. I just wrote it that way because the notifications sent from Clone() are supposed to be a simulation of all the steps that happened on the request before the clone occurred. Feel free to set them all at the beginning, so long as the effect is the same.
Comment 76 User image Johnny Stenback (:jst, jst@mozilla.com) 2009-10-08 15:17:20 PDT
Comment on attachment 401420 [details] [diff] [review]
v20.1

+  virtual already_AddRefed<nsIDocument>
+  CloneForStatic(nsISupports* aCloneContainer);

I'd suggest we name these Clone*ForStatic() methods something like CreateStatic*Clone instead, throughout the patch.

r=jst with that and others' comments addressed.
Comment 77 User image David Baron :dbaron: ⌚️UTC-8 2009-11-12 14:03:44 PST
Comment on attachment 401420 [details] [diff] [review]
v20.1

nsRuleNode.cpp:

+#define NS_SET_IMAGE_REQUEST(method_, context_, request_)                   \
+  if (context_->PresContext()->IsDynamic()) {                               \

Parenthesize:  (context_) rather than context_

nsStyleStruct.h:

>-
>+private:
>   nsCOMPtr<imgIRequest> mBorderImage; // [reset]
> 
>+protected:

Just make the whole section private; nothing derives from this.

>+  void SetListStyleImage(imgIRequest* aReq);

Can the setter be inline, or does that introduce header dependency problems?

+  void SetImage(imgIRequest* aRequest);

Same here.

r=dbaron on the layout/style changes with that fixed.  Sorry for the delay.
Comment 78 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-11-13 10:32:15 PST
(In reply to comment #77)
> >+  void SetListStyleImage(imgIRequest* aReq);
> 
> Can the setter be inline, or does that introduce header dependency problems?
IIRC, header dependency problems were the reason for not inlining, but I'll retest.
Comment 79 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-11-13 11:44:58 PST
Created attachment 412265 [details] [diff] [review]
v21

Joe, could you review the libpr0n/ parts.
I changed imgDummyRequest::Clone based on comment 73.
Comment 80 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-11-13 14:29:43 PST
Created attachment 412302 [details] [diff] [review]
v22

1) DummyRequest->ContainerRequest
2) imgITools::getStaticRequest->imgIRequest::getStaticRequest
3) Use similar code for imgContainerRequest::Clone as what imgRequest::NotifyProxyListener has.
I'm not too happy with copying that code, but it seemed like the simplest
way to keep notification handling similar in both cases.
And fortunately it isn't too many lines of code.
Comment 81 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-11-13 15:59:31 PST
Created attachment 412328 [details] [diff] [review]
v23

There was canvas painting missing.

But still, joe, please review the imglib part.
Comment 82 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-11-14 14:15:07 PST
Created attachment 412430 [details] [diff] [review]
v24

This has simpler canvas handling, but webgl handling is broken because of
Bug 528746.
Comment 83 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-11-14 14:16:45 PST
Comment on attachment 412430 [details] [diff] [review]
v24

Roc, does the <canvas> handling look ok to you?
Look for nsHTMLCanvasElement::CopyInnerTo
Comment 84 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-15 13:31:49 PST
Comment on attachment 412430 [details] [diff] [review]
v24

Yes, that looks fine.

What are we going to do for tests here?
Comment 85 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-15 13:39:18 PST
It would be nice to have some tests that make static clones of documents with different kinds of content and then use WindowSnapshot to make sure they look the same. And we should also be able to use WindowSnapshot to take a snapshot of the original documents, mutate them, and then take a snapshot of the static document to verify that it really is static.
Comment 86 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-11-17 04:45:36 PST
Created attachment 412829 [details] [diff] [review]
v24.1

Adds a test for 2d canvas print preview.
Comment 87 User image Joe Drew (not getting mail) 2009-11-17 13:45:10 PST
Comment on attachment 412829 [details] [diff] [review]
v24.1

>diff --git a/content/base/public/nsContentUtils.h b/content/base/public/nsContentUtils.h
>+class imgITools;

>+  static imgITools* sImgTools;

>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp

>+#include "imgITools.h"

>+imgITools* nsContentUtils::sImgTools = nsnull;

>+  rv = CallCreateInstance("@mozilla.org/image/tools;1", &sImgTools);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

>+  NS_IF_RELEASE(sImgTools);

should all be removed since we're not using imgITools any more.

>diff --git a/modules/libpr0n/src/Makefile.in b/modules/libpr0n/src/Makefile.in
> CPPSRCS		= \
> 			imgContainer.cpp \
> 			imgFrame.cpp \
> 			imgLoader.cpp    \
> 			imgRequest.cpp   \
> 			imgRequestProxy.cpp \
>-			imgTools.cpp
>+			imgTools.cpp \
>+			imgContainerRequest.cpp

While you're in here, will you change this to add a $(NULL) so this makefile's like every other Mozilla makefile?

>diff --git a/modules/libpr0n/src/imgRequestProxy.cpp b/modules/libpr0n/src/imgRequestProxy.cpp

>+NS_IMETHODIMP
>+imgRequestProxy::GetStaticRequest(imgIRequest** aReturn)
>+{
>+  *aReturn = nsnull;
>+  nsCOMPtr<imgIContainer> img, currentFrame;
>+  GetImage(getter_AddRefs(img));
>+  if (img) {
>+    PRBool animated = PR_FALSE;
>+    nsresult rv = img->GetAnimated(&animated);
>+    if (NS_SUCCEEDED(rv) && !animated) {
>+      NS_ADDREF(*aReturn = this);
>+      return NS_OK;
>+    }
>+
>+    PRInt32 w = 0;
>+    PRInt32 h = 0;
>+    img->GetWidth(&w);
>+    img->GetHeight(&h);
>+    nsIntRect rect(0, 0, w, h);
>+    img->ExtractFrame(imgIContainer::FRAME_CURRENT, rect,
>+                      imgIContainer::FLAG_SYNC_DECODE,
>+                      getter_AddRefs(currentFrame));
>+  }
>+
>+  nsCOMPtr<nsIURI> uri;
>+  GetURI(getter_AddRefs(uri));
>+  PRUint32 imageStatus = 0;
>+  GetImageStatus(&imageStatus);
>+  nsCOMPtr<nsIPrincipal> principal;
>+  GetImagePrincipal(getter_AddRefs(principal));
>+
>+  imgContainerRequest* req =
>+    new imgContainerRequest(currentFrame, uri, imageStatus,
>+                            mOwner ? mOwner->GetState() : 0,
>+                            principal);
>+  NS_ENSURE_TRUE(req, NS_ERROR_OUT_OF_MEMORY);

Shouldn't use NS_ENSURE_TRUE - it's not worthy of an NS_WARNING that we're out of memory, we should just handle it.

>+  NS_ADDREF(*aReturn = req);
>+  return NS_OK;
>+}

It isn't valid for an imgContainerRequest to be created with a null currentFrame (at least in Clone() we unconditionally dereference its frame), so I'd rather we early-exit if we have no image.

>+
>diff --git a/modules/libpr0n/src/imgTools.cpp b/modules/libpr0n/src/imgTools.cpp
>--- a/modules/libpr0n/src/imgTools.cpp
>+++ b/modules/libpr0n/src/imgTools.cpp
>@@ -215,8 +215,9 @@ NS_IMETHODIMP imgTools::EncodeScaledImag
>   rv = encoder->InitFromData(bitmapData, bitmapDataLength,
>                              aScaledWidth, aScaledHeight, strideSize,
>                              imgIEncoder::INPUT_FORMAT_HOSTARGB, EmptyString());
> 
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return CallQueryInterface(encoder, aStream);
> }
>+

Left-over, unrelated change.

>+imgContainerRequest::imgContainerRequest(imgIContainer* aImage,
>+#ifdef DEBUG
>+  PRUint32 numFrames = 0;
>+  if (aImage) {
>+    aImage->GetNumFrames(&numFrames);
>+  }
>+  NS_ASSERTION(!aImage || numFrames == 1,
>+               "Shouldn't have image with more than one frame!");

NS_ABORT_IF_FALSE

>+NS_IMETHODIMP
>+imgContainerRequest::Clone(imgIDecoderObserver* aObserver, imgIRequest** _retval)
>+{
>+  imgContainerRequest* req =
>+    new imgContainerRequest(mImage, mURI, mImageStatus, mState, mPrincipal);
>+  NS_ENSURE_TRUE(req, NS_ERROR_OUT_OF_MEMORY);

Don't use NS_ENSURE_TRUE for OOM.
Comment 88 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-11-17 14:52:37 PST
Created attachment 412946 [details] [diff] [review]
v25

Comments addressed except "I'd rather we early-exit if we have no image"
I really want to get a dummy request whenever possible. That way I can
use the dummy request and not the real one, but yet have something else
than just null.
And imgContainerRequest::Clone does have null checks.
Comment 89 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-12-11 09:20:35 PST
Created attachment 417119 [details] [diff] [review]
v29

I pushed this patch.
Comment 90 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-12-11 10:32:56 PST
http://hg.mozilla.org/mozilla-central/rev/180966423a3c
Comment 91 User image Daniel Holbert [:dholbert] 2009-12-13 10:03:52 PST
Should this be marked FIXED?
Comment 92 User image Phil Lacy 2010-01-25 08:01:52 PST
Bug 535343
Comment 93 User image neil@parkwaycc.co.uk 2010-02-01 05:52:35 PST
Comment on attachment 417119 [details] [diff] [review]
v29

>+        this._printPreviewTab = tabbrowser.loadOneTab("about:blank", null, null,
>+                                                      null, true, false);
Sadly most print preview browsers do not implement loadOneTab. These include
* View source
* SeaMonkey
* Thunderbird
Can you easily remove the dependency or do you need to back this out?
Comment 94 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-02-01 07:11:07 PST
This won't be backed out, for sure.

I'll fix the toolkit/ mess. Sorry about that.
Fortunately the code changes there are quite small.
Comment 95 User image neil@parkwaycc.co.uk 2010-02-03 08:28:30 PST
Comment on attachment 417119 [details] [diff] [review]
v29

>+        this._printPreviewTab = tabbrowser.loadOneTab("about:blank", null, null,
>+                                                      null, true, false);
In fact, why bother with loadOneTab(...) when addTab() will do?
Comment 96 User image Philip Chee 2010-02-03 10:02:55 PST
> In fact, why bother with loadOneTab(...) when addTab() will do?
Thunderbird's addTab() implementation is different. I think you need to specify a contentTab as a parameter.
Comment 97 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-02-03 10:05:59 PST
Please don't add comments here, but to the relevant bugs. Thanks.
Comment 98 User image Yun Guo 2012-06-13 03:33:23 PDT
Hello Olli, 

Do you know how to printPreview a page in an embedded xulrunner application with your this patch? Or there's some guide for it? 

I can print preview a page in my xulrunner embed app with below snippet code before(xulrunner1.9.2): nsIWebBrowserPrint::PrintPreview(globalPrintSettings, null, null); But after apply your patch(xulrunner2.0), it not work, since the second parameter of nsIWebBrowserPrint::PrintPreview() should not be null which ruled in this patch. I'm tried to pass the current browser window to it, but not work, an blank page displayed. Could you help me on it? Thank you.
Comment 99 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-06-13 05:22:39 PDT
You could open a new "tab" or "window" (whatever your xulrunner app has) or just any xul:browser
or html:iframe works too and do something similar to 
http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/chrome/printpreview_helper.xul#26
or
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js

Basically you open a new browsing context and clone/printpreview the original document there.
Comment 100 User image Yun Guo 2012-06-14 03:22:51 PDT
Many thanks for your comments. I created a new browser for print preview, get the nsIWebBrowserPrint instance from the new created browser, and pass the original document window to PrintPreview() method, but there still display a blank window. Need I manually clone the original content to print preview window? 

Below is my code, is there some wrong in it? I read the code in printpreview_helper.xul carefully, seems my code flow same as it. Not sure why it not work. I debugged, each sentence code executed succeed, not failure. Could you help give some comments on it? 

Shell shell = new Shell();
shell.setLayout(new FillLayout());
// create a new browser for print preview
DOMBrowser browser = new DOMBrowser(shell, SWT.NULL);
shell.open();
        		
int[] address = new int[1];
nsIWebBrowser newWb = browser.getWebBrowser(); 
                
int rc = newWb.QueryInterface(nsIInterfaceRequestor.NS_IINTERFACEREQUESTOR_IID, address);
nsIInterfaceRequestor req = new nsIInterfaceRequestor(address[0]);               
rc = req.GetInterface(nsIDocShell.NS_IDOCSHELL_IID,address);
req.Release(); 
        		
nsIDocShell docShell = new nsIDocShell(address[0]);
docShell.GetPrintPreview(address);
// get the nsIWebBrowserPrint from new created browser             
printMoz = new nsIWebBrowserPrint(address[0]); 
    		
nsIWebBrowser wb = this.getWebBrowser();
int[] address2=new int[1];
rc = wb.GetContentDOMWindow(address); // get current document's nsIDOMWindow 
 
int[] printSettings = new int[1];
printMoz.GetGlobalPrintSettings(printSettings);
rc = printMoz.PrintPreview(printSettings[0], address2[0], 0); // printpreview it
Comment 101 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-06-14 04:01:19 PDT
(In reply to Yun Guo from comment #100)
> Many thanks for your comments. I created a new browser for print preview,
> get the nsIWebBrowserPrint instance from the new created browser, and pass
> the original document window to PrintPreview() method, but there still
> display a blank window. Need I manually clone the original content to print
> preview window? 
No
        		
> nsIDocShell docShell = new nsIDocShell(address[0]);
What does this do? Is the docshell attached to somewhere?
Comment 102 User image Yun Guo 2012-06-14 23:12:56 PDT
(In reply to Olli Pettay [:smaug] from comment #101)         		
> > nsIDocShell docShell = new nsIDocShell(address[0]);
> What does this do? Is the docshell attached to somewhere?
This line code is to get the new created browser's nsIDocShell, I want to from which to get the nsIWebBrowserPrint interface. No, this docshell is not attached to other place. 

Today, I tried below code which is query the nsIWebBrowserPrint interface directly from new browser, but still can't display the content in print preview window, still a blank window. Seems the printPreview method not work in embed mode or something wrong in my code? 

Shell shell = new Shell();
shell.setLayout(new FillLayout());
// create a new browser for print preview
DOMBrowser browser = new DOMBrowser(shell, SWT.NULL);
shell.open();
        		
int[] address = new int[1];
nsIWebBrowser newWb = browser.getWebBrowser(); 
                
int rc = newWb.QueryInterface(nsIInterfaceRequestor.NS_IINTERFACEREQUESTOR_IID, address);
nsIInterfaceRequestor req = new nsIInterfaceRequestor(address[0]);            
// get the nsIWebBrowserPrint from new created browser 
rc = req.GetInterface(nsIWebBrowserPrint.NS_IWEBBROWSERPRINT_IID,address);
req.Release();         		           
printMoz = new nsIWebBrowserPrint(address[0]); 
    		
nsIWebBrowser wb = this.getWebBrowser();
int[] address2=new int[1];
rc = wb.GetContentDOMWindow(address); // get current document's nsIDOMWindow 
 
int[] printSettings = new int[1];
printMoz.GetGlobalPrintSettings(printSettings);
rc = printMoz.PrintPreview(printSettings[0], address2[0], 0); // printpreview it
Comment 103 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-06-15 01:33:39 PDT
(In reply to Yun Guo from comment #102)

> Shell shell = new Shell();
> shell.setLayout(new FillLayout());
> // create a new browser for print preview
> DOMBrowser browser = new DOMBrowser(shell, SWT.NULL);
> shell.open();
>         		
> int[] address = new int[1];
> nsIWebBrowser newWb = browser.getWebBrowser(); 
>                 
> int rc =
> newWb.QueryInterface(nsIInterfaceRequestor.NS_IINTERFACEREQUESTOR_IID,
> address);
> nsIInterfaceRequestor req = new nsIInterfaceRequestor(address[0]);          
> 
> // get the nsIWebBrowserPrint from new created browser 
> rc = req.GetInterface(nsIWebBrowserPrint.NS_IWEBBROWSERPRINT_IID,address);
> req.Release();         		           
> printMoz = new nsIWebBrowserPrint(address[0]); 
>     		
> nsIWebBrowser wb = this.getWebBrowser();
> int[] address2=new int[1];
> rc = wb.GetContentDOMWindow(address); // get current document's nsIDOMWindow 
>  
> int[] printSettings = new int[1];
> printMoz.GetGlobalPrintSettings(printSettings);
> rc = printMoz.PrintPreview(printSettings[0], address2[0], 0);
Why you pass 'address2'? it doesn't seem to point to anywhere.
Perhaps you mean to store the address of current window in it, but
you're actually using 'address'


We should move this discussion to email.
Comment 104 User image Yun Guo 2012-06-15 02:14:02 PDT
Sorry, there's a typo mistake in line: "rc = wb.GetContentDOMWindow(address);" when I write that comments. But it's correct in my project.
Comment 105 User image Yun Guo 2012-06-15 02:16:06 PDT
rc = wb.GetContentDOMWindow(address); should be 
rc = wb.GetContentDOMWindow(address2);
Comment 106 User image Yun Guo 2012-06-15 02:42:06 PDT
I write a simple html test case and verified that the printPreview is not work to print preview into a new window in embed mode. 

Within the html file, the printPreviewInNewWindow() js method is to print preview the frame content in the new opened window. I tested, it worked in firefox, but not work in our embed xulrunner. The printpreview window is empty in embed mode, same result as execution of above snippet code.

I attached the html file here. 

<html>
  <body>
    <iframe src="http://www.baidu.com" type="content"></iframe>
    <input type="button" value="openNewWindow" onclick="createNewWindow()">
    <input type="button" value="printPreview in NewWindow" onclick="printPreviewInNewWindow()">
    <script>
	var gWbp;
	var newWindow;
      	function createNewWindow() {
       		newWindow=window.open("about:blank");
      	}

	function printPreviewInNewWindow() {
        	netscape.security.PrivilegeManager.enablePrivilege(
          		"UniversalXPConnect");
        	gWbp = newWindow.QueryInterface	(Components.interfaces.nsIInterfaceRequestor)
             .getInterface(Components.interfaces.nsIWebBrowserPrint);
  		gWbp.printPreview(gWbp.globalPrintSettings, window.frames[0], null);
      }

    </script>
  </body>
</html>
Comment 107 User image Yun Guo 2012-06-17 19:08:35 PDT
Olli, do you know is there some other special things to do for embed xulrunner to printpreview a page in new window?
Comment 108 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-06-17 22:12:05 PDT
You do get any useful warnings to the terminal
(I assume you're using a debug build).

And please send me email. This discussion shouldn't happen in this bug.

Note You need to log in before you can comment on or make changes to this bug.