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.
Created attachment 23227 [details] SVGPaintServer cleanUp see comment above
*** Bug 20689 has been marked as a duplicate of this bug. ***
Created attachment 23228 [details] SVGPaintServer cleanUp changed build instruction for Cg.
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+
(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.
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 on attachment 23498 [details] SVGPaintServer cleanUp Looks great, some style issues that we talked about on IRC.
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 on attachment 23498 [details] SVGPaintServer cleanUp Dirk is going to upload a new patch, combining the previous cleanup attempts.
Created attachment 23607 [details] SVGPaintServer cleanUp This one makes CgSupport obsolete and removes the platform dependent SVGPaintServerSolid* as well as RenderPath*.
Created attachment 23609 [details] SVGPaintServer cleanUp some corrections
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.
Created attachment 23611 [details] SVGPaintServer cleanUp - first part First part: lineDash
Created attachment 23612 [details] SVGPaintServer cleanUp - first part fixes issues
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.
Created attachment 23615 [details] SVGPaintServer cleanUp - first part Yes, another patch. Won't be the last one :-)
(In reply to comment #16) There are to many blank lines. But please comment on it, before I upload the patch again.
Created attachment 23616 [details] SVGPaintServer cleanUp - first part final first part?
Comment on attachment 23616 [details] SVGPaintServer cleanUp - first part So pretty!
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.
Rolled out in r36725 as it broke Mac, Windows, and Qt builds.
Created attachment 23622 [details] SVGPaintServer cleanUp - second part second part: Make use of GraphicsContext::setLineDash() on all platforms.
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.
Created attachment 23623 [details] SVGPaintServer cleanUp - second part second try
Comment on attachment 23623 [details] SVGPaintServer cleanUp - second part Great! Landed as r36737. Clearing review flag.
Created attachment 23625 [details] SVGPaintServer cleanUp - third part
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?
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 on attachment 23627 [details] SVGPaintServer cleanUp - third part r=me
Created attachment 23660 [details] SVGPaintServer cleanUp - fourth part Moved out makeMapBetweenRects from CgSupport to AffineTransform.
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!
Landed part four in r36795 and fixed in r36800 by eseidel. Clearing review flag.
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 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 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
I think it also might be time to start a new bug. :)
(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.
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. :)
(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 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.
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