Last Comment Bug 354866 - Replace nsSVGCairoCanvas with gfxContext
: Replace nsSVGCairoCanvas with gfxContext
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: tor
: Hixie (not reading bugmail)
:
Mentors:
Depends on: 359101
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-29 11:31 PDT by tor
Modified: 2006-12-07 15:46 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
snapshot of work (155.21 KB, patch)
2006-09-29 11:32 PDT, tor
no flags Details | Diff | Splinter Review
remove svg renderer layer (155.90 KB, patch)
2006-10-02 10:21 PDT, tor
no flags Details | Diff | Splinter Review
missed uuid updates (156.07 KB, patch)
2006-10-02 11:59 PDT, tor
no flags Details | Diff | Splinter Review
merge to tip (155.21 KB, patch)
2006-10-20 13:41 PDT, tor
no flags Details | Diff | Splinter Review
merge to tip (155.77 KB, patch)
2006-10-27 11:46 PDT, tor
no flags Details | Diff | Splinter Review
update per review comments (168.95 KB, patch)
2006-10-31 13:52 PST, tor
no flags Details | Diff | Splinter Review
all stack, all the time :) (168.76 KB, patch)
2006-11-01 09:45 PST, tor
roc: review+
roc: superreview+
Details | Diff | Splinter Review
checkin version - nsAutoSVGRenderMode moved (168.74 KB, patch)
2006-11-01 12:47 PST, tor
no flags Details | Diff | Splinter Review
update to tip (168.81 KB, patch)
2006-11-14 09:33 PST, tor
no flags Details | Diff | Splinter Review
update to tip (169.46 KB, patch)
2006-11-27 09:05 PST, tor
no flags Details | Diff | Splinter Review

Description tor 2006-09-29 11:31:34 PDT
Start of the switch of SVG code to thebes...
Comment 1 tor 2006-09-29 11:32:06 PDT
Created attachment 240649 [details] [diff] [review]
snapshot of work
Comment 2 tor 2006-10-02 10:21:35 PDT
Created attachment 240955 [details] [diff] [review]
remove svg renderer layer

Start of the move over to thebes entirely in the SVG code.  This only removes the remains of the renderer layer (nsSVGRendererCanvas) - later bugs/patches will continue to remove the direct cairo usage.

