Closed Bug 487667 Opened 15 years ago Closed 15 years ago

Clone documents for printing

Categories

(Core :: Print Preview, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 14 obsolete files)

340 bytes, text/html
Details
1.30 KB, text/html
Details
195.98 KB, patch
joe
: review+
Details | Diff | Splinter Review
126.53 KB, patch
Details | Diff | Splinter Review
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
...
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?
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?
What's the concern with cloning stylesheets, exactly?
And to be clear, I'm not quite sure what the second paragraph of comment 2 is suggesting...
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.
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.
(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)
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.
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.
Attached patch initial WIP (obsolete) — Splinter Review
Doesn't yet take care of iframes/frames nor catalogsheets etc.
Removes the textfragment cloning.
..and script handling is a nice hack. Will replace with something else.
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.
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.
iframes/frames working, using an ugly hack.
Will cleanup while trying to get <img> working.
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.
That last bit wouldn't work well given our current <area> mess, so let's not worry about it quite yet.
(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.
(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.
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #372035 - Attachment is obsolete: true
I've spun off the second half of comment 16 into bug 500882.
Blocks: 500882
Attached patch v7 (obsolete) — Splinter Review
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.
Assignee: nobody → Olli.Pettay
Attachment #375093 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Do you need the artificial docshell?  Can you get away with just a document viewer?
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.
(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.
(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 :(
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.
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?
You mean on Windows? Perhaps we can capture the print output to a Windows metafile and play it back...
Yeah, on Windows. And that sounds like what they likely do today IIRC. No idea what happens on other platforms...
Attached patch v14 (obsolete) — Splinter Review
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?
Attachment #387958 - Attachment is obsolete: true
Attached patch v15 (obsolete) — Splinter Review
Fixes a small problem in media element handling and adds a helper method
to ensure reasonable document viewer for print preview.
Attachment #399186 - Attachment is obsolete: true
Attachment #399221 - Flags: review?(roc)
Comment on attachment 399221 [details] [diff] [review]
v15

Asking first a sort of "what you think about this" review :)
Attached patch v15.1 (obsolete) — Splinter Review
Attachment #399221 - Attachment is obsolete: true
Attachment #399229 - Flags: review?(roc)
Attachment #399221 - Flags: review?(roc)
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.
(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
(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?
(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.
(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).
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.
Well even now if you have same page loaded or same images in two windows,
the animations in both windows are stopped.
...but sure, cloning images for printing would be the better.
I was hoping that keeping the current behavior with plugins and images would be
enough for now.
Plugins yes, because we're not adding any significant new code for them. But for images you're adding these global arrays and mPrintImages...
(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.
The patch does trigger those problems more often than what happens currently, but
the problems are there even now.
If we snapshotted images for !IsDynamic presentations, wouldn't that fix the current problems too?
(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.
I'll try something a bit different what I thought earlier.
I may need to push some bit up to imgloader.
(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.
(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.
(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?
(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.
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.
(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
Attached patch v19 (obsolete) — Splinter Review
Just small changes in this one.
Attachment #399229 - Attachment is obsolete: true
Attachment #401078 - Flags: review?(roc)
Attachment #399229 - Flags: review?(roc)
(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.
... 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.
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?
Ah, perhaps at a bit later than what I thought. Stylesheet would still use the original image request.
But I still wonder who keeps the image request alive...
Or I guess, basically how to pass the context info down to nsStyleImage
Attached patch +animated image cloning (obsolete) — Splinter Review
This clones the current frame of animated images.
Attachment #401078 - Attachment is obsolete: true
Attachment #401193 - Flags: review?(roc)
Attachment #401078 - Flags: review?(roc)
+  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 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.
Attachment #401193 - Flags: review?(roc) → review+
(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.
Attached patch v20.1 (obsolete) — Splinter Review
Attachment #401420 - Flags: review?(jst)
Attachment #401420 - Flags: review?(dbaron)
Attachment #401420 - Flags: review?(bobbyholley)
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.
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.
Depends on: 517543
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?
Attachment #401420 - Flags: review?(bobbyholley) → review-
(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.
(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.
(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.
(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 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.
Attachment #401420 - Flags: review?(jst) → review+
Blocks: 520501
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.
(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.
Attached patch v21 (obsolete) — Splinter Review
Joe, could you review the libpr0n/ parts.
I changed imgDummyRequest::Clone based on comment 73.
Attachment #401420 - Attachment is obsolete: true
Attachment #412265 - Flags: review?(joe)
Attached patch v22 (obsolete) — Splinter Review
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.
Attachment #412265 - Attachment is obsolete: true
Attachment #412302 - Flags: review?(joe)
Attachment #412265 - Flags: review?(joe)
Attached patch v23 (obsolete) — Splinter Review
There was canvas painting missing.

But still, joe, please review the imglib part.
Attachment #412302 - Attachment is obsolete: true
Attachment #412328 - Flags: review?(joe)
Attachment #412302 - Flags: review?(joe)
Attached patch v24 (obsolete) — Splinter Review
This has simpler canvas handling, but webgl handling is broken because of
Bug 528746.
Attachment #412328 - Attachment is obsolete: true
Attachment #412430 - Flags: review?(joe)
Attachment #412328 - Flags: review?(joe)
Comment on attachment 412430 [details] [diff] [review]
v24

Roc, does the <canvas> handling look ok to you?
Look for nsHTMLCanvasElement::CopyInnerTo
Attachment #412430 - Flags: review?(roc)
Comment on attachment 412430 [details] [diff] [review]
v24

Yes, that looks fine.

What are we going to do for tests here?
Attachment #412430 - Flags: review?(roc) → review+
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.
Attached patch v24.1 (obsolete) — Splinter Review
Adds a test for 2d canvas print preview.
Attachment #412430 - Attachment is obsolete: true
Attachment #412829 - Flags: review?(joe)
Attachment #412430 - Flags: review?(joe)
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.
Attachment #412829 - Flags: review?(joe) → review-
Attached patch v25Splinter Review
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.
Attachment #401193 - Attachment is obsolete: true
Attachment #412829 - Attachment is obsolete: true
Attachment #412946 - Flags: review?(joe)
Attachment #412946 - Flags: review?(joe) → review+
Blocks: 435136
Attached patch v29Splinter Review
I pushed this patch.
Depends on: 534316
Depends on: 534502
Should this be marked FIXED?
Blocks: 526403
Depends on: 534720
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 535851
Depends on: 539645
Depends on: 535343
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?
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 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?
Depends on: 544018
> 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.
Please don't add comments here, but to the relevant bugs. Thanks.
Depends on: 545330
Depends on: 560235
Depends on: 573537
Depends on: 651028
Blocks: 110641
No longer blocks: 468568
Blocks: 468568
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.
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.
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
(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?
(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
(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.
Sorry, there's a typo mistake in line: "rc = wb.GetContentDOMWindow(address);" when I write that comments. But it's correct in my project.
rc = wb.GetContentDOMWindow(address); should be 
rc = wb.GetContentDOMWindow(address2);
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>
Olli, do you know is there some other special things to do for embed xulrunner to printpreview a page in new window?
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.
No longer depends on: 573537
Regressions: 573537
No longer depends on: 534316
Blocks: 1843883
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: