Bug 20699 – SVGPaintServer CleanUp

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Removed platform-dependend SVGPaintServerSolid*.cpp and added the platform-independent version to SVGPaintServerSolid.cpp. Moved applyStrokeStyleToContext() to SVGPaintServer.cpp and removed it from CgSupport.

With this patch colors on SVG texts work for Qt and Cairo too, now.

[Dirk Schulze]

Created attachment 23227 [details]
SVGPaintServer cleanUp

see comment above

[Dirk Schulze]

*** Bug 20689 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

Created attachment 23228 [details]
SVGPaintServer cleanUp

changed build instruction for Cg.

[Eric Seidel (no email)]

Comment on attachment 23228 [details]
SVGPaintServer cleanUp

We should really fix dashes.  (Instead of adding an #ifdef).  Just add a GraphicsContext::setLineDash(DashArray) function to GraphicContext, and an:
OwnPtr<DashArray> onto the GraphicsContext state, and then copy the passed in DashArray onto the state any time the method is called.  We'd have to fix stroke* to setup the dash calls per-platform, but I think that's teh better long term solution.  I guess that can be a separate patch.

Does this still work for CG?  If so, then r+

[Dirk Schulze]

(In reply to comment #4) 
> Does this still work for CG?  If so, then r+

Can't test it. I have no windows or MacOs to compile it.

[Dirk Schulze]

Created attachment 23498 [details]
SVGPaintServer cleanUp

Fixed windows and macos build. Tested Cg with Windows and it works without exceptions.

The DashArray can be added to GraphicsContext in a second step.

[Nikolas Zimmermann]

Comment on attachment 23498 [details]
SVGPaintServer cleanUp

Looks great, some style issues that we talked about on IRC.

[Dirk Schulze]

Created attachment 23503 [details]
SVGPaintServer cleanUp 2

I don't know how to make a patch that depends on another one. That's why I upload the whole patch.

This patch cleans up the styling issues, cleans up the PaintServer code in general, moved the DashArray to the GraphicsContext and purges the Qt code a bit.

Either this patch gets a review and landed or I update the patch after the landing of the first one.

[Nikolas Zimmermann]

Comment on attachment 23498 [details]
SVGPaintServer cleanUp

Dirk is going to upload a new patch, combining the previous cleanup attempts.

[Dirk Schulze]

Created attachment 23607 [details]
SVGPaintServer cleanUp

This one makes CgSupport obsolete and removes the platform dependent SVGPaintServerSolid* as well as RenderPath*.

[Dirk Schulze]

Created attachment 23609 [details]
SVGPaintServer cleanUp

some corrections

[Eric Seidel (no email)]

Comment on attachment 23609 [details]
SVGPaintServer cleanUp

Please break this down.  There seem to be lots of little errors.  I think trying to land smaller pieces of this, like just the linedash stuff first, would be helpful to the quality of this patch.

[Dirk Schulze]

Created attachment 23611 [details]
SVGPaintServer cleanUp - first part

First part: lineDash

[Dirk Schulze]

Created attachment 23612 [details]
SVGPaintServer cleanUp - first part

fixes issues

[Nikolas Zimmermann]

It's a good idea to split up these patches. Several small issues in the one, up for review:
- DashArray enum declared twice (GraphicsContext.h & SVGPaintServer.h - the latter should be removed)
- The Qt setLineDash function, could be rewritten to be more readable.

+    unsigned int dashLength = !dashes.isEmpty() ? dashes.size() : 0;
->
+    unsigned int dashLength = dashes.size();


+        unsigned int count = (dashLength % 2) == 0 ? dashLength : dashLength * 2;
->
+        unsigned int count = dashLength;
+        if ((dashLength % 2) != 0)
+            count *= 2;

+        for(unsigned int i = 0; i < count; i++)
->
+        for (unsigned int i = 0; i < count; ++i)

+            pattern.append(dashes[i % dashLength] / (float)pen.widthF());
->
+            pattern.append(dashes[i % dashLength] / narrowPrecisionToFloat(pen.widthF()));

That's it. Could you upload an updated version?
I'm going to land it then.

[Dirk Schulze]

Created attachment 23615 [details]
SVGPaintServer cleanUp - first part

Yes, another patch. Won't be the last one :-)

[Dirk Schulze]

(In reply to comment #16)
There are to many blank lines. But please comment on it, before I upload the patch again.

[Dirk Schulze]

Created attachment 23616 [details]
SVGPaintServer cleanUp - first part

final first part?

[Eric Seidel (no email)]

Comment on attachment 23616 [details]
SVGPaintServer cleanUp - first part

So pretty!

[Eric Seidel (no email)]

Comment on attachment 23616 [details]
SVGPaintServer cleanUp - first part

Landed as r36719.  Clearing review flag, since it seems you might be planning to re-use this bug for the rest of the patch.

[Mark Rowe (bdash)]

Rolled out in r36725 as it broke Mac, Windows, and Qt builds.

[Dirk Schulze]

Created attachment 23622 [details]
SVGPaintServer cleanUp - second part

second part: Make use of GraphicsContext::setLineDash() on all platforms.

[Eric Seidel (no email)]

Comment on attachment 23622 [details]
SVGPaintServer cleanUp - second part

We talked about style, and svgStyle local variables and about possibly renaming renderStyle to style in one usage.  Otherwise looks fine.  GREAT PATCH.

[Dirk Schulze]

Created attachment 23623 [details]
SVGPaintServer cleanUp - second part

second try

[Eric Seidel (no email)]

Comment on attachment 23623 [details]
SVGPaintServer cleanUp - second part

Great!  Landed as r36737.  Clearing review flag.

[Dirk Schulze]

Created attachment 23625 [details]
SVGPaintServer cleanUp - third part

[Eric Seidel (no email)]

Comment on attachment 23625 [details]
SVGPaintServer cleanUp - third part

Looks fine.  Except why are you adding:
 97 void SVGPaintServerSolid::renderPath(GraphicsContext*& context, const RenderObject* path, SVGPaintTargetType type) const

That's already implemented exactly like that in SVGPaintServer, or?  Maybe that's only for CG?

Also, it seems:
context->setFillRule(svgStyle->fillRule());

should go in setup() in the fill if-block not in renderPath(), or?

[Dirk Schulze]

Created attachment 23627 [details]
SVGPaintServer cleanUp - third part

Moved the fillRule from renderPath to setup.

>That's already implemented exactly like that in SVGPaintServer, or?
Yes, but I can't use it. Every platform has it's own renderPath() with it's platform dependent code and we can't get rid of it at the moment.
I'll move this renderPath code to SVGPaintServer, when the gradient and pattern code was changed too.

[Darin Adler]

Comment on attachment 23627 [details]
SVGPaintServer cleanUp - third part

r=me

[Dirk Schulze]

Created attachment 23660 [details]
SVGPaintServer cleanUp - fourth part

Moved out makeMapBetweenRects from CgSupport to AffineTransform.

[Eric Seidel (no email)]

Comment on attachment 23660 [details]
SVGPaintServer cleanUp - fourth part

I'm not sure I'm a huge fan of the name:
 126 AffineTransform& makeMapBetweenRects(const FloatRect& source, const FloatRect& dest);
Nor am I sure it really should go in AffineTransform.h.  But I think this patch is definitely a great step in the right direction, and we can always tweak it later.  Thanks ever so much!

[Dirk Schulze]

Landed part four in r36795 and fixed in r36800 by eseidel.  Clearing review flag.

[Dirk Schulze]

Created attachment 23712 [details]
SVGPaintServer cleanUp - pre fifth part

This only a patch to discuss the next steps. I want to show what I think we could do next.

I creat a new ImageBuffer to have a new GraphicsContext (save and restore could be necessary for qt) to draw on.
I add the given path to the context, call applyStrokeStyleToContext() and call strokeBoundingBox() on the GraphicsContext. Every platform has its own strokeBoundingBox() implementation to create the boundingbox of a stroked path.
This should work for all platforms (don't know how to do it on Qt at the moment).
Skia in Chromium seems to do it as well but duplicate some functions like boundingboxes for normal paths.
After this changes, we can move strokeBBox() to RenderPath.cpp.

The old implementation seems not to create a new context on every call and try to use an existing one, right? I tried something similar, it worked for Cg and Cairo but qt seems to delete the QOainter to early.

[Eric Seidel (no email)]

Comment on attachment 23712 [details]
SVGPaintServer cleanUp - pre fifth part

Th current implementation of:
 704 FloatRect GraphicsContext::strokeBoundingBox()

has the unfortunate side effect of destroying the current path. :(  This is one reason to use a scratch context.

[Eric Seidel (no email)]

Comment on attachment 23712 [details]
SVGPaintServer cleanUp - pre fifth part

See http://developer.apple.com/documentation/GraphicsImaging/Reference/CGContext/Reference/reference.html#//apple_ref/c/func/CGContextReplacePathWithStrokedPath

[Eric Seidel (no email)]

I think it also might be time to start a new bug. :)

[Dirk Schulze]

(In reply to comment #34)
> has the unfortunate side effect of destroying the current path. :(  This is one
> reason to use a scratch context.

I use it! I created an ImageBuffer with a new GraphicsContext. I named it context but you can name it scratchContext. It has nothing to do with drawing context. And GraphicsContext::strokeBoundingBox() is only called by the scratchContext.

[Eric Seidel (no email)]

I'm not sure what the current status of this bug is... but I think it's time to close this one and start a new bug. :)

[Dirk Schulze]

(In reply to comment #38)
> I'm not sure what the current status of this bug is... but I think it's time to
> close this one and start a new bug. :)

The third part was not landed yet. Otherwise I agree with you.

[Dirk Schulze]

Comment on attachment 23627 [details]
SVGPaintServer cleanUp - third part

This breaks svg/custom/svg-fonts-in-html.html and it gives wrong colors for svg/W3C-SVG-1.1/painting-fill-05-b.svg.

+        RGBA32 rgba = color().rgb();
+        ASSERT(!color().hasAlpha());
+        if (style)
+            rgba = colorWithOverrideAlpha(rgba, svgStyle->strokeOpacity());

seems to be handled differently by some platforms.

[Dirk Schulze]

Was landed in r38426. Had r- third part first, because it breaks 2 LayoutTest. I spoke to ggaren and we agreed to role it out if they aren't fixed.

Fixed in r38435, r38439