Markus Stange [:mstange] Assignee
	Description • 14 years ago

I need this for -moz-element, bug 506826.

Currently our image drawing works like this:
 1. DrawImageInternal in nsLayoutUtils.cpp is called with an imgIContainer.
 2. It calculates transforms and fill rects using the image snapping algorithm.
 3. The results of this algorithm are passed to imgContainer::Draw, which passes them on to imgFrame::Draw.
 4. imgFrame::Draw does this:
   4a. Single color? Fill and return.
   4b. For partial images, construct a new gfxASurface with the partial image and transparent margin, or, in the non-repeating case, use the existing surface and tweak the transforms and rects.
   4c. Work around all kinds of bugs.
   4d. Draw.

So this all only works with imgIContainers, and inside imgFrame::Draw only with gfxASurfaces. However, in bug 506826 I need these things for all kinds of different drawable things:
 - gfxASurfaces (e.g. for out-of-document videos)
 - gfxPatterns (for SVG gradients and patterns)
 - drawing callbacks (for rendering a different element directly without temporary surface)

	Markus Stange [:mstange] Assignee
	Comment 2 • 14 years ago

The next part will add a method to gfxUtils that takes a gfxPattern::GraphicsFilter parameter. So I need to include gfxPattern.h in gfxUtils.h. However, since gfxMatrix.h includes gfxUtils.h and gfxPattern.h includes gfxMatrix.h, this doesn't work.
So I need to make gfxMatrix.h stop including gfxUtils.h. gfxMatrix.h only needed gfxUtils.h for its "FuzzyEqual" method, and that method isn't used anywhere else, so I'm making it a private method of gfxMatrix.

	Markus Stange [:mstange] Assignee
	Comment 3 • 14 years ago

This moves 4a in its own method in imgFrame.cpp ("DoSingleColorFastPath"), 4b into another method in imgFrame.cpp ("SurfaceForDrawing"), and 4c and 4d into gfxUtils::DrawPixelSnapped.
It's a big patch, but it's only moving code and hopefully doesn't cause any functionality differences. It passes tests.

I put the cairo / pixman bug workaround from bug 364968 in the constructor and destructor of a class because I think that makes it clearer: If the workaround pushed a group, it needs to pop it afterwards.

	Markus Stange [:mstange] Assignee
	Comment 4 • 14 years ago

gfxUtils::DrawPixelSnapped from part 3 can only draw gfxASurfaces. However, as I said in comment 0, I don't always want to create a temporary surface when we don't really need one. For example an SVG gradient that fills the whole box should just be able to paint the gradient gfxPattern directly, and still use the same code path as all other -moz-element drawing.

So I introduce a new interface: gfxDrawable. It represents something drawable that has an intrinsic integer size and can produce a gfxPattern.
I'm also adding three convenience implementations for gfxDrawable: gfxSurfaceDrawable, gfxPatternDrawable and gfxCallbackDrawable. The surface flavour will be used by imgFrame::Draw and out-of-document images and videos in -moz-element; the pattern flavour will be used by SVG paint servers in -moz-element; and the callback one by normal HTML element paint servers.

PreparePatternForUntiledDrawing does what SetExtendAndFilterOnPattern did in part 3; in part 5 I'll remove the latter.

I'm using 4-space-indent because that's what the rest of gfx seems to be doing.

	Markus Stange [:mstange] Assignee
	Comment 5 • 14 years ago

Straightforward.

	Markus Stange [:mstange] Assignee
	Comment 6 • 14 years ago

This will be used for all -moz-element drawing. It just runs the image snapping algorithm and pushes the result through gfxUtils::DrawPixelSnapped. gfxUtils.h has a comment why we need both and what the difference is.

	Joe Drew (not getting mail)
	Comment 7 • 14 years ago

Comment on attachment 451940 [details] [diff] [review]
part 3: create gfxUtils::DrawPixelSnapped and make imgFrame use it

In general, I really like the way this patch is going, but it has a couple of implementation issues.

>diff --git a/gfx/thebes/public/gfxUtils.h b/gfx/thebes/public/gfxUtils.h

