Closed Bug 745025 Opened 12 years ago Closed 12 years ago

Implement CanvasElement.mozPrintCallback

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: julian.viereck, Assigned: bdahl)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [DocArea=API])

Attachments

(7 files, 22 obsolete files)

44.81 KB, application/pdf
Details
38.77 KB, patch
Details | Diff | Splinter Review
3.91 KB, text/html
Details
14.67 KB, text/html
Details
103.29 KB, text/plain
Details
37.68 KB, patch
myk
: review+
myk
: checkin+
Details | Diff | Splinter Review
17.40 KB, patch
smaug
: review+
myk
: checkin+
Details | Diff | Splinter Review
This bug is about doing the implementation of the proposed mozPrintCallback in [1].

[1]: https://groups.google.com/d/msg/mozilla.dev.tech.layout/-JV2whghEVg/DzwLnwpy_YcJ
Attached patch First implementation (obsolete) — Splinter Review
This implements the proposed mozPrintCallback for the non-azure backend. This includes support for actual printout as well as for the build in Firefox preview.

To get the preview support implemented, I had to do some more lifting on the canvasRendering context. This means adding a new nsICanvasRenderingContextInternal::AdjustToSize, and the internal new members mExternalWidth/mExternalHeight/mExternalScaleX/mExternalScaleY/mExternalScale in the nsCanvasRenderingContext2D.

When using this API in preview, the context size of the canvas has to match the actual pixel size of the canvas. The actual pixel size of the canvas is what I call "externalSize". However, as interacting with the canvas context should result in the same rendering output as if the canvas would have it's DOM size (DOM size = canvasElement.width & canvasElement.height), some scaling is necessary such that the DOM-size gets mapped to the external-size.

After I've got some feedback on the non-azure implementation, I will do the azure-implementation.
Assignee: nobody → julian.viereck
Status: NEW → ASSIGNED
Attachment #614621 - Flags: feedback?(roc)
Attached file Test page I used to test the code (obsolete) —
Can someone give me a pointer on how to write unit tests for this kind of feature?
These patches don't make the canvas rerender to the right size if the zoom level changes in preview (I tested this patch on mac, where there is no build in preview anymore, but now I'm developing on Linux, I see the problem). Will fix this.
Attachment #614621 - Flags: feedback?(bugs)
Comment on attachment 614621 [details] [diff] [review]
First implementation

It is a bit hard to read the patch because it isn't created using
-U 8 -p