I'll post to .builds announcing that SVG will soon switch to depend on cairo gfx.
Comment 3 tor 2006-10-02 11:59:46 PDT
Created attachment 240963 [details] [diff] [review]
missed uuid updates
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-10-08 22:53:36 PDT
Do you want me to review this now? You can't land it yet, I assume.
Comment 5 tor 2006-10-09 05:50:28 PDT
(In reply to comment #4)
> Do you want me to review this now? You can't land it yet, I assume.

Yes, I would like it reviewed now.

Why couldn't I land it?  I just need to coordinate with the tinderboxes to have svg turned off on OS-X for now.

Comment 6 tor 2006-10-20 13:41:26 PDT
Created attachment 242910 [details] [diff] [review]
merge to tip
Comment 7 tor 2006-10-27 11:46:44 PDT
Created attachment 243819 [details] [diff] [review]
merge to tip
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-10-30 13:54:41 PST
+  nsSVGOuterSVGFrame* outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(this);
+  if (!outerSVGFrame)
+    return NS_ERROR_FAILURE;
+
+  nsCOMPtr<nsIRenderingContext> ctx = outerSVGFrame->GetRenderingContext();

Why are we getting nsIRenderingContext from the outerSVGFrame? Can't we just create a temporary nsIRenderingContext that wraps aContext?
Comment 9 tor 2006-10-30 14:02:36 PST
(In reply to comment #8)
> +  nsSVGOuterSVGFrame* outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(this);
> +  if (!outerSVGFrame)
> +    return NS_ERROR_FAILURE;
> +
> +  nsCOMPtr<nsIRenderingContext> ctx = outerSVGFrame->GetRenderingContext();
> 
> Why are we getting nsIRenderingContext from the outerSVGFrame? Can't we just
> create a temporary nsIRenderingContext that wraps aContext?

Partly because I didn't think of it, partly because doing that seems to require having a nsIDeviceContext which isn't available.

Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-10-30 15:16:39 PST
+  nsresult SetupCairoFill(gfxContext *aContext,
                           cairo_t *aCtx,
                           void **aClosure);

Why do we need to pass both aContext and aCtx here? Can't we just pass aContext and use aContext->GetCairo as needed?

+  nsresult GetGlobalTransform(cairo_t *ctx, gfxContext *aContext);

Same question

+  nsRefPtr<gfxContext> ctx =
+    (gfxContext *)aRenderingContext.GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT);

Why do you need a strong ref here? Also, use NS_STATIC_CAST.

+already_AddRefed<nsIRenderingContext>
+nsSVGOuterSVGFrame::GetRenderingContext()

And why does this need to add a ref?

+nsSVGPathGeometryFrame::GeneratePath(cairo_t *ctx, gfxContext* aContext)

It's not really clear why we need two context parameters here. Is it because we don't have a gfxContext when we're hit testing, but we need the aContext to get its matrix when drawing? Could we create a temporary aContext for hit-testing?

+  nsRefPtr<gfxUnknownSurface> tmpSurface =
+    new gfxUnknownSurface(patternSurface);
+  if (!tmpSurface)
+    return NS_ERROR_FAILURE;

Could we stack-allocate this? If we can't, it would be nice to change things so we can allocate temporary wrappers (like you did with gfxContext earlier). Check with vlad/pav?

I'm not really comfortable with the global render-mode. Should we perhaps define some sort of nsSVGRenderState struct containing the render mode, the gfxContext, and possibly other things, and use that as the parameter to PaintSVG? It seems to me that the current code would at least need to save and restore the render mode when it changes the render mode, e.g. in nsSVGClipPathFrame.cpp. If we had nsSVGRenderState we could put the nsIRenderingContext* in there too, so we wouldn't need the cache-it-in-nsSVGOuterFrame hack.



Comment 11 tor 2006-10-31 13:52:55 PST
Created attachment 244245 [details] [diff] [review]
update per review comments
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-10-31 22:24:46 PST
Comment on attachment 244245 [details] [diff] [review]
update per review comments

+  cairo_t *aCtx = aContext->GetCairo();

Take the plunge and rename to "ctx", please :-)

+  nsCOMPtr<nsIRenderingContext> mRenderingContext;

We don't need this in nsSVGOuterSVGFrame any more, right?

This is nice. One more thing we could do. nsSVGRenderState doesn't need an nsTArray stack. Instead we could have a helper class ... nsAutoSVGRenderMode say ... whose constructor saves the current render mode and sets the new mode, and whose destructor restores the old render mode.
Comment 13 tor 2006-11-01 09:45:48 PST
Created attachment 244320 [details] [diff] [review]
all stack, all the time :)
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-11-01 12:22:26 PST
Comment on attachment 244320 [details] [diff] [review]
all stack, all the time :)

Move nsAutoSVGRenderMode to nsSVGUtils.h next to nsSVGRenderState.
Comment 15 tor 2006-11-01 12:47:11 PST
Created attachment 244344 [details] [diff] [review]
checkin version - nsAutoSVGRenderMode moved

Version for checkin, once I've reminded people that svg will need to be disabled on non-cairo builds and had the tinderbox configurations prepared.
Comment 16 tor 2006-11-14 09:33:36 PST
Created attachment 245571 [details] [diff] [review]
update to tip
Comment 17 tor 2006-11-27 09:05:50 PST
Created attachment 246681 [details] [diff] [review]
update to tip
Comment 18 tor 2006-11-27 10:50:57 PST
Checked in.  Needed to disable SVG on Camino until they switch to cairo gfx (bug 358390).

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