>+     * Draw a surface while working around limitations like bad support
>+     * for EXTEND_PAD, lack of source-clipping, or cairo / pixman bugs with
>+     * extreme user-space-to-image-space transforms.
>+     *
>+     * The input paramaters here usually come from the output of our image
                   ^parameters

>diff --git a/gfx/thebes/src/gfxUtils.cpp b/gfx/thebes/src/gfxUtils.cpp

>+// EXTEND_PAD won't help us here; we have to create a temporary surface to hold
>+// the subimage of pixels we're allowed to sample.
>+static gfxPattern*
>+CreateSamplingRestrictedPattern(gfxASurface* aSurface,
>+                                gfxContext* aContext,
>+                                const gfxMatrix& aUserSpaceToImageSpace,
>+                                const gfxRect& aSourceRect,
>+                                const gfxRect& aSubimage,
>+                                const gfxImageSurface::gfxImageFormat aFormat)

This has got leaks written all over it. Better return an already_AddRefed<gfxPattern>, and...


>+    gfxPattern* resultPattern = new gfxPattern(temp);
>+    if (!resultPattern)
>+        return nsnull;

...here, use a nsRefPtr<gfxPattern>, and...

>+
>+    resultPattern->SetMatrix(
>+        gfxMatrix(aUserSpaceToImageSpace).Multiply(gfxMatrix().Translate(-needed.pos)));
>+    return resultPattern;

... here, return resultPattern.forget().

>+// working around cairo/pixman bug (bug 364968)
>+// Our device-space-to-image-space transform may not be acceptable to pixman.
>+struct AutoCairoPixmanBugWorkaround {

This should be an NS_STACK_CLASS.

>diff --git a/modules/libpr0n/src/imgFrame.cpp b/modules/libpr0n/src/imgFrame.cpp
>--- a/modules/libpr0n/src/imgFrame.cpp
>+++ b/modules/libpr0n/src/imgFrame.cpp
>@@ -39,16 +39,17 @@
> #include "imgFrame.h"
> 
> #include <limits.h>
> 
> #include "prmem.h"
> #include "prenv.h"
> 
> #include "gfxPlatform.h"
>+#include "gfxUtils.h"
> 
> static PRBool gDisableOptimize = PR_FALSE;
> 
> /*XXX get CAIRO_HAS_DDRAW_SURFACE */
> #include "cairo.h"
> 
> #ifdef CAIRO_HAS_DDRAW_SURFACE
> #include "gfxDDrawSurface.h"
>@@ -370,288 +371,133 @@ nsresult imgFrame::Optimize()
> #ifdef XP_MACOSX
>     mQuartzSurface = nsnull;
> #endif
>   }
> 
>   return NS_OK;
> }
> 
>-static PRBool                                                                                                       
>-IsSafeImageTransformComponent(gfxFloat aValue)
>+static void
>+DoSingleColorFastPath(gfxContext*    aContext,
>+                      const gfxRGBA& aSinglePixelColor,
>+                      const gfxRect& aFill)
> {
>-  return aValue >= -32768 && aValue <= 32767;
>+  // if a == 0, it's a noop
>+  if (aSinglePixelColor.a == 0.0)
>+    return;
>+
>+  gfxContext::GraphicsOperator op = aContext->CurrentOperator();
>+  if (op == gfxContext::OPERATOR_OVER && aSinglePixelColor.a == 1.0) {
>+    aContext->SetOperator(gfxContext::OPERATOR_SOURCE);
>+  }
>+
>+  aContext->SetDeviceColor(aSinglePixelColor);
>+  aContext->NewPath();
>+  aContext->Rectangle(aFill);
>+  aContext->Fill();
>+  aContext->SetOperator(op);
>+  aContext->SetDeviceColor(gfxRGBA(0,0,0,0));
>+}
>+
>+imgFrame::SurfaceWithFormat
>+imgFrame::SurfaceForDrawing(PRBool             aDoPadding,

>+    gfxASurface* surface =
>+      gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format).get();

This should be an nsRefPtr<gfxASurface>.

>+  if (surfaceResult.mContinue) {

Change this as mentioned below.

>+    gfxUtils::DrawPixelSnapped(aContext, surfaceResult.mSurface,
>+                               userSpaceToImageSpace,
>+                               subimage, sourceRect, imageRect, fill,
>+                               surfaceResult.mFormat, aFilter);
>   }

>diff --git a/modules/libpr0n/src/imgFrame.h b/modules/libpr0n/src/imgFrame.h
>+  struct SurfaceWithFormat {
>+    PRBool mContinue;
>+    gfxASurface* mSurface;

This should be an nsRefPtr<gfxASurface>. And mContinue should be removed; instead, add a method here called IsValid(), which just checks to see if mSurface is non-null.

	Joe Drew (not getting mail)
	Comment 8 • 14 years ago

Comment on attachment 451943 [details] [diff] [review]
part 4: create gfxDrawable


>diff --git a/gfx/thebes/public/gfxDrawable.h b/gfx/thebes/public/gfxDrawable.h

>+/**
>+ * gfxDrawingCallback
>+ * An Interface representing something that can draw itself.
>+ */
>+class THEBES_API gfxDrawingCallback {
>+    NS_INLINE_DECL_REFCOUNTING(gfxDrawingCallback)
>+public:
>+    virtual ~gfxDrawingCallback() {}
>+
>+    /**
>+     * Draw into aContext filling aFillRect, possibly repeating, using aFilter.
>+     * aTransform is a userspace to "image"space matrix. For example, if Draw
>+     * draws using a gfxPattern, this is the matrix that should be set on the
>+     * pattern prior to rendering it.
>+     *  @return whether drawing was successful
>+     */
>+    virtual PRBool Draw(gfxContext* aContext,
>+                        const gfxRect aFillRect,
>+                        const PRBool aRepeat,
>+                        const gfxPattern::GraphicsFilter& aFilter,
>+                        const gfxMatrix aTransform = gfxMatrix())=0;
>+
>+};
>+
>+/**
>+ * gfxDrawable
>+ * An Interface representing something that has an intrinsic size, can draw
>+ * itself and can generate a repeatable pattern.
>+ */
>+class THEBES_API gfxDrawable : public gfxDrawingCallback {

It's very odd to me that Drawables both _are_ callbacks, and also (in the case of gfxCallbackDrawable) can _have_ callbacks. I think what you should do here is combine gfxDrawable and gfxDrawingCallback into one class, and then make gfxCallbackDrawable take a separate functor (or, failing that, function pointer) as an argument.

	Joe Drew (not getting mail)
	Comment 9 • 14 years ago

Comment on attachment 451944 [details] [diff] [review]
part 5: use gfxDrawables for image drawing

This'll need changes that result from the earlier patches, but at first glance looks OK. I'm only minusing this because I want to look at it again once the other changes are made.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 14 years ago

Comment on attachment 451934 [details] [diff] [review]
part 1: pull image snapping algorithm out of DrawImageInternal

+// helper function to convert a nsRect to a gfxRect
+// borrowed from nsCSSRendering.cpp

move to nsLayoutUtils?

+  // Whether there's anything to draw at all
+  PRBool mShouldDraw;

PRPackedBool and move it down to mResetCTM (which should also be PRPackedBool)

+  // An image-space to device-space transform (translation and scale)
+  gfxMatrix mUserSpaceToImageSpace;

The comment doesn't match the name. I believe this transforms from either device space or user space (depending on mResetCTM) to image space?

Otherwise looks good.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 14 years ago

Why does gfxUtils::DrawPixelSnapped need to take an aFormat parameter? Can't you get the format from the surface?

By the way, I think the work you've done here is *fantastic*.

	Daniel Holbert [:dholbert]
	Comment 12 • 14 years ago

Comment on attachment 451934 [details] [diff] [review]
part 1: pull image snapping algorithm out of DrawImageInternal

From part 1:
>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>+  SnappedImageDrawingParameters(gfxMatrix aUserSpaceToImageSpace,
>+                                gfxRect   aFillRect,
>+                                nsIntRect aSubimage,
>+                                PRBool    aResetCTM)
>+   : mShouldDraw(PR_TRUE)
> [etc]

Those first three args should probably be const references, right? (to reduce unnecessary copying of large-ish data structures)

	Markus Stange [:mstange] Assignee
	Comment 13 • 14 years ago

(In reply to comment #10)
> (From update of attachment 451934 [details] [diff] [review])
> +// helper function to convert a nsRect to a gfxRect
> +// borrowed from nsCSSRendering.cpp
> 
> move to nsLayoutUtils?

That would mean that I have to prefix all invocations with nsLayouUtils:: and in many cases this means I have to wrap the line because of the 80-column restriction, and then I've lost the main advantage of having a helper in the first place. Should I do it anyway? I haven't done it in this patch.

> +  // Whether there's anything to draw at all
> +  PRBool mShouldDraw;
> 
> PRPackedBool and move it down to mResetCTM (which should also be PRPackedBool)

Done.

> +  // An image-space to device-space transform (translation and scale)
> +  gfxMatrix mUserSpaceToImageSpace;
> 
> The comment doesn't match the name. I believe this transforms from either
> device space or user space (depending on mResetCTM) to image space?

Yes I think that's correct. Fixed.

(In reply to comment #12)
> (From update of attachment 451934 [details] [diff] [review])
> From part 1:
> >diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
> >+  SnappedImageDrawingParameters(gfxMatrix aUserSpaceToImageSpace,
> >+                                gfxRect   aFillRect,
> >+                                nsIntRect aSubimage,
> >+                                PRBool    aResetCTM)
> >+   : mShouldDraw(PR_TRUE)
> > [etc]
> 
> Those first three args should probably be const references, right?

Done.

	Markus Stange [:mstange] Assignee
	Comment 15 • 14 years ago

(In reply to comment #8)
> (From update of attachment 451943 [details] [diff] [review])

> It's very odd to me that Drawables both _are_ callbacks, and also (in the case
> of gfxCallbackDrawable) can _have_ callbacks.

That allows gfxPatternDrawables to do this:
mCallbackDrawable = new gfxCallbackDrawable(this, mSize);
They can just pass themselves as the callback to a gfxCallbackDrawable and then delegate pattern creation and tiled drawing to it.

> I think what you should do here
> is combine gfxDrawable and gfxDrawingCallback into one class, and then make
> gfxCallbackDrawable take a separate functor

So I'd need a new functor base class. What should I call that class? gfxDrawingCallback? ;)

And I'd need an implementation of that functor that wraps a gfxDrawable so that I can do the magic for patterns that I described above.
All in all, this doesn't seem any simpler to me, but maybe I'm missing something?

	Markus Stange [:mstange] Assignee
	Comment 16 • 14 years ago

This update doesn't make the changes you requested, see previous comment.

But there was a different issue that caused tests to fail on Linux: PreparePatternForUntiledDrawing was never called on the pattern created in CreateSamplingRestrictedPattern because it's only called for gfxSurfaceDrawables and CreateSamplingRestrictedPattern used a gfxPatternDrawable. In order to make it possible for it to use a gfxSurfaceDrawable, I had to add an additional mTransform field to gfxSurfaceDrawable that gets applied to the gfxPattern that is created by when drawing. That's what this patch does.

	Markus Stange [:mstange] Assignee
	Comment 17 • 14 years ago

Updated for changes in part 3 and to use a gfxSurfaceDrawable in CreateSamplingRestrictedPattern.

	Markus Stange [:mstange] Assignee
	Comment 18 • 14 years ago

(In reply to comment #11)
> Why does gfxUtils::DrawPixelSnapped need to take an aFormat parameter? Can't
> you get the format from the surface?

I don't think I can. If I could then imgFrame wouldn't need its mFormat field, for example.
We could add a GetFormat() method to gfxASurface that returns ImageFormatUnknown by default and make those surface implementations that know their format override it to return the correct format. But I'd rather like to do that in a different bug after this.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 14 years ago

Hmm, I guess that's right.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 20 • 14 years ago

+                        const gfxRect aFillRect,
+                        const PRBool aRepeat,
+                        const gfxPattern::GraphicsFilter& aFilter,
+                        const gfxMatrix aTransform = gfxMatrix());

aFillRect should be a const gfxRect&. aTransform should be const gfxMatrix&. PRBool aRepeat need not be const.

	Markus Stange [:mstange] Assignee
	Comment 21 • 14 years ago

Comment on attachment 453031 [details] [diff] [review]
part 4, gfxDrawable

I've changed my mind and agree that splitting gfxDrawingCallback from gfxDrawable is a good idea. That way we can say gfxDrawingCallback only needs to be able to draw itself *once*, but gfxDrawable must also handle drawing *repeatedly*.

	Markus Stange [:mstange] Assignee
	Comment 22 • 14 years ago

Landed part 1 and 2:
http://hg.mozilla.org/mozilla-central/rev/7b4b7e59f093
http://hg.mozilla.org/mozilla-central/rev/8425474fc441

	Markus Stange [:mstange] Assignee
	Comment 23 • 14 years ago

I've called the functor base class gfxDrawingCallback. In gfxDrawable.cpp there's a class called DrawingCallbackFromDrawable that works as an adapter from gfxDrawable to gfxDrawingCallback and just passes false to aRepeat.

	Joe Drew (not getting mail)
	Comment 25 • 14 years ago

Comment on attachment 453029 [details] [diff] [review]
part 3, v2


>diff --git a/gfx/thebes/src/gfxUtils.cpp b/gfx/thebes/src/gfxUtils.cpp

>+// working around cairo/pixman bug (bug 364968)
>+// Our device-space-to-image-space transform may not be acceptable to pixman.
>+struct NS_STACK_CLASS AutoCairoPixmanBugWorkaround {

Put the { on its own line, please.

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

>+imgFrame::SurfaceForDrawing(PRBool             aDoPadding,

>+    nsRefPtr<gfxASurface> surface =
>+      gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format).get();

You don't need this .get() here.

Otherwise awesome.

	Joe Drew (not getting mail)
	Comment 26 • 14 years ago

Comment on attachment 455483 [details] [diff] [review]
part 4, gfxDrawable

General comment: please put braces for new classes on their own line:
class Foo
{
  // some stuff
};

>diff --git a/gfx/thebes/gfxDrawable.cpp b/gfx/thebes/gfxDrawable.cpp

>+gfxCallbackDrawable::Draw(gfxContext* aContext,
>+                          const gfxRect& aFillRect,
>+                          PRBool aRepeat,
>+                          const gfxPattern::GraphicsFilter& aFilter,
>+                          const gfxMatrix& aTransform)
>+{
>+    if (aRepeat) {
>+        MakeSurfaceDrawable(aFilter);

Here, we should say mSurfaceDrawable = MakeSurfaceDrawable(...), and have MakeSurfaceDrawable just return an already_AddRefed<gfxSurfaceDrawable>.

>+class DrawingCallbackFromDrawable : public gfxDrawingCallback {

>+    virtual PRBool operator()(gfxContext* aContext,
>+                              const gfxRect& aFillRect,
>+                              const gfxPattern::GraphicsFilter& aFilter,
>+                              const gfxMatrix& aTransform = gfxMatrix()) {

Add a newline before the brace here too.


>+gfxPatternDrawable::Draw(gfxContext* aContext,

>+    if (aRepeat) {
>+        // We can't use mPattern directly: We want our repeated tiles to have
>+        // the size mSize, which might not be the case in mPattern.
>+        nsRefPtr<gfxDrawable> cbDrawable = CallbackDrawable();
>+        return cbDrawable->Draw(aContext, aFillRect, PR_TRUE, aFilter,
>+                                aTransform);
>+    }

This feels wrong. I've traced the code, and it seems like all we do is eventually call Draw() on ourselves with aRepeat = PR_FALSE. If we can't handle repeat, we should just do so explicitly, instead of hiding it like this. I'd like it for CallbackDrawable() to be removed altogether.

>diff --git a/gfx/thebes/gfxDrawable.h b/gfx/thebes/gfxDrawable.h

>+/**
>+ * gfxDrawingCallback
>+ * A simple drawing functor.
>+ */
>+class THEBES_API gfxDrawingCallback {

Maybe move this functor definition to just above gfxCallbackDrawable?


>+    /**
>+     * Draw into aContext filling aFillRect, without tiling, using aFilter.
>+     * aTransform is a userspace to "image"space matrix. For example, if Draw
>+     * draws using a gfxPattern, this is the matrix that should be set on the
>+     * pattern prior to rendering it.
>+     *  @return whether drawing was successful
>+     */
>+    virtual PRBool operator()(gfxContext* aContext,
>+                              const gfxRect& aFillRect,
>+                              const gfxPattern::GraphicsFilter& aFilter,
>+                              const gfxMatrix& aTransform = gfxMatrix())=0;

Add spaces in here, "= 0;".

Also, I don't think it's necessary to say "without tiling" here, since it doesn't make any sense for a drawing callback, which is conceptually of infinite size, to tile.

>+
>+};
>+
>+/**
>+ * gfxDrawable
>+ * An Interface representing something that has an intrinsic size and can draw
>+ * itself repeatedly.
>+ */
>+class THEBES_API gfxDrawable {
>+    NS_INLINE_DECL_REFCOUNTING(gfxDrawable)
>+public:
>+    gfxDrawable(const gfxIntSize aSize)
>+     : mSize(aSize) {}
>+    virtual ~gfxDrawable() {}
>+
>+    /**
>+     * Draw into aContext filling aFillRect, possibly repeating, using aFilter.
>+     * aTransform is a userspace to "image"space matrix. For example, if Draw
>+     * draws using a gfxPattern, this is the matrix that should be set on the
>+     * pattern prior to rendering it.
>+     *  @return whether drawing was successful
>+     */
>+    virtual PRBool Draw(gfxContext* aContext,
>+                        const gfxRect& aFillRect,
>+                        PRBool aRepeat,
>+                        const gfxPattern::GraphicsFilter& aFilter,
>+                        const gfxMatrix& aTransform = gfxMatrix())=0;

Add spaces here, ") = 0;".


>+/**
>+ * gfxSurfaceDrawable
>+ * A convenience implementation of gfxDrawable for callbacks.
>+ */

Whoops, copy and paste error here - gfxCallbackDrawable, not gfxSurfaceDrawable.

r- because of the CallbackDrawable() stuff, but it's otherwise great -- exactly the changes I wanted in the earlier review!

	Joe Drew (not getting mail)
	Comment 27 • 14 years ago

Comment on attachment 455486 [details] [diff] [review]
part 5, updated to trunk


>diff --git a/gfx/thebes/gfxUtils.cpp b/gfx/thebes/gfxUtils.cpp

>+            nsRefPtr<gfxDrawable> restrictedPattern =

Maybe rename this restrictedDrawable? It's not really important, but it's what it actually is. If you think that way too, you should probably rename CreateSamplingRestrictedPattern to CreateSamplingRestrictedDrawable.

Great otherwise.

	Markus Stange [:mstange] Assignee
	Comment 28 • 14 years ago

(In reply to comment #26)
> >+gfxCallbackDrawable::Draw(gfxContext* aContext,
> >+                          const gfxRect& aFillRect,
> >+                          PRBool aRepeat,
> >+                          const gfxPattern::GraphicsFilter& aFilter,
> >+                          const gfxMatrix& aTransform)
> >+{
> >+    if (aRepeat) {
> >+        MakeSurfaceDrawable(aFilter);
> 
> Here, we should say mSurfaceDrawable = MakeSurfaceDrawable(...), and have
> MakeSurfaceDrawable just return an already_AddRefed<gfxSurfaceDrawable>.

Where should I put the if (!mSurfaceDrawable) {} caching guard, then?

> >+gfxPatternDrawable::Draw(gfxContext* aContext,
> 
> >+    if (aRepeat) {
> >+        // We can't use mPattern directly: We want our repeated tiles to have
> >+        // the size mSize, which might not be the case in mPattern.
> >+        nsRefPtr<gfxDrawable> cbDrawable = CallbackDrawable();
> >+        return cbDrawable->Draw(aContext, aFillRect, PR_TRUE, aFilter,
> >+                                aTransform);
> >+    }
> 
> This feels wrong. I've traced the code, and it seems like all we do is
> eventually call Draw() on ourselves with aRepeat = PR_FALSE.

Right. We construct a surface (done by gfxCallbackDrawable), paint into that surface by calling Draw() with aRepeat = PR_FALSE on ourselves, create a pattern with the resulting surface and repeat that pattern (done by gfxSurfaceDrawable).

> If we can't handle repeat, we should just do so explicitly,

What exactly do you mean? Drawing to a surface, putting that into a pattern and repeating that pattern is exactly what needs to be done in this case, so do you want me to duplicate that code here?

> I'd like it for CallbackDrawable() to be removed altogether.

I need it in bug 506826 part 3 for normal HTML paint servers: If we have background-image:-moz-element(#someelement), we use a callback to draw #someelement. If we don't need to repeat, this can happen without a temporary surface by using gfxCallbackDrawable::Draw() with aRepeat = PR_FALSE; if we *do* need to repeat (controlled by background-repeat) we need to draw into a surface first.

	Joe Drew (not getting mail)
	Comment 29 • 14 years ago

(In reply to comment #28)
> (In reply to comment #26)
> > >+gfxCallbackDrawable::Draw(gfxContext* aContext,
> > >+                          const gfxRect& aFillRect,
> > >+                          PRBool aRepeat,
> > >+                          const gfxPattern::GraphicsFilter& aFilter,
> > >+                          const gfxMatrix& aTransform)
> > >+{
> > >+    if (aRepeat) {
> > >+        MakeSurfaceDrawable(aFilter);
> > 
> > Here, we should say mSurfaceDrawable = MakeSurfaceDrawable(...), and have
> > MakeSurfaceDrawable just return an already_AddRefed<gfxSurfaceDrawable>.
> 
> Where should I put the if (!mSurfaceDrawable) {} caching guard, then?

In the same block (if (!mSurfaceDrawable) mSurfaceDrawable = ...). The thing I'm objecting to is the fact that it hides the initialization of mSurfaceDrawable.

> > >+gfxPatternDrawable::Draw(gfxContext* aContext,
> > 
> > >+    if (aRepeat) {
> > >+        // We can't use mPattern directly: We want our repeated tiles to have
> > >+        // the size mSize, which might not be the case in mPattern.
> > >+        nsRefPtr<gfxDrawable> cbDrawable = CallbackDrawable();
> > >+        return cbDrawable->Draw(aContext, aFillRect, PR_TRUE, aFilter,
> > >+                                aTransform);
> > >+    }
> > 
> > This feels wrong. I've traced the code, and it seems like all we do is
> > eventually call Draw() on ourselves with aRepeat = PR_FALSE.
> 
> Right. We construct a surface (done by gfxCallbackDrawable), paint into that
> surface by calling Draw() with aRepeat = PR_FALSE on ourselves, create a
> pattern with the resulting surface and repeat that pattern (done by
> gfxSurfaceDrawable).

Hm, I must have misread. How about you change the comment in the aRepeat case to say that? :)

	Markus Stange [:mstange] Assignee
	Comment 33 • 14 years ago

code simplification + necessary groundwork for -moz-element (bug 506826)

	Markus Stange [:mstange] Assignee
	Comment 34 • 14 years ago

Comment on attachment 459475 [details] [diff] [review]
part 3 to 6 combined for approval

I'll request approval when I've finished the rest of the -moz-element work.

	Markus Stange [:mstange] Assignee
	Comment 35 • 14 years ago

I had to make a change to this patch in order to fix a leak caused by a reference cycle. gfxPatternDrawable now no longer caches the gfxCallbackDrawable it creates when drawing with repeat; otherwise there would be a cycle like this:
gfxPatternDrawable::mCallbackDrawable -> gfxCallbackDrawable::mCallback -> DrawingCallbackFromDrawable::mDrawable -> gfxPatternDrawable

	Markus Stange [:mstange] Assignee
	Comment 36 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/97188fb7b44a
http://hg.mozilla.org/mozilla-central/rev/0fa683b7233d
http://hg.mozilla.org/mozilla-central/rev/b5f727a62c7c
http://hg.mozilla.org/mozilla-central/rev/91f0d2cd19e8

	Ed Morley [:emorley]
	Comment 37 • 14 years ago

This is believed to have caused a ~1MB/tab memory usage increase, see:
bug 598466 comment 106