Bug 20699

Summary: SVGPaintServer CleanUp
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal Keywords: Cairo, Qt
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
SVGPaintServer cleanUp
none
SVGPaintServer cleanUp
eric: review+
SVGPaintServer cleanUp
zimmermann: review-
SVGPaintServer cleanUp 2
none
SVGPaintServer cleanUp
none
SVGPaintServer cleanUp
none
SVGPaintServer cleanUp - first part
none
SVGPaintServer cleanUp - first part
none
SVGPaintServer cleanUp - first part
none
SVGPaintServer cleanUp - first part
none
SVGPaintServer cleanUp - second part
none
SVGPaintServer cleanUp - second part
none
SVGPaintServer cleanUp - third part
eric: review-
SVGPaintServer cleanUp - third part
krit: review-
SVGPaintServer cleanUp - fourth part
none
SVGPaintServer cleanUp - pre fifth part eric: review-

Description Dirk Schulze 2008-09-07 09:46:58 PDT
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.
Comment 1 Dirk Schulze 2008-09-07 09:56:55 PDT
Created attachment 23227 [details]
SVGPaintServer cleanUp

see comment above
Comment 2 Dirk Schulze 2008-09-07 09:58:54 PDT
*** Bug 20689 has been marked as a duplicate of this bug. ***
Comment 3 Dirk Schulze 2008-09-07 12:02:50 PDT
Created attachment 23228 [details]
SVGPaintServer cleanUp

changed build instruction for Cg.
Comment 4 Eric Seidel (no email) 2008-09-09 09:56:21 PDT
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+
Comment 5 Dirk Schulze 2008-09-09 11:08:36 PDT
(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.
Comment 6 Dirk Schulze 2008-09-17 01:57:12 PDT
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.
Comment 7 Nikolas Zimmermann 2008-09-17 02:26:20 PDT
Comment on attachment 23498 [details]
SVGPaintServer cleanUp

Looks great, some style issues that we talked about on IRC.
Comment 8 Dirk Schulze 2008-09-17 11:06:52 PDT
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.
Comment 9 Nikolas Zimmermann 2008-09-20 11:12:00 PDT
Comment on attachment 23498 [details]
SVGPaintServer cleanUp

Dirk is going to upload a new patch, combining the previous cleanup attempts.
Comment 10 Dirk Schulze 2008-09-20 11:46:51 PDT
Created attachment 23607 [details]
SVGPaintServer cleanUp

This one makes CgSupport obsolete and removes the platform dependent SVGPaintServerSolid* as well as RenderPath*.
Comment 11 Dirk Schulze 2008-09-20 12:53:37 PDT
Created attachment 23609 [details]
SVGPaintServer cleanUp

some corrections
Comment 12 Eric Seidel (no email) 2008-09-20 13:28:23 PDT
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.
Comment 13 Dirk Schulze 2008-09-20 13:49:21 PDT
Created attachment 23611 [details]
SVGPaintServer cleanUp - first part

First part: lineDash
Comment 14 Dirk Schulze 2008-09-20 14:23:02 PDT
Created attachment 23612 [details]
SVGPaintServer cleanUp - first part

fixes issues
Comment 15 Nikolas Zimmermann 2008-09-20 15:11:36 PDT
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.
Comment 16 Dirk Schulze 2008-09-20 15:23:14 PDT
Created attachment 23615 [details]
SVGPaintServer cleanUp - first part

Yes, another patch. Won't be the last one :-)
Comment 17 Dirk Schulze 2008-09-20 15:31:27 PDT
(In reply to comment #16)
There are to many blank lines. But please comment on it, before I upload the patch again.
Comment 18 Dirk Schulze 2008-09-20 15:50:52 PDT
Created attachment 23616 [details]
SVGPaintServer cleanUp - first part

final first part?
Comment 19 Eric Seidel (no email) 2008-09-20 15:52:27 PDT
Comment on attachment 23616 [details]
SVGPaintServer cleanUp - first part

So pretty!
Comment 20 Eric Seidel (no email) 2008-09-20 16:47:12 PDT
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.
Comment 21 Mark Rowe (bdash) 2008-09-20 18:31:22 PDT
Rolled out in r36725 as it broke Mac, Windows, and Qt builds.
Comment 22 Dirk Schulze 2008-09-21 03:00:52 PDT
Created attachment 23622 [details]
SVGPaintServer cleanUp - second part

second part: Make use of GraphicsContext::setLineDash() on all platforms.
Comment 23 Eric Seidel (no email) 2008-09-21 03:11:08 PDT
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.
Comment 24 Dirk Schulze 2008-09-21 03:20:39 PDT
Created attachment 23623 [details]
SVGPaintServer cleanUp - second part

second try
Comment 25 Eric Seidel (no email) 2008-09-21 03:39:42 PDT
Comment on attachment 23623 [details]
SVGPaintServer cleanUp - second part

Great!  Landed as r36737.  Clearing review flag.
Comment 26 Dirk Schulze 2008-09-21 06:41:32 PDT
Created attachment 23625 [details]
SVGPaintServer cleanUp - third part
Comment 27 Eric Seidel (no email) 2008-09-21 07:19:12 PDT
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?
Comment 28 Dirk Schulze 2008-09-21 10:20:59 PDT
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.
Comment 29 Darin Adler 2008-09-21 11:28:41 PDT
Comment on attachment 23627 [details]
SVGPaintServer cleanUp - third part

r=me
Comment 30 Dirk Schulze 2008-09-22 12:15:54 PDT
Created attachment 23660 [details]
SVGPaintServer cleanUp - fourth part

Moved out makeMapBetweenRects from CgSupport to AffineTransform.
Comment 31 Eric Seidel (no email) 2008-09-22 23:00:43 PDT
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!
Comment 32 Dirk Schulze 2008-09-23 00:58:20 PDT
Landed part four in r36795 and fixed in r36800 by eseidel.  Clearing review flag.
Comment 33 Dirk Schulze 2008-09-23 12:53:47 PDT
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.
Comment 34 Eric Seidel (no email) 2008-09-23 13:18:39 PDT
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.
Comment 36 Eric Seidel (no email) 2008-09-23 13:20:02 PDT
I think it also might be time to start a new bug. :)
Comment 37 Dirk Schulze 2008-09-24 08:06:06 PDT
(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.
Comment 38 Eric Seidel (no email) 2008-10-06 18:49:20 PDT
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. :)
Comment 39 Dirk Schulze 2008-10-06 22:27:05 PDT
(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.
Comment 40 Dirk Schulze 2008-11-15 15:46:04 PST
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.
Comment 41 Dirk Schulze 2008-11-17 13:11:25 PST
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