>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::AdjustToSize(PRInt32 width, PRInt32 height)
>+{
>+    if (!EnsureSurface())
>+        return NS_ERROR_FAILURE;
Not sure what coding style this file uses, but
the right coding style is 2 space indentation
and
if (expr) {
  stmt;
}
But if the file uses some other coding style, use the same.


>+
>+    if (width == 0 || height == 0)
>+        return NS_ERROR_FAILURE;
>+
>+    PRInt32 oldWidth = mWidth, oldHeight = mHeight;
>+
>+    SetDimensions(width, height);
>+    mExternalWidth = width;
>+    mExternalHeight = height;
>+    mExternalScaleX = ((float)mWidth)/((float)oldWidth);
>+    mExternalScaleY = ((float)mHeight)/((float)oldHeight);
C++ casting and space before and after /


> nsHTMLCanvasElement::nsHTMLCanvasElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>-  : nsGenericHTMLElement(aNodeInfo), mWriteOnly(false)
>+  : nsGenericHTMLElement(aNodeInfo), mIsReadyToPrint(false), mWriteOnly(false),
>+    mIsPrintCanvas(false), mIsPrintPreview(false)
> {
> }
> 
> nsHTMLCanvasElement::~nsHTMLCanvasElement()
> {
>+  mPrintCallback = NULL;
>+  mCurrentContext = NULL;
Not needed.

But mPrintCallback and mCurrentContext should both be added to cycle collection


>+[scriptable, function, uuid(8d5fb8a0-7782-11e1-b0c4-0800200c9a66)]
>+interface nsIPrintCallback : nsISupports {
>+  void render(in nsISupports ctx);
>+};
What is ctx here?
Attachment #614621 - Flags: feedback?(bugs) → feedback-
Comment on attachment 614621 [details] [diff] [review]
First implementation


>+++ b/layout/generic/nsHTMLCanvasFrame.cpp
>@@ -292,6 +292,9 @@
>   nsHTMLCanvasElement* element = static_cast<nsHTMLCanvasElement*>(GetContent());
>   nsIntSize canvasSize = GetCanvasSize();
> 
>+  if (!element->HandlePrintCallback(canvasSize))
>+    return nsnull;
>+
This looks like a very scary place to call any JS callback.
Olli, thanks a lot for your feedback!

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 614621 [details] [diff] [review]
> First implementation
> 
> 
> >+++ b/layout/generic/nsHTMLCanvasFrame.cpp
> >@@ -292,6 +292,9 @@
> >   nsHTMLCanvasElement* element = static_cast<nsHTMLCanvasElement*>(GetContent());
> >   nsIntSize canvasSize = GetCanvasSize();
> > 
> >+  if (!element->HandlePrintCallback(canvasSize))
> >+    return nsnull;
> >+
> This looks like a very scary place to call any JS callback.

Can you give me an idea where a better place for calling the JS callback would be? Dump question: What's so scary about calling the `HandlePrintCallback` function here, that calls the the actual JS-callback?
The js callback can for example call window.close() which might have rather unexpected results, like
possibly killing the nsHTMLCanvasFrame.
(In reply to Olli Pettay [:smaug] from comment #9)
> The js callback can for example call window.close() which might have rather
> unexpected results, like
> possibly killing the nsHTMLCanvasFrame.

Note that the canvas that gets printed/drawn here and that the `HandlePrintCallback` function is called from is the canvas for the print document, which is a static clone of the original document. That's not the same document the JS is executed in. That means, if within the callback the `window.close()` function is called, the original canvas/document might gets destroyed, but the copied one form the print document is still around.

Is this still a problem then?
Comment on attachment 614621 [details] [diff] [review]
First implementation

Review of attachment 614621 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/public/nsICanvasRenderingContextInternal.h
@@ +86,4 @@
>    // whenever the size of the element changes.
>    NS_IMETHOD SetDimensions(PRInt32 width, PRInt32 height) = 0;
>  
> +  NS_IMETHOD AdjustToSize(PRInt32 width, PRInt32 height) = 0;

Need to document this.

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +468,5 @@
>      // Member vars
>      PRInt32 mWidth, mHeight;
> +    PRInt32 mExternalWidth, mExternalHeight;
> +    bool mExternalScale;
> +    float mExternalScaleX, mExternalScaleY;

The external-scale support is a little bit hacky. I'd like to have a separate, clean patch that allows the backing canvas to have different size from mWidth/mHeight --- let's call it mSurfaceWidth/mSurfaceHeight and doesn't store mExternalScaleX/mExternalScaleY, since they're redundant with the widths and heights.

Does that make sense?

::: content/html/content/public/nsHTMLCanvasElement.h
@@ +205,5 @@
>    nsString mCurrentContextId;
> +
> +  bool mIsReadyToPrint;
> +
> +  nsCOMPtr<nsIPrintCallback> mPrintCallback;

You'll need to add some cycle collector goop to traverse mPrintCallback --- to NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED and NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +165,5 @@
> +
> +  // For print preview, resize the canvas context, such that
> +  if (mIsPrintPreview) {
> +    GetClientWidth(&size.width);
> +    GetClientHeight(&size.height);

Instead of calling these, you need to GetPrimaryFrame for this element, call GetContentRect().GetSize() on that frame, then convert the size in appunits to the size in dev pixels (using nsPresContext::AppUnitsToDevPixels) and round up.

::: dom/interfaces/html/nsIDOMHTMLCanvasElement.idl
@@ +57,5 @@
>  interface nsIInputStreamCallback;
>  
> +[scriptable, function, uuid(8d5fb8a0-7782-11e1-b0c4-0800200c9a66)]
> +interface nsIPrintCallback : nsISupports {
> +  void render(in nsISupports ctx);

This is going to be some kind of nsIDOMPrintState object right? It deserves its own IDL definition.

::: layout/generic/nsSimplePageSequence.cpp
@@ +522,5 @@
>  }
>  
> +void GetCanvasElementsInFrame(nsTArray<nsRefPtr<nsHTMLCanvasElement> >* aArr, nsIFrame* aFrame)
> +{
> +

Put the out parameter last and drop this blank line.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Comment on attachment 614621 [details] [diff] [review]
> ::: content/canvas/src/nsCanvasRenderingContext2D.cpp
> @@ +468,5 @@
> >      // Member vars
> >      PRInt32 mWidth, mHeight;
> > +    PRInt32 mExternalWidth, mExternalHeight;
> > +    bool mExternalScale;
> > +    float mExternalScaleX, mExternalScaleY;
> 
> The external-scale support is a little bit hacky. I'd like to have a
> separate, clean patch that allows the backing canvas to have different size
> from mWidth/mHeight --- let's call it mSurfaceWidth/mSurfaceHeight [...]

The |mExternalWidth| and |mExternalHeight| do not mean the size of the surface, but the size of the canvas as set in the DOM with the "widht" and "height" attributes on the canvas element e.g. <canvas height="200px"> -> mExternalHeight = 200px. The size of the surface is stored in the |mWidth| and |mHeight| property.

What about renaming |mExternalWidth| -> |mCanvasWidth| and same for height?

> [...] and doesn't store mExternalScaleX/mExternalScaleY, since they're redundant with
> the widths and heights.

They are redundant but the scaling between surface and canvas size is used at some points in the code. If you think it's not worth storing the scaling factor and do the computation every time it's needed, can I create a macro to not retype the scaling calculation all the time?
Actually, I'm wondering if doing this resize magic just for preview should be included in the first iteration of this API. That would make some stuff easier and in the case of PDF.JS this shouldn't be a big issue.
(In reply to Julian Viereck from comment #12)
> The |mExternalWidth| and |mExternalHeight| do not mean the size of the
> surface, but the size of the canvas as set in the DOM with the "widht" and
> "height" attributes on the canvas element e.g. <canvas height="200px"> ->
> mExternalHeight = 200px. The size of the surface is stored in the |mWidth|
> and |mHeight| property.
> 
> What about renaming |mExternalWidth| -> |mCanvasWidth| and same for height?

OK, but also change mWidth/mHeight to mSurfaceWidth/mSurfaceHeight.

> > [...] and doesn't store mExternalScaleX/mExternalScaleY, since they're redundant with
> > the widths and heights.
> 
> They are redundant but the scaling between surface and canvas size is used
> at some points in the code. If you think it's not worth storing the scaling
> factor and do the computation every time it's needed, can I create a macro
> to not retype the scaling calculation all the time?

A helper function or two would be better.
(In reply to Julian Viereck from comment #13)
> Actually, I'm wondering if doing this resize magic just for preview should
> be included in the first iteration of this API. That would make some stuff
> easier and in the case of PDF.JS this shouldn't be a big issue.

That sounds good.
Blocks: 748935
Attached patch WIP 2Splinter Review
=== Current iteration as I got stuff. What changed
* There is no an object passed to the first argument of the mozPrintCallback that has the new functions |done()| and |abort()| on them. Calling |done()| signals the printing backend, that the canvas has successfuly finished printing, while |done()| tells the backend to stop the entire print process. |abort()| has no effect in print preview right now.
* The API is not async --- that means, the user don't have to call the |done()| function right away. S/He can call the function after some timout.
* I forced to use the Non-Azure-2D backend for now. That way, I don't have to do for this API specific implementations on both backends at this point.


=== Known broken stuff:
* The canvas element doesn't rerender properly in PrintPreview. This is partially, because the Canvas::MarkContextClean() function is not called. Need to dive more into this...
* There is no effect of calling |abort()| while in preview. Maybe it's good enough to ship a first iteration without any effect to the preview window?

=== Things that would be helpful to get feedback on:
* I tried to implement some CC stuff for nsCanvasPrintState, as you can see in the patch. It's commented out as the compiler doesn't like it:

/Users/jviereck/develop/moz/pdfjs/ff_hg_callback/content/html/content/src/nsHTMLCanvasElement.cpp:135:30: error: no member named 'cycleCollection' in 'nsCanvasPrintState'
nsresult nsCanvasPrintState::cycleCollection::Traverse (void *p, nsCycleCollectionTraversalCallback &cb) { nsISupports *s = static_cast<nsISupports*>(p); do { if (!(CheckForRightISupports(s))) { NS_DebugBreak_P(NS_DEBUG_ASSERTION, "not the nsISupports pointer we expect", "CheckForRightISupports(s)", "/Users/jviereck/develop/moz/pdfjs/ff_hg_callback/content/html/content/src/nsHTMLCanvasElement.cpp", 135); } } while (0); nsCanvasPrintState *tmp = static_cast<nsCanvasPrintState*>(Downcast(s));

Where there is an arrow in the console output below the "nsCanvasPrintState" at the very end.

* The PrintTimer calls a new function nsPrintEngine::PrePrintPage(...) that checks if all canvas elements on the next page have finished (== the done() callback got called or the abort(), which sets mPrt->mIsAborted=true). Does it make sense to do this kind of periodical checking using the PrintTimer?
* The rendering context returns the originalCanvas now. The way this is implemented in nsCanvasRenderingContext2D::GetCanvas() makes me think there might be potential to introduce a leak there? (See also the new |nsHTMLCanvasElement::GetOriginalCanvas()| function).
Attachment #614621 - Attachment is obsolete: true
Attachment #614621 - Flags: feedback?(roc)
Attachment #625809 - Flags: feedback?(roc)
Attached file Upated test file
Has async-painting & checkbox to abort the second page while printing.
Attachment #614626 - Attachment is obsolete: true
Comment on attachment 625809 [details] [diff] [review]
WIP 2

Review of attachment 625809 [details] [diff] [review]:
-----------------------------------------------------------------

Remove the debug printfs, that'll make this easier to review.

::: content/html/content/public/nsHTMLCanvasElement.h
@@ +206,5 @@
>                              nsICanvasRenderingContextInternal **aContext);
>  
>    nsString mCurrentContextId;
> +
> +  bool mIsReadyToPrint;

Move this down with the other bools

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +139,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsCanvasPrintState)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mContext);
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +*/

You need to participate in cycle collection. The interface map can be something like

NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsCanvasPrintState)
  NS_INTERFACE_MAP_ENTRY(nsISupports)
  NS_INTERFACE_MAP_ENTRY(nsICanvasPrintState)
  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(CanvasPrintState)
NS_INTERFACE_MAP_END

::: layout/generic/nsSimplePageSequence.h
@@ +170,5 @@
>    PRInt32      mToPageNum;
>    nsTArray<PRInt32> mPageRanges;
> +  nsTArray<nsRefPtr<nsHTMLCanvasElement> > mCurrentCanvasList;
> +
> +  bool         mCurrentCanvasListSetup;

This bool should be with the other bools

@@ +171,5 @@
>    nsTArray<PRInt32> mPageRanges;
> +  nsTArray<nsRefPtr<nsHTMLCanvasElement> > mCurrentCanvasList;
> +
> +  bool         mCurrentCanvasListSetup;
> +  void         ComputePrintThisPage();

This needs a better name, say DetermineWhetherToPrintPage, and it needs to be moved to be near the other methods.
(In reply to Julian Viereck from comment #16)
> === Current iteration as I got stuff. What changed
> * There is no an object passed to the first argument of the mozPrintCallback
> that has the new functions |done()| and |abort()| on them. Calling |done()|
> signals the printing backend, that the canvas has successfuly finished
> printing, while |done()| tells the backend to stop the entire print process.
> |abort()| has no effect in print preview right now.
> * The API is not async --- that means, the user don't have to call the
> |done()| function right away. S/He can call the function after some timout.
> * I forced to use the Non-Azure-2D backend for now. That way, I don't have
> to do for this API specific implementations on both backends at this point.

That's all fine.

> === Known broken stuff:
> * The canvas element doesn't rerender properly in PrintPreview. This is
> partially, because the Canvas::MarkContextClean() function is not called.
> Need to dive more into this...
> * There is no effect of calling |abort()| while in preview. Maybe it's good
> enough to ship a first iteration without any effect to the preview window?

Yes.

> === Things that would be helpful to get feedback on:
> * I tried to implement some CC stuff for nsCanvasPrintState, as you can see
> in the patch. It's commented out as the compiler doesn't like it:
> 
> /Users/jviereck/develop/moz/pdfjs/ff_hg_callback/content/html/content/src/
> nsHTMLCanvasElement.cpp:135:30: error: no member named 'cycleCollection' in
> 'nsCanvasPrintState'
> nsresult nsCanvasPrintState::cycleCollection::Traverse (void *p,
> nsCycleCollectionTraversalCallback &cb) { nsISupports *s =
> static_cast<nsISupports*>(p); do { if (!(CheckForRightISupports(s))) {
> NS_DebugBreak_P(NS_DEBUG_ASSERTION, "not the nsISupports pointer we expect",
> "CheckForRightISupports(s)",
> "/Users/jviereck/develop/moz/pdfjs/ff_hg_callback/contenhtml/content/src/
> nsHTMLCanvasElement.cpp", 135); } } while (0); nsCanvasPrintState *tmp =
> static_cast<nsCanvasPrintState*>(Downcast(s));

The suggestion in my previous comment might help. Also, declare this in your class
  NS_DECL_CYCLE_COLLECTION_CLASS(nsCanvasPrintState)
  NS_DECL_CYCLE_COLLECTING_ISUPPORTS

> * The PrintTimer calls a new function nsPrintEngine::PrePrintPage(...) that
> checks if all canvas elements on the next page have finished (== the done()
> callback got called or the abort(), which sets mPrt->mIsAborted=true). Does
> it make sense to do this kind of periodical checking using the PrintTimer?

It's not great to simply poll with the timer. You should really have some code in Done() and Abort() that triggers a rerun of the timer code as soon as possible via an XPCOM event (but not synchronously during the Done() or Abort() call!)

> * The rendering context returns the originalCanvas now. The way this is
> implemented in nsCanvasRenderingContext2D::GetCanvas() makes me think there
> might be potential to introduce a leak there? (See also the new
> |nsHTMLCanvasElement::GetOriginalCanvas()| function).

GetCanvas() must addref its result. That is a standard XPCOM/XPConnect requirement.
This patch tries to implement the async event as requested by :roc in previous comment.

This patch doesn't compile right now /bc:

In file included from /Users/jviereck/develop/moz/pdfjs/ff_hg_callback/content/html/content/src/nsHTMLCanvasElement.cpp:61:
In file included from /Users/jviereck/develop/moz/pdfjs/ff_hg_callback/content/html/content/src/../../../../layout/printing/nsPrintCanvasEvent.h:40:
/Users/jviereck/develop/moz/pdfjs/ff_hg_callback/content/html/content/src/../../../../layout/printing/nsPagePrintTimer.h:43:10: fatal error: 'nsIDocumentViewerPrint.h' file
      not found
#include "nsIDocumentViewerPrint.h"
          ^

Is the solution to create an interface for the nsPagePrintTimer? That way, the nsHTMLCanvasElement.cpp don't have to include the "nsPagePrintTimer.h", which requires again, that "nsIDocumentViewerPrint.h" is included while building nsHTMLCanvasElement.cpp?

Overall, I have the feeling I've over-engineered stuff to get the async-event implemented:

* Created a new class nsPrintCanvasNotifier that derives nsRunnable
* The nsPrintEngine creates an instance (mPrintCanvasNotifier) of nsPrintCanvasNotifier, passes in the mPagePrintTimer and stores it on the printEngine
* The PagePrintTimer calls nsPrintEngine::SetupPrintPage (what was before PrePrintPage), which now returns the number of PrintCanvas on the current page.
* nsPrintEngine::SetupPrintPage passes the reference to the mPrintCanvasNotifier to nsSimplePageSequence::PreSetupPage(...), which passes it again along to the nsCanvasElement::CallPrintCallback(...) function, which passes it to the created nsPrintState
* as done() or abort() is called on the nsPrintState, the passed in nsPrintCanvasNotifier is dispatched agains the current main thread
* the nsPrintCanvasNotifier::Run method invokes the new nsPagePrintTimer::PrintCanvasFinished(bool aAborted)
* the nsPagePrintTimer::PrintCanvasFinished() method then either aborts the print process if aAborted is true OR it decrements a numberOfPrintCanvasOnCurrentPage counter (aka mPrintCanvasWaiting) and if this counter is zero and then start the actual printing of the page.

Any simplier idea? The runnable got to know about the PrintEngine or the PagePrintTimer to decrease some counter variable on how many printCanvas are there still outstanding. My thinking was, that either pass along the PrintEngine object, the PagePrintTimer object or create a new object, which I did.
You can probably add nsIDocumentViewerPrint.h to layout/base/Makefile.in to fix your compile error.
Maybe this would be simpler:
-- Just pass around the nsPagePrintTimer directly
-- Pass it around via its nsITimerCallbackInterface and have Done() and Abort() simply call Notify passing a null timer parameter
-- Instead of keeping around state for the number of outstanding printcanvases, and passing aborted-status when triggering nsPagePrintTimer, just sweep the entire list of initial print canvases to count how many are done and whether any are aborted.
Based on :roc's comments 22.

This turns out to be way simplier.

This patch also includes some new methodes to "reset" the PrintState instance. This cleans up the references to the mCallaback and mContext. Without adding cycle-collection goodness, this calls the destructor of the nsCanvasPrintState class. AFAIKT there can't be any cyclic references to an nsCanvasPrintState and all the printStates are about to get reset. Therefore, is adding the macros to define the cycle collection stuff for the nsCanvasPrintState necessary in this case?

Next up: Figure out why invalidation don't happen in print-preview.
Attachment #626224 - Attachment is obsolete: true
(In reply to Julian Viereck from comment #23)
> This patch also includes some new methodes to "reset" the PrintState
> instance. This cleans up the references to the mCallaback and mContext.
> Without adding cycle-collection goodness, this calls the destructor of the
> nsCanvasPrintState class. AFAIKT there can't be any cyclic references to an
> nsCanvasPrintState and all the printStates are about to get reset.
> Therefore, is adding the macros to define the cycle collection stuff for the
> nsCanvasPrintState necessary in this case?

Maybe not, but it's pretty harmless.
I think I figured out why the canvas invalidation is not working within print preview:

a) the nsCanvasRenderingContext2D::MarkContextClean function is not called, due to to the if in [1]:

    if (aBuilder->IsPaintingToWindow()) {

A workaround for this might be to check if the canvas associated to the rendering context is a print canvas and in such a case, make the if evaluate to true and execute the if body.

b) The invalidation is wrong, as the scrolled offset is not taken into account. When turning on the layer flashing nglayout.debug.paint_flashing=true, I see the invalidation rect way to high up. The value of |r| in [2] also shows, that the position is thought to be always at the very top of the browser, but that's not the case if the user scrolled down the print preview window.

For this either 
i) someone can help me with how to calculate the scroll offset into the invalidation math or 
ii) whenever the canvas wants to invalidate it's content and it's in print preview and the canvas is a print canvas, then lookup the pageSequence frame and do an invalidate on the entire pageSequence frame (which is of cause very bad...).


[1]: http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#4196
[2]: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLCanvasFrame.cpp#274
(In reply to Julian Viereck from comment #25)
> a) the nsCanvasRenderingContext2D::MarkContextClean function is not called,
> due to to the if in [1]:
> 
>     if (aBuilder->IsPaintingToWindow()) {
> 
> A workaround for this might be to check if the canvas associated to the
> rendering context is a print canvas and in such a case, make the if evaluate
> to true and execute the if body.

Sure.

> b) The invalidation is wrong, as the scrolled offset is not taken into
> account. When turning on the layer flashing
> nglayout.debug.paint_flashing=true, I see the invalidation rect way to high
> up. The value of |r| in [2] also shows, that the position is thought to be
> always at the very top of the browser, but that's not the case if the user
> scrolled down the print preview window.
> 
> For this either 
> i) someone can help me with how to calculate the scroll offset into the
> invalidation math or 
> ii) whenever the canvas wants to invalidate it's content and it's in print
> preview and the canvas is a print canvas, then lookup the pageSequence frame
> and do an invalidate on the entire pageSequence frame (which is of cause
> very bad...).

How about iii) ignore the problem until bug 539356 lands and makes all the existing invalidation code obsolete? :-)
Attached patch WIP 3 (obsolete) — Splinter Review
This iteration fixes the former mentioned issues of not closing print preview as well as the rerendering issue in print-preview.

I cleaned the patch up by removing printf and some other stuff I've introduced. |nsICanvasPrintState| became |nsIDOMCanvasPrintState| as it's exposed to the DOM.

The blocking two things before setting this up for final review are:
* Abortion for printing documents doesn't work on linux.
* The way GetOriginalCanvas works is not right yet. I guess it should has as return type already_AddRefed<nsHTMLCanvasElement>?
* Missing unit tests
* If the user opens print preview, starts the printing from print preview and the printing get aborted, print preview won't close.
Attachment #626595 - Attachment is obsolete: true
Attached patch WIP 3.1 (obsolete) — Splinter Review
Turned out WIP 3 was partial broken :/ Sorry.

This patch also has -pU8, one printf statement that survived was killed, |nsCanvasPrintState| -> |nsHTMLCanvasPrintState|.
Attachment #629360 - Attachment is obsolete: true
While looking at the problem with "aborting continues printing", I've noticed, that pressing the cancel button on the print progress window doesn't stop the printing progress. The pages printed so far still make it to the printer and get printed. I've filled this bug about it: 

    https://bugzilla.mozilla.org/show_bug.cgi?id=761008

If this gets confirmed, I think it's better to land the API without the |abort()|, although this is a requirement for PDF.JS.
Unassign me as I won't be working on this in the near future. Someone else from the PDF.JS team will take care of this feature to get landed.
Status: ASSIGNED → NEW
Doing a try server run of the current patches in https://tbpl.mozilla.org/?tree=Try&rev=f34ec2a67da2, one of the tests fail reproduceable on Windows and Linux. Note that it doesn't fail on Mac or Android.
Talking to :bholley, the crashes in the try-server run might be related to bug 758415. However, running the tests locally, there seem to be something other broken still.
Attached patch WIP 3.1 without abort() support (obsolete) — Splinter Review
Same as WIP 3.1, but without the code for handling |abort()|. This takes some of the issues from the TODO list for landing this feature. Once some other stuff are resolved like bug 761008, it makes sense to add the |abort()| functionality to the API.
Attachment #629632 - Attachment is obsolete: true
Failing test. The tests work without applying the patch 629860. Just looking at the traceback, I don't really understand what's failing here. Removing the |ResetPrintCallback()| line from the ~nsHTMLCanvasElement function doesn't fix this.
Attached patch mozPrintCallback 3.2 (obsolete) — Splinter Review
-Fixes the crash problem when the .done() callback was immediately called.
-Adds printing reftest and makes it so printing reftests make a static clone.
-Fixed two memory leaks.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=b2673f4abd08
Attachment #629860 - Attachment is obsolete: true
Attachment #642151 - Flags: review?(roc)
Comment on attachment 642151 [details] [diff] [review]
mozPrintCallback 3.2

Review of attachment 642151 [details] [diff] [review]:
-----------------------------------------------------------------

Can you break this patch up a bit?

The changes to reftests to use a static clone of the document should be split out and landed on their own before the rest of this stuff lands.

The rest of the implementation can probably also be split up a bit. Support for CanvasPrintState can probably be done in a patch separate from the code that actually triggers the print callbacks.
> The changes to reftests to use a static clone of the document should be
> split out and landed on their own before the rest of this stuff lands.
Should this be in a separate bug or just a patch here? (I'm still new the mc process)
Depends on: 774423
Attachment #642151 - Attachment is obsolete: true
Attachment #642151 - Flags: review?(roc)
Attachment #642705 - Flags: review?(roc)
roc: I went ahead and made a new bug for the static clone(see bug 774423).  I also split the patches up by dom/content and layout.  Let me know if this is okay.  Also, do you have recommendation for the dom/content review?
(In reply to Brendan Dahl from comment #40)
> roc: I went ahead and made a new bug for the static clone(see bug 774423).

Thanks. Another bug is good, although this bug would have been fine too.

> I also split the patches up by dom/content and layout.  Let me know if this
> is okay.  Also, do you have recommendation for the dom/content review?

Olli Pettay.
Comment on attachment 642705 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback

Review of attachment 642705 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLCanvasFrame.cpp
@@ +259,5 @@
>    nsHTMLCanvasElement* element = static_cast<nsHTMLCanvasElement*>(GetContent());
>    nsIntSize canvasSize = GetCanvasSize();
>  
> +  if (!element->HandlePrintCallback())
> +    return nsnull;

I don't understand why this check is here

::: layout/generic/nsSimplePageSequence.cpp
@@ +508,5 @@
> +      // If there is a canvasFrame, try to get actual canvas element.
> +      if (canvasFrame) {
> +        nsHTMLCanvasElement *canvas =
> +          nsHTMLCanvasElement::FromContent(canvasFrame->GetContent());
> +          // CanvasElementFromContent(canvasFrame->GetContent());

Remove commentd-out code

@@ +622,5 @@
> +          renderingContext->ThebesContext()->CurrentSurface();
> +      NS_ENSURE_TRUE(renderingSurface, NS_ERROR_OUT_OF_MEMORY);
> +    }
> +
> +    for (PRInt32 i = mCurrentCanvasList.Length() - 1; i >= 0 ; i--) {

This loop can move into the previous if block

@@ +683,5 @@
> +    nsHTMLCanvasElement *canvas = mCurrentCanvasList[i];
> +    nsICanvasRenderingContextInternal *ctx = canvas->GetContextAtIndex(0);
> +
> +    if (ctx)
> +      ctx->Reset();

Completely wiping out the canvas contents doesn't seem to be the right thing to do here.

Also, {} around the if body.

@@ +705,5 @@
> +// actual page number, when printing selection it prints the page number starting
> +// with the first page of the selection. For example if the user has a 
> +// selection that starts on page 2 and ends on page 3, the page numbers when
> +// print are 1 and then two (which is different than printing a page range, where
> +// the page numbers would have been 2 and then 3)

Indent comment

@@ +760,5 @@
>        PR_PL(("SeqFr::PrintNextPage -> %p PageNo: %d", pf, mPageNum));
>  
>        nsRefPtr<nsRenderingContext> renderingContext;
>        dc->CreateRenderingContext(*getter_AddRefs(renderingContext));
>        NS_ENSURE_TRUE(renderingContext, NS_ERROR_OUT_OF_MEMORY);

We've duplicated some code between PrintNextPage and PrePrintNextPage here. Can we fix that?

::: layout/generic/nsSimplePageSequence.h
@@ +99,5 @@
>    virtual nsIAtom* GetType() const;
> +
> +  virtual void InvalidateInternal(const nsRect& aDamageRect,
> +                                   nscoord aX, nscoord aY, nsIFrame* aForChild,
> +                                   PRUint32 aFlags);

Fix indent.
Attachment #642703 - Flags: review?(bugs)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> Comment on attachment 642705 [details] [diff] [review]
> Part 2 - Layout - Printing support for mozPrintCallback
> 
> Review of attachment 642705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsHTMLCanvasFrame.cpp
> @@ +259,5 @@
> >    nsHTMLCanvasElement* element = static_cast<nsHTMLCanvasElement*>(GetContent());
> >    nsIntSize canvasSize = GetCanvasSize();
> >  
> > +  if (!element->HandlePrintCallback())
> > +    return nsnull;
> 
> I don't understand why this check is here

I'm not sure what Julian's original intentions were here.  I've removed the check
and just made it call HandlePrintCallback and made HandlePrintCallback return void.

> ::: layout/generic/nsSimplePageSequence.cpp
> @@ +508,5 @@
> > +      // If there is a canvasFrame, try to get actual canvas element.
> > +      if (canvasFrame) {
> > +        nsHTMLCanvasElement *canvas =
> > +          nsHTMLCanvasElement::FromContent(canvasFrame->GetContent());
> > +          // CanvasElementFromContent(canvasFrame->GetContent());
> 
> Remove commentd-out code

Removed

> 
> @@ +622,5 @@
> > +          renderingContext->ThebesContext()->CurrentSurface();
> > +      NS_ENSURE_TRUE(renderingSurface, NS_ERROR_OUT_OF_MEMORY);
> > +    }
> > +
> > +    for (PRInt32 i = mCurrentCanvasList.Length() - 1; i >= 0 ; i--) {
> 
> This loop can move into the previous if block

Done

> 
> @@ +683,5 @@
> > +    nsHTMLCanvasElement *canvas = mCurrentCanvasList[i];
> > +    nsICanvasRenderingContextInternal *ctx = canvas->GetContextAtIndex(0);
> > +
> > +    if (ctx)
> > +      ctx->Reset();
> 
> Completely wiping out the canvas contents doesn't seem to be the right thing
> to do here.
> 
> Also, {} around the if body.
> 

Removed

> @@ +705,5 @@
> > +// actual page number, when printing selection it prints the page number starting
> > +// with the first page of the selection. For example if the user has a 
> > +// selection that starts on page 2 and ends on page 3, the page numbers when
> > +// print are 1 and then two (which is different than printing a page range, where
> > +// the page numbers would have been 2 and then 3)
> 
> Indent comment

Done

> @@ +760,5 @@
> >        PR_PL(("SeqFr::PrintNextPage -> %p PageNo: %d", pf, mPageNum));
> >  
> >        nsRefPtr<nsRenderingContext> renderingContext;
> >        dc->CreateRenderingContext(*getter_AddRefs(renderingContext));
> >        NS_ENSURE_TRUE(renderingContext, NS_ERROR_OUT_OF_MEMORY);
> 
> We've duplicated some code between PrintNextPage and PrePrintNextPage here.
> Can we fix that?

I removed an unused variable "renderingSurface" in PrintNextPage() and now the only
duplicate lines are the above three lines. I'm not sure moving just those three
lines to a function will make anything better.

> 
> ::: layout/generic/nsSimplePageSequence.h
> @@ +99,5 @@
> >    virtual nsIAtom* GetType() const;
> > +
> > +  virtual void InvalidateInternal(const nsRect& aDamageRect,
> > +                                   nscoord aX, nscoord aY, nsIFrame* aForChild,
> > +                                   PRUint32 aFlags);
> 
> Fix indent.

Done
Attachment #642705 - Attachment is obsolete: true
Attachment #642705 - Flags: review?(roc)
Attachment #643201 - Flags: review?(roc)
Comment on attachment 643201 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback

Review of attachment 643201 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #643201 - Flags: review?(roc) → review+
Comment on attachment 642703 [details] [diff] [review]
Part 1 - DOM - mozPrintCallback and MozCanvasPrintState

> nsCanvasRenderingContext2D::GetCanvas(nsIDOMHTMLCanvasElement **canvas)
> {
>-    NS_IF_ADDREF(*canvas = mCanvasElement);
>+    NS_IF_ADDREF(*canvas = mCanvasElement->GetOriginalCanvas());
> 
You should null check mCanvasElement


>+++ b/content/html/content/src/nsHTMLCanvasElement.cpp
>@@ -24,51 +24,151 @@
> #include "mozilla/Preferences.h"
> #include "mozilla/Telemetry.h"
> 
> #include "nsFrameManager.h"
> #include "nsDisplayList.h"
> #include "ImageLayers.h"
> #include "BasicLayers.h"
> #include "imgIEncoder.h"
>+#include "nsITimer.h"
>+#include "nsAsyncDOMEvent.h"
> 
> #include "nsIWritablePropertyBag2.h"
> 
>+#include "nsIDOMHTMLCanvasElement.h"
>+
You really need to #include this here?


>+#define NS_ICANVASPRINTSTATE_IID \
>+   {0x8d5fb8a0, 0x7782, 0x11e1, { 0xb0, 0xc4, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x67 }} 
Why do you need this?


>+
>+class nsHTMLCanvasPrintState : public nsIDOMMozCanvasPrintState
>+{
>+public:
>+  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ICANVASPRINTSTATE_IID)
>+
>+
>+  nsHTMLCanvasPrintState(
>+      nsHTMLCanvasElement *aCanvas,
>+      nsICanvasRenderingContextInternal *aContext, 
>+      nsITimerCallback *aCallback)
>+  : mIsDone(false), mPendingNotify(false),
>+    mCanvas(aCanvas), mContext(aContext), mCallback(aCallback) {
{ should be in the next line


> nsHTMLCanvasElement::nsHTMLCanvasElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>-  : nsGenericHTMLElement(aNodeInfo), mWriteOnly(false)
>-{
>+  : nsGenericHTMLElement(aNodeInfo), 
>+    mOriginalCanvas(nsnull), mPrintState(nsnull), mWriteOnly(false),
>+    mIsPrintCanvas(false), mIsPrintPreview(false) {
> }
No need to initialize nsCOMPtr member variables to nsnull


>+nsHTMLCanvasElement::GetOriginalCanvas()
>+{
>+  if (mOriginalCanvas == nsnull) {
>+    return this;
>+  } else {
>+    return mOriginalCanvas.get();
>+  }
>+}
return mOriginalCanvas ? mOriginalCanvas.get() : this;


> nsHTMLCanvasElement::CopyInnerTo(nsGenericElement* aDest)
> {
>   nsresult rv = nsGenericHTMLElement::CopyInnerTo(aDest);
>   NS_ENSURE_SUCCESS(rv, rv);
>   if (aDest->OwnerDoc()->IsStaticDocument()) {
>     nsHTMLCanvasElement* dest = static_cast<nsHTMLCanvasElement*>(aDest);
>+    nsHTMLCanvasElement* self = const_cast<nsHTMLCanvasElement*>(this);
>+
>+    // By default, we assume this canvas will be displayed in PrintPreview.
>+    dest->mIsPrintPreview = true;
>+    dest->mPrintCallback = self->mPrintCallback;
>+    dest->mOriginalCanvas = self;
>+
>+    // Mark the canvas to be a "PrintCanvas" in case there is a special
>+    // rendering callback for printing.
>+    if (dest->mPrintCallback) {
>+      dest->mIsPrintCanvas = true;
>+    }
So why do we need mIsPrintCanvas?
It it ever true when mPrintCallback is false?


>+[scriptable, uuid(8d5fb8a0-7782-11e1-b0c4-0800200c9a67)]
Make this builtinclass.

>+interface nsIDOMMozCanvasPrintState : nsISupports {
{ to next line

>+  readonly attribute nsISupports context;
Hmm, what is this object? At least needs some documentation.


>+
>+  void done();
>+};
>+
>+[scriptable, function, uuid(8d5fb8a0-7782-11e1-b0c4-0800200c9a66)]
>+interface nsIPrintCallback : nsISupports {
ditto
Attachment #642703 - Flags: review?(bugs) → review-
Olli: Since I didn't write most of this patch I can't give complete answers to
some of your questions, but if further clarifcation is needed I can write Julian.

(In reply to Olli Pettay [:smaug] from comment #45)
> Comment on attachment 642703 [details] [diff] [review]
> Part 1 - DOM - mozPrintCallback and MozCanvasPrintState
> 
> > nsCanvasRenderingContext2D::GetCanvas(nsIDOMHTMLCanvasElement **canvas)
> > {
> >-    NS_IF_ADDREF(*canvas = mCanvasElement);
> >+    NS_IF_ADDREF(*canvas = mCanvasElement->GetOriginalCanvas());
> > 
> You should null check mCanvasElement
> 

Done

> 
> >+++ b/content/html/content/src/nsHTMLCanvasElement.cpp
> >@@ -24,51 +24,151 @@
> > #include "mozilla/Preferences.h"
> > #include "mozilla/Telemetry.h"
> > 
> > #include "nsFrameManager.h"
> > #include "nsDisplayList.h"
> > #include "ImageLayers.h"
> > #include "BasicLayers.h"
> > #include "imgIEncoder.h"
> >+#include "nsITimer.h"
> >+#include "nsAsyncDOMEvent.h"
> > 
> > #include "nsIWritablePropertyBag2.h"
> > 
> >+#include "nsIDOMHTMLCanvasElement.h"
> >+
> You really need to #include this here?
> 

Removed

> 
> >+#define NS_ICANVASPRINTSTATE_IID \
> >+   {0x8d5fb8a0, 0x7782, 0x11e1, { 0xb0, 0xc4, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x67 }} 
> Why do you need this?

Not sure, removed, seems fine.

> 
> >+
> >+class nsHTMLCanvasPrintState : public nsIDOMMozCanvasPrintState
> >+{
> >+public:
> >+  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ICANVASPRINTSTATE_IID)
> >+
> >+
> >+  nsHTMLCanvasPrintState(
> >+      nsHTMLCanvasElement *aCanvas,
> >+      nsICanvasRenderingContextInternal *aContext, 
> >+      nsITimerCallback *aCallback)
> >+  : mIsDone(false), mPendingNotify(false),
> >+    mCanvas(aCanvas), mContext(aContext), mCallback(aCallback) {
> { should be in the next line

Done

> 
> > nsHTMLCanvasElement::nsHTMLCanvasElement(already_AddRefed<nsINodeInfo> aNodeInfo)
> >-  : nsGenericHTMLElement(aNodeInfo), mWriteOnly(false)
> >-{
> >+  : nsGenericHTMLElement(aNodeInfo), 
> >+    mOriginalCanvas(nsnull), mPrintState(nsnull), mWriteOnly(false),
> >+    mIsPrintCanvas(false), mIsPrintPreview(false) {
> > }
> No need to initialize nsCOMPtr member variables to nsnull
>

Removed

> 
> >+nsHTMLCanvasElement::GetOriginalCanvas()
> >+{
> >+  if (mOriginalCanvas == nsnull) {
> >+    return this;
> >+  } else {
> >+    return mOriginalCanvas.get();
> >+  }
> >+}
> return mOriginalCanvas ? mOriginalCanvas.get() : this;

Done

> 
> > nsHTMLCanvasElement::CopyInnerTo(nsGenericElement* aDest)
> > {
> >   nsresult rv = nsGenericHTMLElement::CopyInnerTo(aDest);
> >   NS_ENSURE_SUCCESS(rv, rv);
> >   if (aDest->OwnerDoc()->IsStaticDocument()) {
> >     nsHTMLCanvasElement* dest = static_cast<nsHTMLCanvasElement*>(aDest);
> >+    nsHTMLCanvasElement* self = const_cast<nsHTMLCanvasElement*>(this);
> >+
> >+    // By default, we assume this canvas will be displayed in PrintPreview.
> >+    dest->mIsPrintPreview = true;
> >+    dest->mPrintCallback = self->mPrintCallback;
> >+    dest->mOriginalCanvas = self;
> >+
> >+    // Mark the canvas to be a "PrintCanvas" in case there is a special
> >+    // rendering callback for printing.
> >+    if (dest->mPrintCallback) {
> >+      dest->mIsPrintCanvas = true;
> >+    }
> So why do we need mIsPrintCanvas?
> It it ever true when mPrintCallback is false?

No, this was an uneeded variable.  I've redone this, but it required making mPrintCallback public, which is hopefully acceptable.

> 
> >+[scriptable, uuid(8d5fb8a0-7782-11e1-b0c4-0800200c9a67)]
> Make this builtinclass.
>

I assume you mean add builtinclass, since it also needs to be scriptable.

> >+interface nsIDOMMozCanvasPrintState : nsISupports {
> { to next line

Done

> 
> >+  readonly attribute nsISupports context;
> Hmm, what is this object? At least needs some documentation.

Added comment.

> 
> 
> >+
> >+  void done();
> >+};
> >+
> >+[scriptable, function, uuid(8d5fb8a0-7782-11e1-b0c4-0800200c9a66)]
> >+interface nsIPrintCallback : nsISupports {
> ditto

Done
Attachment #642703 - Attachment is obsolete: true
Attachment #644438 - Flags: review?(bugs)
Some of the layout code snuck into the last patch, moving it back to part 1.
Attachment #643201 - Attachment is obsolete: true
How is the callback called, or when? I think it should happen between beforeprint and afterprint events.
mozPrintCallback is currently called after the afterprint event.  Do you have any suggestions on how to move it in between? 
It looks like the events are triggered in http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#468 and that object is created down in http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#3605

I see two ways:
1) Trigger the mozPrintCallbacks to earlier somewhere in nsPrintEngine maybe in DoCommonPrint. That's probably bad though since it should happen async when the page print timer is created.
2) Delay calling afterprint until we get a notification back from the print engine that all the mozPrintCallbacks have been called.

Thoughts?
We can't change afterprint behavior.
It is defined in http://www.whatwg.org/specs/web-apps/current-work/#printing and implemented
in several browser engines.

So, perhaps callbacks need to be called after afterprint after all.
But should we let the page to know when all the canvas elements have been printed?
How does the API behave when one changes settings in print preview, like from
landscape to portrait? I assume the callbacks are called again?
(In reply to Olli Pettay [:smaug] from comment #50)
> We can't change afterprint behavior.
> It is defined in http://www.whatwg.org/specs/web-apps/current-work/#printing
> and implemented
> in several browser engines.
> 
> So, perhaps callbacks need to be called after afterprint after all.
> But should we let the page to know when all the canvas elements have been
> printed?

Or if the user wants to know when all the canvas elements have finished printing they could easily keep track of a counter themselves since they will be adding a mozPrintCallback to each.

(In reply to Olli Pettay [:smaug] from comment #51)
> How does the API behave when one changes settings in print preview, like from
> landscape to portrait? I assume the callbacks are called again?

Yes the mozPrintCallbacks are called again.
(In reply to Brendan Dahl from comment #52)
> Or if the user wants to know when all the canvas elements have finished
> printing they could easily keep track of a counter themselves since they
> will be adding a mozPrintCallback to each.
> 
printing/print preview can be cancelled, so not all the callbacks get always called, right?
(In reply to Olli Pettay [:smaug] from comment #53)
> printing/print preview can be cancelled, so not all the callbacks get always
> called, right?
Correct, though cancelling printing is broken right now (bug 761008). Is this something we need to add for this bug or can we create a subsequent bug to do that?
We're adding a new API here, and exposing it to web, so yes, we need to be a bit careful what the
API looks like.
What are your thoughts on:

1) The event name (maybe afterprintcallbacks, printend, or ...)?

2) Is the event triggered if there are no mozPrintCallbacks?

You also now have me questioning whether it should be called mozPrintCallback since no other dom callbacks use the word 'callback' in there name.
(In reply to Brendan Dahl from comment #54)
> Correct, though cancelling printing is broken right now (bug 761008). 
I can certainly cancel printing on linux.
Comment on attachment 644438 [details] [diff] [review]
Part 1 - DOM - mozPrintCallback and MozCanvasPrintState


>+nsHTMLCanvasElement::CallPrintCallback(nsITimerCallback *aCallback)
>+{
>+  mPrintState = new nsHTMLCanvasPrintState(this, mCurrentContext, aCallback);
>+  mPrintCallback->Render(mPrintState);
You should push JS context to stack before calling JS implemented callbacks
(unless you're sure the callback is called only asynchronously from event loop in which case XPConnect push the stuff to stack.)
nsCxPusher pusher;
NS_ENSURE_STATE(pusher.Push(originalCanvasElement));
mPrintState = new nsHTMLCanvasPrintState(this, mCurrentContext, aCallback);
mPrintCallback->Render(mPrintState);



> nsresult
> nsHTMLCanvasElement::CopyInnerTo(nsGenericElement* aDest)
> {
>   nsresult rv = nsGenericHTMLElement::CopyInnerTo(aDest);
>   NS_ENSURE_SUCCESS(rv, rv);
>   if (aDest->OwnerDoc()->IsStaticDocument()) {
>     nsHTMLCanvasElement* dest = static_cast<nsHTMLCanvasElement*>(aDest);
>+    nsHTMLCanvasElement* self = const_cast<nsHTMLCanvasElement*>(this);
>+
>+    // By default, we assume this canvas will be displayed in PrintPreview.
>+    dest->mIsPrintPreview = true;
>+    dest->mPrintCallback = self->mPrintCallback;
This doesn't look right. aDest should get callback from the original just when calling the callback

Fix those and I'll r+ this, but we need still something to indicate that printing the canvas elements is done
(so that web app can for example remove those elements from document if it needs to).
Attachment #644438 - Flags: review?(bugs) → review-
Comment on attachment 644440 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback

>+
>+        // Start the rendering process.
>+        canvas->CallPrintCallback(aCallback);
So after the call 'this' may point to a deleted object if the callback happens to
do something evil. window.close() might be enough, Or if not, chrome code certainly could delete the print presentation.
nsWeakFrame weakFrame = this;
canvas->CallPrintCallback(aCallback);
NS_ENSURE_STATE(weakFrame.IsAlive()); should be good enough protection
Does firing printingcomplete event after the printing is truly complete sound ok?
It should be fired on the set event target as beforeprint/afterprint.
Attachment #644438 - Attachment is obsolete: true
Attachment #648767 - Flags: review?(bugs)
Roc: from smaug's feedback I added the new mozprintcomplete which is in layout code. I also added back the mIsPrintPreview flag as I misinterpreted some of the logic.  There is a case when mIsPrintPreview is false and mPrintCallback is true (a regular canvas).  I'm not sure if you wanted to glance at this again.
Attachment #644440 - Attachment is obsolete: true
Attachment #648769 - Flags: review?(roc)
Missed virtual on destructor.
Attachment #648767 - Attachment is obsolete: true
Attachment #648767 - Flags: review?(bugs)
Attachment #648842 - Flags: review?(bugs)
Comment on attachment 648842 [details] [diff] [review]
Part 1 - DOM - mozPrintCallback and MozCanvasPrintState

> 
>   nsString mCurrentContextId;
>+  nsCOMPtr<nsIDOMHTMLCanvasElement> mOriginalCanvas;
>+  nsCOMPtr<nsIPrintCallback> mPrintCallback;
>   nsCOMPtr<nsICanvasRenderingContextInternal> mCurrentContext;
>+  nsCOMPtr<nsHTMLCanvasPrintState> mPrintState;
>   
> public:
>   // Record whether this canvas should be write-only or not.
>   // We set this when script paints an image from a different origin.
>   // We also transitively set it when script paints a canvas which
>   // is itself write-only.
>   bool                     mWriteOnly;
>+
>+  bool mIsPrintCanvas;
>+
>+  bool mIsPrintPreview;
Could you please document the new member variables.


>+  nsresult CallPrintCallback(nsITimerCallback *aCallback);
* goes with type

>+class nsHTMLCanvasPrintState : public nsIDOMMozCanvasPrintState
>+{
>+public:
>+  nsHTMLCanvasPrintState(
>+      nsHTMLCanvasElement *aCanvas,
>+      nsICanvasRenderingContextInternal *aContext, 
>+      nsITimerCallback *aCallback)
ditto, and 2 space indentation



>+  : mIsDone(false), mPendingNotify(false),
>+    mCanvas(aCanvas), mContext(aContext), mCallback(aCallback)
>+  {
>+  }
>+
>+  NS_IMETHOD GetContext(nsISupports **context)
nsISupports**

> nsHTMLCanvasElement::~nsHTMLCanvasElement()
> {
>+  ResetPrintCallback();
Is there any need for ResetPrintCallback here?
>-  bool forceThebes = false;
>+  // xxx force thebes if this is a print canvas.
>+  // Remove this once the Azure backend has the right now not implemented
>+  // functions for the printCanvas usecase ready.
>+  bool forceThebes = mIsPrintCanvas;
Bug filed for this case? Please add the bug number here.

>+[scriptable, builtinclass, uuid(8d5fb8a0-7782-11e1-b0c4-0800200c9a67)]
>+interface nsIDOMMozCanvasPrintState : nsISupports
>+{
>+  // A canvas rendering context.
>+  readonly attribute nsISupports context;
>+
>+  void done();
>+};
I hope done() handling has been tested, even in case it is called using a timeout. Mochitests should be provided.


> [scriptable, uuid(5929542B-C68E-48AB-84F9-D9642DA39720)]
> interface nsIDOMHTMLCanvasElement : nsIDOMHTMLElement
You need to update the uuid when changing an interface.


r=me, but I'm  expecting to see at least some tests.
Attachment #648842 - Flags: review?(bugs) → review+
Comment on attachment 648769 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback


> DocumentViewerImpl::SetIsPrintPreview(bool aIsPrintPreview)
> {
> #ifdef NS_PRINTING
>+  if (!aIsPrintPreview && mDocument && mDocument->IsStaticDocument()) {
>+    DispatchPrintComplete(mDocument->GetOriginalDocument());
>+  }
>   // Set all the docShells in the docshell tree to be printing.
>   // that way if anyone of them tries to "navigate" it can't
>   nsCOMPtr<nsIDocShellTreeNode> docShellTreeNode(do_QueryReferent(mContainer));
>   if (docShellTreeNode || !aIsPrintPreview) {
>     SetIsPrintingInDocShellTree(docShellTreeNode, aIsPrintPreview, true);
>   }
Since dispatching events can destroy the worlds, please do it as the last step in the method,
if possible.


...and I see this patch has some tests :)
(In reply to Olli Pettay [:smaug] from comment #50)
> We can't change afterprint behavior.
> It is defined in http://www.whatwg.org/specs/web-apps/current-work/#printing
> and implemented
> in several browser engines.
> 
> So, perhaps callbacks need to be called after afterprint after all.
> But should we let the page to know when all the canvas elements have been
> printed?

I don't see anything in the spec that would prevent us from delaying afterprint until all callbacks have fired. Why can't we do that instead of adding mozprintcomplete?
afterprint is dispatched synchronously right before window.print() returns.
Here we do something very much asynchronous.
Can't we dispatch afterprint synchronously if there are no mozPrintCallbacks pending? That way the old behavior would be preserved, and we wouldn't have to introduce two event names for the same thing, which I'd find confusing if I didn't know the history above.
Correction: Two event names for *almost* the thing... 

Point is having both `afterprint` and `printingcomplete` would be confusing, and I can't think of a use case where both are required.

roc?
Well, afterprint means that document has been pushed to browser's printing pipeline (whatever that is),
printingcomplete should be indicating that browser has done with printing.

Although dispatching afterprint in some cases asynchronously and in some cases synchronously
would be quite odd API, I think I could live with it.
(In reply to Olli Pettay [:smaug] from comment #64)
> Comment on attachment 648842 [details] [diff] [review]
> Part 1 - DOM - mozPrintCallback and MozCanvasPrintState
> 
> > 
> >   nsString mCurrentContextId;
> >+  nsCOMPtr<nsIDOMHTMLCanvasElement> mOriginalCanvas;
> >+  nsCOMPtr<nsIPrintCallback> mPrintCallback;
> >   nsCOMPtr<nsICanvasRenderingContextInternal> mCurrentContext;
> >+  nsCOMPtr<nsHTMLCanvasPrintState> mPrintState;
> >   
> > public:
> >   // Record whether this canvas should be write-only or not.
> >   // We set this when script paints an image from a different origin.
> >   // We also transitively set it when script paints a canvas which
> >   // is itself write-only.
> >   bool                     mWriteOnly;
> >+
> >+  bool mIsPrintCanvas;
> >+
> >+  bool mIsPrintPreview;
> Could you please document the new member variables.
> 

Done

> 
> >+  nsresult CallPrintCallback(nsITimerCallback *aCallback);
> * goes with type

Done

> >+class nsHTMLCanvasPrintState : public nsIDOMMozCanvasPrintState
> >+{
> >+public:
> >+  nsHTMLCanvasPrintState(
> >+      nsHTMLCanvasElement *aCanvas,
> >+      nsICanvasRenderingContextInternal *aContext, 
> >+      nsITimerCallback *aCallback)
> ditto, and 2 space indentation
> 

Done

> 
> 
> >+  : mIsDone(false), mPendingNotify(false),
> >+    mCanvas(aCanvas), mContext(aContext), mCallback(aCallback)
> >+  {
> >+  }
> >+
> >+  NS_IMETHOD GetContext(nsISupports **context)
> nsISupports**

Done

> > nsHTMLCanvasElement::~nsHTMLCanvasElement()
> > {
> >+  ResetPrintCallback();
> Is there any need for ResetPrintCallback here?

No, removed.

> >-  bool forceThebes = false;
> >+  // xxx force thebes if this is a print canvas.
> >+  // Remove this once the Azure backend has the right now not implemented
> >+  // functions for the printCanvas usecase ready.
> >+  bool forceThebes = mIsPrintCanvas;
> Bug filed for this case? Please add the bug number here.

This has been removed after talking with Julian.

> >+[scriptable, builtinclass, uuid(8d5fb8a0-7782-11e1-b0c4-0800200c9a67)]
> >+interface nsIDOMMozCanvasPrintState : nsISupports
> >+{
> >+  // A canvas rendering context.
> >+  readonly attribute nsISupports context;
> >+
> >+  void done();
> >+};
> I hope done() handling has been tested, even in case it is called using a
> timeout. Mochitests should be provided.

As noted test's are in other patch.

> 
> > [scriptable, uuid(5929542B-C68E-48AB-84F9-D9642DA39720)]
> > interface nsIDOMHTMLCanvasElement : nsIDOMHTMLElement
> You need to update the uuid when changing an interface.

Done

> > DocumentViewerImpl::SetIsPrintPreview(bool aIsPrintPreview)
> > {
> > #ifdef NS_PRINTING
> >+  if (!aIsPrintPreview && mDocument && mDocument->IsStaticDocument()) {
> >+    DispatchPrintComplete(mDocument->GetOriginalDocument());
> >+  }
> >   // Set all the docShells in the docshell tree to be printing.
> >   // that way if anyone of them tries to "navigate" it can't
> >   nsCOMPtr<nsIDocShellTreeNode> docShellTreeNode(do_QueryReferent(mContainer));
> >   if (docShellTreeNode || !aIsPrintPreview) {
> >     SetIsPrintingInDocShellTree(docShellTreeNode, aIsPrintPreview, true);
> >   }
> Since dispatching events can destroy the worlds, please do it as the last step in the method,
> if possible.

Done

I also changed it around the the afterprint event is delayed if there are any mozprint callback canvases.
Attachment #648842 - Attachment is obsolete: true
Attachment #654870 - Flags: review?(bugs)
Attachment #648769 - Attachment is obsolete: true
Changing the print reftests to create a static clone before printing ended up causing too many issues.  Instead of marking the canvases during the static clone operation I now just check with the press context what the layout mode is.  This ends up getting rid of the two flags we added to canvas and I think makes things easier to understand.
No longer depends on: 774423
Sorry for the bug spam, but I forgot the try run link:
https://tbpl.mozilla.org/?tree=Try&rev=271353863cff
Comment on attachment 654870 [details] [diff] [review]
Part 1 - DOM - mozPrintCallback and MozCanvasPrintState

Sorry, I think we need to make sure to either not use azure canvas
for printing or make sure that it works.
Attachment #654870 - Flags: review?(bugs) → review-
Comment on attachment 654870 [details] [diff] [review]
Part 1 - DOM - mozPrintCallback and MozCanvasPrintState


> NS_IMETHODIMP
> nsCanvasRenderingContext2D::GetCanvas(nsIDOMHTMLCanvasElement **canvas)
> {
>-    NS_IF_ADDREF(*canvas = mCanvasElement);
>+    if (mCanvasElement) {
>+      NS_IF_ADDREF(*canvas = mCanvasElement->GetOriginalCanvas());
>+    }
> 
>     return NS_OK;
> }
*Azure needs to have this too.


>   nsString mCurrentContextId;
>+  nsCOMPtr<nsIDOMHTMLCanvasElement> mOriginalCanvas;
>+  nsCOMPtr<nsIPrintCallback> mPrintCallback;
>   nsCOMPtr<nsICanvasRenderingContextInternal> mCurrentContext;
>+  nsCOMPtr<nsHTMLCanvasPrintState> mPrintState;
>   
> public:
>   // Record whether this canvas should be write-only or not.
>   // We set this when script paints an image from a different origin.
>   // We also transitively set it when script paints a canvas which
>   // is itself write-only.
>   bool                     mWriteOnly;
>+
>+  bool IsPrintCallbackDone();
>+
>+  void HandlePrintCallback(nsPresContext::nsPresContextType);
You should add parameter name


>+class nsHTMLCanvasPrintState : public nsIDOMMozCanvasPrintState
>+{
>+public:
>+  nsHTMLCanvasPrintState(nsHTMLCanvasElement* aCanvas,
>+                         nsICanvasRenderingContextInternal* aContext,
>+                         nsITimerCallback* aCallback)
>+    : mIsDone(false), mPendingNotify(false), mCanvas(aCanvas),
>+      mContext(aContext), mCallback(aCallback)
>+  {
>+  }
>+
>+  NS_IMETHOD GetContext(nsISupports** context)
s/context/aContext/

>+nsHTMLCanvasElement::CallPrintCallback(nsITimerCallback *aCallback)
* goes with the type, not with parameter. nsITimerCallback*


+void
+nsHTMLCanvasElement::HandlePrintCallback(nsPresContext::nsPresContextType aType)
+{
+  // Only call the print callback here if 1) we're in a print testing mode or
+  // print preview mode, 2) the canvas has a print callback and 3) the callback
+  // hasn't already been called. For real printing the callback is handled in
+  // nsSimplePageSequenceFrame::PrePrintNextPage.
+  nsCOMPtr<nsIPrintCallback> printCallback;
+  if ((aType == nsPresContext::eContext_PageLayout ||
+       aType == nsPresContext::eContext_PrintPreview) &&
+      !mPrintState &&
+      NS_SUCCEEDED(GetMozPrintCallback(getter_AddRefs(printCallback))) && printCallback) {
+    CallPrintCallback(nullptr);
+    return;
Useless return;
Addresses nits and fixes azure's GetCanvas.
Attachment #654870 - Attachment is obsolete: true
Attachment #655229 - Flags: review?(bugs)
Attachment #655229 - Attachment is patch: true
Moved around some more asterisks in the wrong spot for pointers.
Attachment #654871 - Attachment is obsolete: true
Attachment #655229 - Flags: review?(bugs) → review+
Addressed feedback on event and nullptr check.
Attachment #655230 - Attachment is obsolete: true
Attachment #655257 - Flags: review?(bugs)
Comment on attachment 655257 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback


>diff --git a/layout/generic/nsHTMLCanvasFrame.cpp b/layout/generic/nsHTMLCanvasFrame.cpp
>--- a/layout/generic/nsHTMLCanvasFrame.cpp
>+++ b/layout/generic/nsHTMLCanvasFrame.cpp
>@@ -255,26 +255,28 @@ already_AddRefed<Layer>
> nsHTMLCanvasFrame::BuildLayer(nsDisplayListBuilder* aBuilder,
>                               LayerManager* aManager,
>                               nsDisplayItem* aItem)
> {
>   nsRect area = GetContentRect() - GetPosition() + aItem->ToReferenceFrame();
>   nsHTMLCanvasElement* element = static_cast<nsHTMLCanvasElement*>(GetContent());
>   nsIntSize canvasSize = GetCanvasSize();
> 
>+  nsPresContext* presContext = PresContext();
>+  element->HandlePrintCallback(presContext->Type());
>+
This it not safe in any way.
You call a callback while keeping only raw reference to the element, and
the callback must not be called when building layers, AFAIK. (roc knows more about layers)

And there are so many layout changes comparing to the previous patch that it would be better if roc reviewed those.
Attachment #655257 - Flags: review?(bugs) → review-
Comment on attachment 655257 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback

(In reply to Olli Pettay [:smaug] from comment #81)
> Comment on attachment 655257 [details] [diff] [review]
> Part 2 - Layout - Printing support for mozPrintCallback
> 
> 
> >diff --git a/layout/generic/nsHTMLCanvasFrame.cpp b/layout/generic/nsHTMLCanvasFrame.cpp
> >--- a/layout/generic/nsHTMLCanvasFrame.cpp
> >+++ b/layout/generic/nsHTMLCanvasFrame.cpp
> >@@ -255,26 +255,28 @@ already_AddRefed<Layer>
> > nsHTMLCanvasFrame::BuildLayer(nsDisplayListBuilder* aBuilder,
> >                               LayerManager* aManager,
> >                               nsDisplayItem* aItem)
> > {
> >   nsRect area = GetContentRect() - GetPosition() + aItem->ToReferenceFrame();
> >   nsHTMLCanvasElement* element = static_cast<nsHTMLCanvasElement*>(GetContent());
> >   nsIntSize canvasSize = GetCanvasSize();
> > 
> >+  nsPresContext* presContext = PresContext();
> >+  element->HandlePrintCallback(presContext->Type());
> >+
> This it not safe in any way.
> You call a callback while keeping only raw reference to the element, and
> the callback must not be called when building layers, AFAIK. (roc knows more
> about layers)
> 
> And there are so many layout changes comparing to the previous patch that it
> would be better if roc reviewed those.

This is how it was before I started working on the patch.  Where should this be handled?

Re-requesting a review...
Attachment #655257 - Flags: review- → review?(roc)
Comment on attachment 655257 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback

Review of attachment 655257 [details] [diff] [review]:
-----------------------------------------------------------------

How does mBeforeAndAfterPrint get cleared (and afterprint fired) when we have mozPrintCallbacks and they've finished running?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #83)
> Comment on attachment 655257 [details] [diff] [review]
> Part 2 - Layout - Printing support for mozPrintCallback
> 
> Review of attachment 655257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How does mBeforeAndAfterPrint get cleared (and afterprint fired) when we
> have mozPrintCallbacks and they've finished running?

That happens in SetIsPrinting and SetIsPrintPreview when aIsPrinting/aIsPreview is false.

https://bugzilla.mozilla.org/attachment.cgi?id=655257&action=diff#a/layout/base/nsDocumentViewer.cpp_sec9

roc: Also, could you look at the comment in #81 and let me know if I need to change that.
(In reply to Brendan Dahl from comment #84)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #83)
> > Comment on attachment 655257 [details] [diff] [review]
> > Part 2 - Layout - Printing support for mozPrintCallback
> > 
> > Review of attachment 655257 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > How does mBeforeAndAfterPrint get cleared (and afterprint fired) when we
> > have mozPrintCallbacks and they've finished running?
> 
> That happens in SetIsPrinting and SetIsPrintPreview when
> aIsPrinting/aIsPreview is false.

I believe SetIsPrintPreview(false) only happens when we exit print preview. But I guess it's OK to delay firing afterprint until then.

> roc: Also, could you look at the comment in #81 and let me know if I need to
> change that.

I think nsHTMLCanvasElement::CallPrintCallback instead of doing a direct call should dispatch the callback asynchronously. So instead of calling Render(), dispatch an XPCOM event whose Run() method calls Render(). So you should rename that method to DispatchPrintCallback. Then you don't need the nsCxPusher stuff.
Comment on attachment 655257 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback

Review of attachment 655257 [details] [diff] [review]:
-----------------------------------------------------------------

How does mBeforeAndAfterPrint get cleared (and afterprint fired) when we have mozPrintCallbacks and they've finished running?
Attachment #655257 - Flags: review?(roc) → review+
Comment on attachment 655229 [details] [diff] [review]
Part 1 - DOM - mozPrintCallback and MozCanvasPrintState

Review of attachment 655229 [details] [diff] [review]:
-----------------------------------------------------------------

per my last comment
Attachment #655229 - Flags: review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #86)
> How does mBeforeAndAfterPrint get cleared (and afterprint fired) when we
> have mozPrintCallbacks and they've finished running?

Ignore this; Splinter fail.
Addresses review feedback and fixes iframes.
Attachment #655229 - Attachment is obsolete: true
Attachment #657085 - Flags: review?(bugs)
Links to the main changes both of which were gone over with Robert and/or Olli:
Runnable:
http://pastebin.mozilla.org/1786497

Iframe Fixes:
http://pastebin.mozilla.org/1786498
I had to invalidate the canvas to get it to work with print reftests when mozprintcallback was done.  Latest try is looking better: https://tbpl.mozilla.org/?tree=Try&rev=e694f6fa71e5
Attachment #657085 - Attachment is obsolete: true
Attachment #657085 - Flags: review?(bugs)
Attachment #657437 - Flags: review?(bugs)
Attachment #657437 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Comment on attachment 657086 [details] [diff] [review]
Part 2 - Layout - Printing support for mozPrintCallback

https://hg.mozilla.org/integration/mozilla-inbound/rev/206e0ad324c9

(Carrying forward roc's review, as this patch updates the one he r+ed based on further discussion between bdahl and him.)
Attachment #657086 - Flags: review+
Attachment #657086 - Flags: checkin+
Assignee: jviereck.dev → bdahl
Flags: in-testsuite+
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla18
Depends on: 788639
Depends on: 789507
Depends on: 791466
Depends on: 791480
Depends on: 793844
Depends on: 800535
Whiteboard: [DocArea=API]
Depends on: 1308579
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: