| Summary: | SVGPaintServer needs to be converted to the new RenderSVGResource* system | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, bdakin, dglazkov, eric, gustavo, jorlow, krit, staikos, webkit.review.bot, xan.lopez | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 38106 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Nikolas Zimmermann
2010-04-22 05:02:53 PDT
Created attachment 54052 [details]
File moves / build system changes
(In reply to comment #1) > Created an attachment (id=54052) [details] > Patch What for a garbage ;-) Hrmpf, just because SVGResourceListener.h is empty the ews has troubles, more specifically svn-apply. I don't really get it, as I created the patch using "webkit-patch post".... Comment on attachment 54052 [details]
File moves / build system changes
r=me.
Committed r58093: <http://trac.webkit.org/changeset/58093> Heh, forgot to tell webkit-patch not to close this bug, it's our master bug for the transition. Comment on attachment 54052 [details]
File moves / build system changes
Marking first patch as obsolete, it has been landed and was just a preparation.
Created attachment 54062 [details]
LayoutTests changes
Created attachment 54063 [details]
WebCore changes
Huuuuuge speedup, try zooming/panning around pattern/gradient tests :-)
(In reply to comment #9) > Created an attachment (id=54063) [details] > WebCore changes > > Huuuuuge speedup, try zooming/panning around pattern/gradient tests :-) In RenderSVGresource.cpp 41 ASSERT(svgElement && svgElement->isStyled()); two asserts please. I dislike the general changes on applyResource(), Can't we set FillMode or StrokeMode as a locale variable during the fillPaintingResource/strokePaintingResources calls? Solid-, Pattern-, GradientResources wold ask for it in applyResource. It's not quite understandable to have this in Clipper, Masker, Marker and Filter, without needing it as well. The same for postApplyResource. What is anyQName() in RenderSVGResourceGradient and Pattern? Can we get a better name for it? RenderSVGResourcePattern: 245 OwnPtr<ImageBuffer> tileImage = ImageBuffer::create(imageSize); 295 OwnPtr<ImageBuffer> newTileImage = ImageBuffer::create(IntSize(tileWidth, tileHeight)); Can you add check's right before creating the ImageBuffers, if both sides will be >= 1? 63 PassOwnPtr<ImageBuffer> createTileImage(FloatRect& patternBoundaries, AffineTransform& patternTransform, const SVGPatternElement*, RenderObject*) const; Why do you pass the PatternElement, do you think it's a speed win? 85 #if PLATFORM(SKIA) 86 // FIXME: Move this into the GraphicsContext 87 // WebKit implicitly expects us to reset the path. 88 // For example in fillAndStrokePath() of RenderPath.cpp the path is 89 // added back to the context after filling. This is because internally it 90 // calls CGContextFillPath() which closes the path. 91 context->beginPath(); 92 context->platformContext()->setFillShader(0); 93 context->platformContext()->setStrokeShader(0); 94 #endif I think thats a general issue with Skia and mainly affects Patterns and Gradients. This code might be placed in GraphicsContextSkia directly. 149 // FIXME: Why doesn't ColorStop take a Color and an offset?? You wrote this line in SVGGradientElement.cpp, but 4 lines later you just do it :-) Realy great patch!!! (In reply to comment #8) > Created an attachment (id=54062) [details] > LayoutTests changes The new DRT results look realy great!! Can you explain why pservers-grad-19-b looks different? It was broken, but the new reult doesn't look better! (In reply to comment #10) > (In reply to comment #9) > > Created an attachment (id=54063) [details] [details] > > WebCore changes > > > > Huuuuuge speedup, try zooming/panning around pattern/gradient tests :-) > > In RenderSVGresource.cpp > > 41 ASSERT(svgElement && svgElement->isStyled()); > > two asserts please. Done. > > I dislike the general changes on applyResource(), Can't we set FillMode or > StrokeMode as a locale variable during the > fillPaintingResource/strokePaintingResources calls? > Solid-, Pattern-, GradientResources wold ask for it in applyResource. It's not > quite understandable to have this in Clipper, Masker, Marker and Filter, > without needing it as well. The same for postApplyResource. Remember the first version of my patch? It contains the same concept that you're demanding. I had "resourceMode()" and "setResourceMode()" methods in RenderSVGResource. For paths everything worked well, though in text rendering the concept has a problem. Let me explain: When rendering text SVGRootInlineBox sets up the painting resources (setupFill/setupStroke) etc, and then SVGInlineTextBox is used to actually draw the text (drawText() on GraphicsContext). For regular text this works fine. When the rendering happens through SVGFont, paths have to be filled/stroked. The problem is that SVGRootInlineBox sets setResourceMode(ApplyToFillMode | ApplyToTextMode) (to indicate text rendering) and SVGFont calls setResourceMode(ApplyToFillMode). The information that we're actually rendering text is lost, so we'd have to preserve the resourceMode, _before_ calling applyResource, and restoring it after calling postApplyResource. I would have done that though the problem is a bit more complex, as it affects more places than just SVGFont. The problem is completely gone when _not_ storing the resourceMode anywhere, but explicitely passing it in applyResource. So I think in the end this solution is better. I've added ASSERTS in RenderSVGResourceClipper/Filter/Masker, to make sure no one ever passes something else then ApplyToDefaultMode there. Hope this solution is acceptable. > What is anyQName() in RenderSVGResourceGradient and Pattern? Can we get a > better name for it? anyQName() == any QualifiedName - it's defined in QualifiedName.h, I think it's a good name :-) > RenderSVGResourcePattern: > > 245 OwnPtr<ImageBuffer> tileImage = ImageBuffer::create(imageSize); > > 295 OwnPtr<ImageBuffer> newTileImage = > ImageBuffer::create(IntSize(tileWidth, tileHeight)); > > Can you add check's right before creating the ImageBuffers, if both sides will > be >= 1? Ok will do that. > 63 PassOwnPtr<ImageBuffer> createTileImage(FloatRect& patternBoundaries, > AffineTransform& patternTransform, const SVGPatternElement*, RenderObject*) > const; > > Why do you pass the PatternElement, do you think it's a speed win? Sure, we save doing the same node() -> SVGPatternElement* cast twice. > 85 #if PLATFORM(SKIA) > 86 // FIXME: Move this into the GraphicsContext > 87 // WebKit implicitly expects us to reset the path. > 88 // For example in fillAndStrokePath() of RenderPath.cpp the path is > 89 // added back to the context after filling. This is because internally > it > 90 // calls CGContextFillPath() which closes the path. > 91 context->beginPath(); > 92 context->platformContext()->setFillShader(0); > 93 context->platformContext()->setStrokeShader(0); > 94 #endif > > I think thats a general issue with Skia and mainly affects Patterns and > Gradients. This code might be placed in GraphicsContextSkia directly. I have no clue about the Skia code, and would rather prefer this to be done by some Chromium folks. I am just preserving existing behasviour. > > > 149 // FIXME: Why doesn't ColorStop take a Color and an offset?? > > You wrote this line in SVGGradientElement.cpp, but 4 lines later you just do it > :-) No. ColorStop takes an offset and four floats - see platform/graphics/Gradient.h > Realy great patch!!! Thanks a lot, will tidy it up and write a detailed ChangeLog. (In reply to comment #11) > (In reply to comment #8) > > Created an attachment (id=54062) [details] [details] > > LayoutTests changes > > The new DRT results look realy great!! Can you explain why pservers-grad-19-b > looks different? It was broken, but the new reult doesn't look better! Oh, pservsers-grad-19-b is a progression. The animateColor animation is now actually working (for the bottom-right rectangle). This is because we're now wrapping <g display="none"> in RenderSVGHiddenContainers instead of not creating renderers. This also resolves the problem that SVGGradientElement::buildStop() had to do manual style resolving if a <stop> element renderer was not existant. This is not the case anymore. If you wonder why it's yellow now. This is the expected _beginning_ of the animation. If you wait the 5 seconds, it will turn into solid green color, of course the layout tests don't test that, as you know. Comment on attachment 54062 [details]
LayoutTests changes
Ok great!
> > 85 #if PLATFORM(SKIA) > > 86 // FIXME: Move this into the GraphicsContext > > 87 // WebKit implicitly expects us to reset the path. > > 88 // For example in fillAndStrokePath() of RenderPath.cpp the path is > > 89 // added back to the context after filling. This is because internally > > it > > 90 // calls CGContextFillPath() which closes the path. > > 91 context->beginPath(); > > 92 context->platformContext()->setFillShader(0); > > 93 context->platformContext()->setStrokeShader(0); > > 94 #endif > > > > I think thats a general issue with Skia and mainly affects Patterns and > > Gradients. This code might be placed in GraphicsContextSkia directly. > I have no clue about the Skia code, and would rather prefer this to be done by > some Chromium folks. I am just preserving existing behasviour. OK, but make sure someone of the Skia/Chrome-guys is online to fix this issue on landing please. > > > > > > > 149 // FIXME: Why doesn't ColorStop take a Color and an offset?? > > > > You wrote this line in SVGGradientElement.cpp, but 4 lines later you just do it > > :-) > No. ColorStop takes an offset and four floats - see > platform/graphics/Gradient.h Ah you mean Color()? ok. I missunderstood it. Created attachment 54145 [details]
WebCore changes - v2
Attachment 54145 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderSVGResourceGradient.cpp:30: Alphabetical sorting problem. [build/include_order] [4]
WebCore/rendering/RenderSVGResource.h:44: One space before end of line comments [whitespace/comments] [5]
WebCore/rendering/RenderSVGResource.cpp:137: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/svg/SVGDocumentExtensions.h:31: Alphabetical sorting problem. [build/include_order] [4]
WebCore/svg/GradientAttributes.h:49: More than one command on the same line [whitespace/newline] [4]
WebCore/svg/PatternAttributes.h:28: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 6 in 66 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 54147 [details]
WebCore changes - v3
Hopefully w/o any more style issues now.
Attachment 54145 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1854060 Attachment 54145 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1886011 Created attachment 54150 [details]
WebCore changes - v4
I love EWS! :-) Forgot to include a header for the Skia port, and missed to modify GNUmakefile.am... hopefully it's working now!
Attachment 54147 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1862043 Attachment 54147 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1849063 Created attachment 54151 [details]
WebCore changes - v5
Hm, must have uploaded the wrong patch :( Retrying...
Weird though that gtk doesn't build as even the patch v4, included the GNUmakefile.am changes, hm.
Comment on attachment 54151 [details]
WebCore changes - v5
Looks like the other bots don't want to test this monster ;-)
Patch looks great! Good job.
r=me
Committed r58212: <http://trac.webkit.org/changeset/58212> http://trac.webkit.org/changeset/58212 might have broken Qt Linux Release Landed fix for release builds in r58213. Landed linking fix in r58214. Landed new qt results in r58215. Committed r58524: <http://trac.webkit.org/changeset/58524> |