Summary: | Repaint issues with -webkit-svg-shadow used on a container | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||||||||||||||||||||||||
Component: | SVG | Assignee: | Tim Horton <thorton> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | bdakin, dglazkov, d-r, eric, fmalita, gustavo.noronha, gustavo, gyuyoung.kim, krit, pdr, rakuco, schenney, webkit.review.bot, xan.lopez, zimmermann | ||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||
Bug Blocks: | 99533 | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Tim Horton
2011-08-03 14:40:58 PDT
It seems like the child elements don't know about their shadow, because it's entirely contained within the parent. When they're asked to redraw, they don't know about the shadow rect, so they don't include it in the intersection test. The problem of course being that the child needs to repaint in order to repaint its shadow! Ways that immediately come to mind to fix this: a) have some way of informing children that they *have* a shadow (and should take that into consideration when determining whether or not to repaint), but don't need to *set* it/appear to have it. b) stop allowing containers to have shadows. box-shadow on an HTML element isn't inherited by children, is there any reason -webkit-svg-shadow should be? This would change behavior, though, and I don't really want to do that. Any ideas? Created attachment 103699 [details]
correct repro
Somehow the copy of the repro I uploaded had some extra characters in it. Here's a corrected copy.
Created attachment 103710 [details]
preliminary patch sans changelog/test
Comment on attachment 103710 [details] preliminary patch sans changelog/test Attachment 103710 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9371223 Comment on attachment 103710 [details] preliminary patch sans changelog/test Attachment 103710 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9357432 Created attachment 103714 [details]
preliminary patch sans changelog/test
Comment on attachment 103714 [details] preliminary patch sans changelog/test Attachment 103714 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9346501 New failing tests: svg/css/shadow-changes.svg fast/repaint/moving-shadow-on-container.html svg/css/mask-with-shadow.svg svg/css/composite-shadow-text.svg svg/css/group-with-shadow.svg svg/css/arrow-with-shadow.svg svg/css/clippath-with-shadow.svg Comment on attachment 103714 [details]
preliminary patch sans changelog/test
Tim, the idea behind this patch is correct. OTOH it slows down every common case by requiring these additional tree traversal.
This needs to be optimized. We need to know whether a child needs this logic, that means we have to cache somewhere if the parent of an arbitary child has shadow or not.
Storing this inside SVGRenderStylle of the child is not an option, as styles may be shared.
<g style="-webkit-svg-shadow: ...">
<rect/>
</g>
<rect/>
Both rects may share the same style, so we can't store the information whether the parent is shadowed in the childs style.
The renderers are unique though, so it might be possible to stash this info into RenderSVGContainer.
I'll bet you'll find an elegant solution :-)
There are other issues I want to work out first before the performance, but that is a valid issue. Primarily: We already recursively include parent's shadows in the *repaint* rect via computeRectForRepaint; it's unfortunate that we're adding a second run up the tree, in *any* case (we shouldn't have to). Another issue is that since clippedOverflowRectForRepaint uses repaintRectInLocalCoordinates and then computeRectForRepaints that, with the patch as-is, the shadow gets included *twice*, and we overpaint. (In reply to comment #9) > Another issue is that since clippedOverflowRectForRepaint uses repaintRectInLocalCoordinates and then computeRectForRepaints that, with the patch as-is, the shadow gets included *twice*, and we overpaint. Actually, this isn't my fault, I just make it happen more often. Currently, if you apply -webkit-svg-shadow to a <rect/>, the repaint rect is adjusted for the shadow twice (thus, being quite a bit bigger than needed in my pathological cases). Created attachment 104596 [details]
expanded preliminary patch
(In reply to comment #11) > Created an attachment (id=104596) [details] > expanded preliminary patch This still has the issue of the overrepaint (which isn't due to this patch at all), but other than that is in good working order. Created attachment 104597 [details]
last patch plus image results that I missed
Comment on attachment 104597 [details] last patch plus image results that I missed Attachment 104597 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9432540 New failing tests: svg/css/shadow-changes.svg fast/repaint/moving-shadow-on-container.html svg/css/mask-with-shadow.svg svg/css/composite-shadow-text.svg svg/custom/repaint-webkit-svg-shadow.html svg/css/arrow-with-shadow.svg svg/css/clippath-with-shadow.svg svg/css/group-with-shadow.svg Comment on attachment 104597 [details] last patch plus image results that I missed View in context: https://bugs.webkit.org/attachment.cgi?id=104597&action=review It's getting better! Still some questions/suggestions. > Source/WebCore/platform/graphics/mac/FontMac.mm:225 > + bool hasSimpleShadow = context->textDrawingMode() == TextModeFill && shadowColor.isValid() && !shadowBlur && !platformData.isColorBitmapFont() && (!context->shadowsIgnoreTransforms() || context->getCTM().isIdentityOrTranslationOrFlipped()) && !context->inTransparencyLayer(); Aren't other platforms affected by this as well? Why is this needed? Can you explain a bit more? > Source/WebCore/rendering/RenderObject.cpp:1789 > + setHasSVGShadow(TRUE); s/TRUE/true/ > Source/WebCore/rendering/RenderObject.h:919 > +#if ENABLE(SVG) > + bool m_hasSVGShadow; > +#endif This bit of information is needed by all renderers which use intersectRepaintRectWithResources: RenderSVG(Container|Image|Path|Root|Text). RenderSVG(Container|Image|Path) all inherit from RenderSVGModelObject. RenderSVGRoot inherits from RenderBox, RenderSVGText from RenderSVGBlock, and thus from RenderBlock. I'd rather store m_hasSVGShadow in RenderSVGModelObject, RenderSVGRoot and RenderSVGText, to avoid consuming more memory for HTML use cases. hasSVGShadow could still be a virtual method in RenderObject, just like it's done for eg. nodeAtFloatPoint, and we'd override the getters/setters in RenderSVGModelObject/Root/Text. What do you think? > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:287 > + > + if (!style) > + continue; IIRC neither style nor svgStyle can be null, please double-check. > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:301 > + if (!currentObject->hasSVGShadow()) > + return; > + } while (currentObject); while (currentObject && currentObject->hasSVGShadow() ? Comment on attachment 104597 [details] last patch plus image results that I missed View in context: https://bugs.webkit.org/attachment.cgi?id=104597&action=review >> Source/WebCore/platform/graphics/mac/FontMac.mm:225 >> + bool hasSimpleShadow = context->textDrawingMode() == TextModeFill && shadowColor.isValid() && !shadowBlur && !platformData.isColorBitmapFont() && (!context->shadowsIgnoreTransforms() || context->getCTM().isIdentityOrTranslationOrFlipped()) && !context->inTransparencyLayer(); > > Aren't other platforms affected by this as well? > Why is this needed? Can you explain a bit more? Oh, yeah, that actually needs explanation. I'll provide an example: If we set the shadow on a group, a transparency layer is created, and the shadow is cleared (because it's applied to the group, not the children). Then, when we go to draw the text shadow, if it has no blur (and can be rendered by simply redrawing the text), we a) store the shadow b) draw the text by hand in the shadow color c) restore the shadow. But, if we're in a transparency layer, the shadow is not clearable! So what happens in effect is that we draw the text shadow into the transparency layer, then when it's composited onto the screen, the group's shadow is applied, leading to the *shadow* having its own shadow! So, we can't use simple shadows inside a transparency layer. I assume the behavior is the same on other platforms (with the shadow clearing and all) because that makes the most sense. I think perhaps we should make the increment/decrement/inTransparencyLayer() stuff enabled on all platforms. I might be able to concoct a test for this that still exhibits after we fix the overdraw, but for right now it's made blindingly obvious in a variety of other tests because of that flaw in this patch. >> Source/WebCore/rendering/RenderObject.h:919 >> +#endif > > This bit of information is needed by all renderers which use intersectRepaintRectWithResources: RenderSVG(Container|Image|Path|Root|Text). > > RenderSVG(Container|Image|Path) all inherit from RenderSVGModelObject. > RenderSVGRoot inherits from RenderBox, RenderSVGText from RenderSVGBlock, and thus from RenderBlock. > > I'd rather store m_hasSVGShadow in RenderSVGModelObject, RenderSVGRoot and RenderSVGText, to avoid consuming more memory for HTML use cases. > hasSVGShadow could still be a virtual method in RenderObject, just like it's done for eg. nodeAtFloatPoint, and we'd override the getters/setters in RenderSVGModelObject/Root/Text. > > What do you think? Sounds better, I'll do that. >> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:287 >> + continue; > > IIRC neither style nor svgStyle can be null, please double-check. I will double-check. >> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:301 >> + } while (currentObject); > > while (currentObject && currentObject->hasSVGShadow() ? Sure! I was using the space after the if(!currentObject->hasSVGShadow()) for debugging before, but that's certainly possible now. Created attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject
Comment on attachment 104713 [details] previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject Attachment 104713 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9468804 Comment on attachment 104713 [details] previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject Attachment 104713 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9467868 Comment on attachment 104713 [details] previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject Attachment 104713 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9461979 It's unclear to me how to fix the build of all of these other platforms; where is their implementation of GraphicsContext? Comment on attachment 104713 [details] previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject View in context: https://bugs.webkit.org/attachment.cgi?id=104713&action=review > Source/WebCore/platform/graphics/GraphicsContext.cpp:350 > +#if (OS(WINCE) && !PLATFORM(QT)) || PLATFORM(WX) It seems highly likely this list of platforms will get out of sync with the underlying machinery. Maybe instead there should be a private static inline member function that returns false or true that you could || with the transparency count. > Source/WebCore/platform/graphics/GraphicsContext.h:373 > + bool inTransparencyLayer() const; This should probably be named isInTransparencyLayer. Comment on attachment 104713 [details] previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject Attachment 104713 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9463919 So, this is still quite wrong, because it applies all of the shadows above an element in the tree to that element. So, for example <svg xmlns="http://www.w3.org/2000/svg" id="asdf" style="-webkit-svg-shadow: 50px 50px 5px red;"> <g><g><g><g><g><g><g> <rect x="10" y="10" width="20" height="20" fill="black" /> </g></g></g></g></g></g></g> </svg> Each of the <g> elements will be expanded by the size of <svg>'s shadow, and by the time we get to <svg>, we're *way* overrepainting. I wonder, is it appropriate to apply the shadow expansion only to leaf nodes? I'll have to think about that some more. Comment on attachment 104713 [details] previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject View in context: https://bugs.webkit.org/attachment.cgi?id=104713&action=review Looks great, some further comments from my side and Darin lead to r- for now: > Source/WebCore/platform/graphics/GraphicsContext.cpp:338 > + m_transparencyCount++; Use ++m_transparencyCount as micro optimization. > Source/WebCore/platform/graphics/GraphicsContext.cpp:345 > + m_transparencyCount--; Use --m_transparencyCount as micro optimization. >> Source/WebCore/platform/graphics/GraphicsContext.cpp:350 >> +#if (OS(WINCE) && !PLATFORM(QT)) || PLATFORM(WX) > > It seems highly likely this list of platforms will get out of sync with the underlying machinery. Maybe instead there should be a private static inline member function that returns false or true that you could || with the transparency count. Agreed! > Source/WebCore/platform/graphics/GraphicsContext.h:546 > + int m_transparencyCount; unsigned should be sufficient. > Source/WebCore/rendering/RenderObject.h:403 > + virtual void setHasSVGShadow(bool shadowedParent) { UNUSED_PARAM(shadowedParent); } I'd rather just use setHasSVGShadow(bool) { } here. Comment on attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject
Btw. The ChangeLog is still missing ;)
Created attachment 105525 [details]
new patch, with per-platform fixes
This should probably be split into two bugs/patches, one about the simple text shadow bug, and this one, about shadow repaint.
Comment on attachment 105525 [details] new patch, with per-platform fixes Attachment 105525 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9563276 New failing tests: svg/css/shadow-changes.svg fast/repaint/moving-shadow-on-container.html svg/css/mask-with-shadow.svg svg/css/composite-shadow-text.svg svg/css/group-with-shadow.svg svg/css/arrow-with-shadow.svg svg/css/clippath-with-shadow.svg Niko, do you have any suggestion about how to prevent over-expanding the repaint rect because extend our repaint rect by the shadow every time? (the example in Comment #24, for starters) (In reply to comment #29) > Niko, do you have any suggestion about how to prevent over-expanding the repaint rect because extend our repaint rect by the shadow every time? (the example in Comment #24, for starters) I rechecked your patch and thought about this. To be able to determine the point when the shadow rect extension should happen it's necessary to know _who_ is asking for the repaint rect. Suppose following tree: RenderSVGRoot (1) [shadowed] RenderSVGContainer (2) RenderSVGContainer (3) RenderSVGPath (4) When asking (4) for it's repaint rect, we want to include the shadow from (1) at the point where the recursion hits (1) (call to (4) -> (3) -> (2) -> (1)). Same for (3), (2). Your current solution extends each of the nodes (4) / (3) / (2) by the shadow of (1). Even when changing that, there's still the problem that we're caching the repaintRectInLocalCoordinates() that eg. (3) asked for in (3). We're doing the same for (2) and (1). When asking for the total repaint rect starting from (4), computeRectForRepaint() comes into play which traverses the parent hierarchy from (4) to (1) mapping the cached repaintBoundingBox() through the localToParentTransform(). An obvious problem with your current code is that you're extending _each_ of the repaint rects of (4) / (3) / (2) with the shadow rect defined in local coordinates of (1), that ignores the fact that when traversing from (4) -> (3) -> (2) -> (1) the repaint rect gets mapped through the local trafos. That will break cases like <svg style="-webkit-svg-shadow:.." <g transform="scale(2)"><g transform="scale(2)"> <rect ..>. In another comments you've mentioned <g><g><g><g>... <rect> ... which is currently over-drawing a lot as well. The problem here is the usage of ShadowData::adjustRectforShadow(). Each time you're calculating the <g> repaint rect through computeRectForRepaint() you're just extending the repaint rect by the amount of shadow. The function is named "intersectRepaintRectWithShadow" but it's never intersecting, rather it just grows the boundaries on each invocation. To summarize: The current approach is tricky. We should rethink about what we want to cache in repaintRectInLocalCoordinates(). The obvious solution for me is to stop including the shadow at all in repaintRectInLocalCoordinates() and only let computeRectForRepaint deal with it at all, to avoid mapping shadows of parents through localToParentTransform() of children, which is the root of all evil. I don't see at all why the children of a shadowed container should know that it's container has shadow. When computing the rect for repaint for the rect in <g><g><g><rect/> you'll always walk up the parents and can call adjustRectForShadow() on the renderer which contains the shadow and extend the repaint rect once by the amount of shadow distance. What do you think? :-) Moving all of the transparency layer and text shadow changes to https://bugs.webkit.org/show_bug.cgi?id=67543 Created attachment 106521 [details] test case of applying a shadow, and transforming the shadowed paths below the shadowed element (In reply to comment #30) > That will break cases like <svg style="-webkit-svg-shadow:.." <g transform="scale(2)"><g transform="scale(2)"> <rect ..>. I'm a bit intrigued by what actually *does* happen here, right now. Since the shadow is on the parent and is applied before the transformations, we end up with a different result than I initially expected; thinking about it now, though, I think it makes sense. Still, it would be nice to hear if this is expected -- attaching a test case. What's the status of this bug report? I'm a bit lost here. (In reply to comment #33) > What's the status of this bug report? I'm a bit lost here. I think the most recent patch still over expands the reprint rect; I forgot about this bug and haven't really worked through your long comment above. Thanks for reminding me, though! (In reply to comment #34) > (In reply to comment #33) > > What's the status of this bug report? I'm a bit lost here. > > I think the most recent patch still over expands the reprint rect; I forgot about this bug and haven't really worked through your long comment above. Thanks for reminding me, though! This is causing infinitely more problems now with tiled rendering. I'm going to take another look at this. (In reply to comment #30) > To summarize: The current approach is tricky. We should rethink about what we want to cache in repaintRectInLocalCoordinates(). The obvious solution for me is to stop including the shadow at all in repaintRectInLocalCoordinates() and only let computeRectForRepaint deal with it at all, to avoid mapping shadows of parents through localToParentTransform() of children, which is the root of all evil. > > I don't see at all why the children of a shadowed container should know that it's container has shadow. When computing the rect for repaint for the rect in <g><g><g><rect/> you'll always walk up the parents and can call adjustRectForShadow() on the renderer which contains the shadow and extend the repaint rect once by the amount of shadow distance. > > What do you think? :-) I don't see how children can get away without knowing that their container has shadow. There are places (RenderSVGShape::paint, for example) that use repaintRectInLocalCoordinates directly; the paint will bail there if the paintInfo doesn't intersect the repaintRectInLocalCoordinates (which you were proposing doesn't include the shadow), which is quite easy to cause in the tiled drawing case (or in the case where the shape is off-screen but the shadow is not). Created attachment 165758 [details]
patch
Needs a changelog, and I'd like to avoid adding anything to RenderObject if possible, and there's some minor cleanup required, and it might be crazysauce. However, it gets the repaint rects right for everything except text, with shadow on any parent, with arbitrary transforms in between, fixing a multitude of bugs (shadows not rendering when the object is off screen, shadows being clipped by tiles, shadows not invalidating correctly, etc.).
I don't have sufficient time at the moment, but next week, I plan on writing the changelog and putting the finishing touches on, and will file a followup bug to fix text (which is a bit crazier since it ends back up in non-SVG-land during painting).
Comment on attachment 165758 [details] patch Attachment 165758 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14035375 New failing tests: svg/custom/focus-ring.svg svg/css/composite-shadow-with-opacity.html svg/W3C-SVG-1.1/types-basicDOM-01-b.svg svg/css/clippath-with-shadow.svg svg/css/composite-shadow-example.html svg/css/shadow-changes.svg svg/batik/text/textFeatures.svg fast/repaint/moving-shadow-on-container.html svg/css/mask-with-shadow.svg svg/batik/text/smallFonts.svg svg/css/composite-shadow-text.svg svg/css/arrow-with-shadow.svg svg/W3C-SVG-1.1/animate-elem-82-t.svg svg/custom/simple-text-double-shadow.svg svg/W3C-SVG-1.1-SE/types-dom-01-b.svg svg/W3C-SVG-1.1/animate-elem-24-t.svg Comment on attachment 165758 [details] patch Attachment 165758 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14021612 New failing tests: svg/custom/focus-ring.svg svg/W3C-SVG-1.1/animate-elem-82-t.svg svg/css/shadow-changes.svg svg/W3C-SVG-1.1/types-basicDOM-01-b.svg svg/css/clippath-with-shadow.svg svg/css/composite-shadow-example.html svg/css/composite-shadow-with-opacity.html svg/batik/text/textFeatures.svg fast/repaint/moving-shadow-on-container.html svg/batik/text/smallFonts.svg svg/css/composite-shadow-text.svg svg/css/group-with-shadow.svg svg/css/arrow-with-shadow.svg svg/css/mask-with-shadow.svg svg/custom/simple-text-double-shadow.svg svg/W3C-SVG-1.1-SE/types-dom-01-b.svg svg/W3C-SVG-1.1/animate-elem-24-t.svg Comment on attachment 165758 [details]
patch
Great that you're back on this Tim. Looking forward to the detailed ChangeLog - even if this adds new stuff to RenderObject the approach in general looks much safer, though I only had a quick look.
Created attachment 169072 [details]
patch
I'm not going to handle the RenderSVGBlock-based cases (text and foreignobject) in this bug. Filed https://bugs.webkit.org/show_bug.cgi?id=99533 to cover those, I'll add it to my fixme. Attachment 169072 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/rendering/svg/SVGRenderSupport.h:68: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/rendering/svg/SVGRenderSupport.h:81: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/rendering/svg/SVGRenderSupport.h:82: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 3 in 28 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 169072 [details] patch Attachment 169072 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14384432 New failing tests: fast/repaint/moving-shadow-on-container.html Comment on attachment 169072 [details] patch Attachment 169072 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14400304 New failing tests: svg/css/shadow-changes.svg svg/repaint/repaint-webkit-svg-shadow.svg svg/css/composite-shadow-example.html svg/css/composite-shadow-with-opacity.html fast/repaint/moving-shadow-on-container.html svg/css/composite-shadow-text.svg svg/custom/simple-text-double-shadow.svg Did you already prototype a general solution based on your patch locally? Can you assure the interface will be the same for RenderSVGBlock & co? (In reply to comment #46) > Did you already prototype a general solution based on your patch locally? Can you assure the interface will be the same for RenderSVGBlock & co? I had it sort of working locally, but it required changes outside of SVG land. The parts of the interface that I'm changing here will be the same, surely. There might be additional stuff required though. Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow? Even after promotion, it was never used by content. Adding Beth who added this property. (In reply to comment #48) > Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow? > > Even after promotion, it was never used by content. > > Adding Beth who added this property. This would definitely need to be discussed on webkit-dev. We generally do not like to remove properties that have shipped, no matter how custom. (In reply to comment #48) > Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow? No, we cannot. > Even after promotion, it was never used by content. This is not quite accurate. Also, isn’t -webkit-svg-shadow faster? Does filter: drop-shadow always go through the filters code path? (In reply to comment #50) > (In reply to comment #48) > > Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow? > > No, we cannot. > We had multiple issues with -webkit-svg-shadow in the past and this is just the next one. I spend a lot ot time fixing animations and it seems still not working correctly. We should ask ourself is is worth it spending so much time on it? > > Even after promotion, it was never used by content. > > This is not quite accurate. > > Also, isn’t -webkit-svg-shadow faster? Does filter: drop-shadow always go through the filters code path? Not sure what you mean with filter path. So yes, we create a FilterEffect object for drop-shadow Instead of removing it, how about a compromise. Make -webkit-filter a shorthand for -webkit-svg-shadow with: filter: drop-shadow(<shadow>)+ So the the CSSParser would set the -webkit-filter create a FilterEffect object like for the filter property (well it really sets the filter property). This way we have one code path and not multiple of features with the same effect, each of them hard to maintain. (In reply to comment #51) > (In reply to comment #50) > > (In reply to comment #48) > > > Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow? > > > > No, we cannot. > > > > We had multiple issues with -webkit-svg-shadow in the past and this is just the next one. I spend a lot ot time fixing animations and it seems still not working correctly. We should ask ourself is is worth it spending so much time on it? > > > > Even after promotion, it was never used by content. > > > > This is not quite accurate. > > > > Also, isn’t -webkit-svg-shadow faster? Does filter: drop-shadow always go through the filters code path? > Not sure what you mean with filter path. So yes, we create a FilterEffect object for drop-shadow > > > Instead of removing it, how about a compromise. Make -webkit-filter a shorthand for -webkit-svg-shadow with: > > filter: drop-shadow(<shadow>)+ > > So the the CSSParser would set the -webkit-filter create a FilterEffect object like for the filter property (well it really sets the filter property). This way we have one code path and not multiple of features with the same effect, each of them hard to maintain. This would be better in the long term, yes. In the short term, I feel that setting the CG shadow while painting is in many cases going to be enormously faster than painting into a buffer, blurring it in software, and compositing it back. Also, we're looking for a safe, easy fix for this terrible repaint problem; we should investigate more invasive long-term solutions in another bug. Comment on attachment 169072 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=169072&action=review Why is this code so much more complex than the equivalent non-SVG code? > Source/WebCore/rendering/RenderObject.cpp:277 > +#if ENABLE(SVG) > + SVGRenderSupport::setRendererHasSVGShadow(newChild, SVGRenderSupport::rendererHasSVGShadow(this) || SVGRenderSupport::rendererHasSVGShadow(newChild)); > +#endif This is pretty gross. Can't we update this flag during layout or something? > Source/WebCore/rendering/RenderObject.cpp:1917 > +#if ENABLE(SVG) > + SVGRenderSupport::setRendererHasSVGShadow(this, (m_parent && SVGRenderSupport::rendererHasSVGShadow(m_parent)) || (style()->svgStyle() && style()->svgStyle()->shadow())); > +#endif Ditto. > Source/WebCore/rendering/svg/RenderSVGModelObject.h:80 > + bool m_hasSVGShadow; Is it OK to increase the size of all RenderSVGModelObjects by this? (In reply to comment #54) > (From update of attachment 169072 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169072&action=review > > Why is this code so much more complex than the equivalent non-SVG code? We talked about this in person. > > Source/WebCore/rendering/RenderObject.cpp:277 > > +#if ENABLE(SVG) > > + SVGRenderSupport::setRendererHasSVGShadow(newChild, SVGRenderSupport::rendererHasSVGShadow(this) || SVGRenderSupport::rendererHasSVGShadow(newChild)); > > +#endif > > This is pretty gross. Can't we update this flag during layout or something? Maybe! > > Source/WebCore/rendering/RenderObject.cpp:1917 > > +#if ENABLE(SVG) > > + SVGRenderSupport::setRendererHasSVGShadow(this, (m_parent && SVGRenderSupport::rendererHasSVGShadow(m_parent)) || (style()->svgStyle() && style()->svgStyle()->shadow())); > > +#endif > > Ditto. > > > Source/WebCore/rendering/svg/RenderSVGModelObject.h:80 > > + bool m_hasSVGShadow; > > Is it OK to increase the size of all RenderSVGModelObjects by this? sizeof(RenderSVGModelObject) = 56 before and after. I wish we had compile-time-asserts for the size of everything. Created attachment 172699 [details]
patch trying to fix testexpectations
Created attachment 172701 [details]
and the style bot
Attachment 172701 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/rendering/svg/RenderSVGModelObject.h:71: Extra space before ) [whitespace/parens] [2]
Source/WebCore/rendering/svg/RenderSVGModelObject.h:71: Too many spaces inside { }. [whitespace/braces] [5]
Source/WebCore/rendering/svg/RenderSVGModelObject.h:80: Should have a space between // and comment [whitespace/comments] [4]
LayoutTests/platform/qt/TestExpectations:2399: Path does not exist. [test/expectations] [5]
Total errors found: 4 in 33 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 172701 [details] and the style bot Attachment 172701 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14732993 Comment on attachment 172701 [details] and the style bot Attachment 172701 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14745545 (In reply to comment #58) > Attachment 172701 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 > Source/WebCore/rendering/svg/RenderSVGModelObject.h:71: Extra space before ) [whitespace/parens] [2] > Source/WebCore/rendering/svg/RenderSVGModelObject.h:71: Too many spaces inside { }. [whitespace/braces] [5] > Source/WebCore/rendering/svg/RenderSVGModelObject.h:80: Should have a space between // and comment [whitespace/comments] [4] > LayoutTests/platform/qt/TestExpectations:2399: Path does not exist. [test/expectations] [5] > Total errors found: 4 in 33 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Whoops, wrong patch. > LayoutTests/platform/qt/TestExpectations:2399: Path does not exist. [test/expectations] [5] That's webkit.org/b/101302 css3/line-break, which I didn't add. Created attachment 172706 [details]
post the right patch
Attachment 172706 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/qt/TestExpectations:2399: Path does not exist. [test/expectations] [5]
Total errors found: 1 in 33 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 172706 [details] post the right patch Attachment 172706 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14759214 New failing tests: svg/css/clippath-with-shadow.svg svg/css/arrow-with-shadow.svg Comment on attachment 172706 [details] post the right patch Attachment 172706 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14744643 New failing tests: svg/css/clippath-with-shadow.svg svg/css/arrow-with-shadow.svg Created attachment 172853 [details]
patch
Created attachment 172860 [details]
patch
Attachment 172860 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/qt/TestExpectations:2401: Path does not exist. [test/expectations] [5]
Total errors found: 1 in 33 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 172860 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=172860&action=review > Source/WebCore/rendering/RenderObject.cpp:289 > +#if ENABLE(SVG) > + SVGRenderSupport::setRendererHasSVGShadow(newChild, SVGRenderSupport::rendererHasSVGShadow(this) || SVGRenderSupport::rendererHasSVGShadow(newChild)); > +#endif This could be slightly less gross by having SVGRenderSupport::childAdded(RO*, RO*) and burying this inside it. > Source/WebCore/rendering/RenderObject.cpp:1923 > + SVGRenderSupport::setRendererHasSVGShadow(this, (m_parent && SVGRenderSupport::rendererHasSVGShadow(m_parent)) || (style()->svgStyle() && style()->svgStyle()->shadow())); And SVGRenderSupport::styleChanged(). > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:332 > + ASSERT(style); Not much point asserting there; you'll crash on the next line. (In reply to comment #70) > http://trac.webkit.org/changeset/133834 Hmpf, just realized that the review I made wasn't posted :( Anyhow, the bottom line was a r+ as well. Sorry Tim, for the long delays from my side :( The thesis keeps me busy. |