Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
343443003:
Remove SVGElement::rendererIsNeeded (Closed)
|
Created:
6 years, 6 months ago by rwlbuis Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, krit, eae+blinkwatch, ed+blinkwatch_opera.com, f(malita), fs, gyuyoung.kim_webkit.org, kouhei+svg_chromium.org, Stephen Chennney, sof Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionRemove SVGElement::rendererIsNeeded
Remove SVGElement::rendererIsNeeded, the same check is done in RenderTreeBuilder::shouldCreateRenderer.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176887
Patch Set 1 #Patch Set 2 : Add remaining rendererIsNeeded changes #
Total comments: 5
Patch Set 3 : Fix issues #Patch Set 4 : Remove accidently squashed patch #
Total comments: 5
Patch Set 5 : Different approach #Messages
Total messages: 16 (0 generated)
PTAL. This is probably not 100% as rendererIsNeeded is intended but I think the benefits outweigh that. rendererIsNeeded probably gets abused already :)
Oops, actually add reviewer :)
Oops, actually add reviewer :)
https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGEleme... File Source/core/svg/SVGElement.cpp (right): https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGEleme... Source/core/svg/SVGElement.cpp:178: if ((parentOrShadowHostElement() && !parentOrShadowHostElement()->isSVGElement()) || !isValid()) Put these in separate if statements. if (parentOrShadowHostElement() && !parentOrShadowHostElement()->isSVGElement()) return false; if (!isValid()) return false; https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGGElem... File Source/core/svg/SVGGElement.cpp (right): https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGGElem... Source/core/svg/SVGGElement.cpp:85: return ((parentOrShadowHostElement() && parentOrShadowHostElement()->isSVGElement()) && isValid()); Remove parens, it's all &&. https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGStopE... File Source/core/svg/SVGStopElement.cpp (right): https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGStopE... Source/core/svg/SVGStopElement.cpp:93: return ((parentOrShadowHostElement() && parentOrShadowHostElement()->isSVGElement()) && isValid()); remove parens https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGTSpan... File Source/core/svg/SVGTSpanElement.cpp (right): https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGTSpan... Source/core/svg/SVGTSpanElement.cpp:53: return Element::rendererIsNeeded(style); Remove all the extra parens, instead do: return isValid() && Element::rendererIsNeeded(style); https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGTextP... File Source/core/svg/SVGTextPathElement.cpp (right): https://codereview.chromium.org/343443003/diff/20001/Source/core/svg/SVGTextP... Source/core/svg/SVGTextPathElement.cpp:138: if (parentNode() && (isSVGAElement(*parentNode()) || isSVGTextElement(*parentNode())) && isValid()) Use separate if statements and early returns. if (!isValid()) return false; etc.
Thinking about this more and I realized it will also make use resolve the style for these elements when we knew the renderers were not needed before we would have done that. That's probably okay since it's so rare to put random SVG elements in bad places and it's better to favor simplicity, plus we made it slower long ago by using isChildAllowed... Do you have an opinion here pdr@?
I'm not a big fan of this patch because it moves tricky complexity from a single place out into 6 or so different places without a perf win. I would prefer if we could unify some of this logic into SVGElement::rendererIsNeeded. Some comments below. https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGEleme... File Source/core/svg/SVGElement.cpp (right): https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGEleme... Source/core/svg/SVGElement.cpp:178: if (parentOrShadowHostElement() && !parentOrShadowHostElement()->isSVGElement()) Won't this create renderers for elements without a parent? https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGGElem... File Source/core/svg/SVGGElement.cpp (right): https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGGElem... Source/core/svg/SVGGElement.cpp:85: return (parentOrShadowHostElement() && parentOrShadowHostElement()->isSVGElement()) && isValid(); No need for parenthesis, these are all &&'s https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGStopE... File Source/core/svg/SVGStopElement.cpp (right): https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGStopE... Source/core/svg/SVGStopElement.cpp:93: return parentOrShadowHostElement() && parentOrShadowHostElement()->isSVGElement() && isValid(); Do we need the same in SVGMarkerElement? Do we need this override at all? Why not just rely on SVGElement::rendererIsNeeded? https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTSpan... File Source/core/svg/SVGTSpanElement.cpp (right): https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTSpan... Source/core/svg/SVGTSpanElement.cpp:53: return isValid() && Element::rendererIsNeeded(style); Can you call SVGElement::rendererIsNeeded here? There's some small duplication of work due to checking for the parent twice, but it'll be harder to screw this up in a refactor in the future.
I'm not sure this patch is correct. m_renderingParent is not the same thing as parentOrShadowHostElement() if you're dealing with Shadow DOM distributions. Maybe that doesn't matter since distributions don't really work yet in SVG, but we'd have to switch back in the future. I suspect we shouldn't make this change unless there's a perf win. https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTextP... File Source/core/svg/SVGTextPathElement.cpp (right): https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTextP... Source/core/svg/SVGTextPathElement.cpp:138: if (!parentNode() || !(isSVGAElement(*parentNode()) || isSVGTextElement(*parentNode()))) It shouldn't be possible to get in here without a parentNode(). If you do want to leave the check please break this if into two statements and propagate the !.
On 2014/06/20 07:58:27, esprehn wrote: > I'm not sure this patch is correct. m_renderingParent is not the same thing as > parentOrShadowHostElement() if you're dealing with Shadow DOM distributions. > Maybe that doesn't matter since distributions don't really work yet in SVG, but > we'd have to switch back in the future. > > I suspect we shouldn't make this change unless there's a perf win. > > https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTextP... > File Source/core/svg/SVGTextPathElement.cpp (right): > > https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTextP... > Source/core/svg/SVGTextPathElement.cpp:138: if (!parentNode() || > !(isSVGAElement(*parentNode()) || isSVGTextElement(*parentNode()))) > It shouldn't be possible to get in here without a parentNode(). > > If you do want to leave the check please break this if into two statements and > propagate the !. It seems the patch is tricky in its current form and actually my need for it is not that big (anymore). What about patch set #5 though? I don't think we need SVGElement::rendererIsNeeded. If that patch is acceptable, I can update the CL title and changelog...
On 2014/06/24 17:03:05, rwlbuis wrote: > On 2014/06/20 07:58:27, esprehn wrote: > > I'm not sure this patch is correct. m_renderingParent is not the same thing as > > parentOrShadowHostElement() if you're dealing with Shadow DOM distributions. > > Maybe that doesn't matter since distributions don't really work yet in SVG, > but > > we'd have to switch back in the future. > > > > I suspect we shouldn't make this change unless there's a perf win. > > > > > https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTextP... > > File Source/core/svg/SVGTextPathElement.cpp (right): > > > > > https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTextP... > > Source/core/svg/SVGTextPathElement.cpp:138: if (!parentNode() || > > !(isSVGAElement(*parentNode()) || isSVGTextElement(*parentNode()))) > > It shouldn't be possible to get in here without a parentNode(). > > > > If you do want to leave the check please break this if into two statements and > > propagate the !. > > It seems the patch is tricky in its current form and actually my need for it is > not that big (anymore). What about patch set #5 though? I don't think we need > SVGElement::rendererIsNeeded. If that patch is acceptable, I can update the CL > title and changelog... Can you explain what you need this for?
This patch looks fine, but can you update the patch description?
On 2014/06/24 19:47:18, pdr wrote: > On 2014/06/24 17:03:05, rwlbuis wrote: > > On 2014/06/20 07:58:27, esprehn wrote: > > > I'm not sure this patch is correct. m_renderingParent is not the same thing > as > > > parentOrShadowHostElement() if you're dealing with Shadow DOM distributions. > > > Maybe that doesn't matter since distributions don't really work yet in SVG, > > but > > > we'd have to switch back in the future. > > > > > > I suspect we shouldn't make this change unless there's a perf win. > > > > > > > > > https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTextP... > > > File Source/core/svg/SVGTextPathElement.cpp (right): > > > > > > > > > https://codereview.chromium.org/343443003/diff/60001/Source/core/svg/SVGTextP... > > > Source/core/svg/SVGTextPathElement.cpp:138: if (!parentNode() || > > > !(isSVGAElement(*parentNode()) || isSVGTextElement(*parentNode()))) > > > It shouldn't be possible to get in here without a parentNode(). > > > > > > If you do want to leave the check please break this if into two statements > and > > > propagate the !. > > > > It seems the patch is tricky in its current form and actually my need for it > is > > not that big (anymore). What about patch set #5 though? I don't think we need > > SVGElement::rendererIsNeeded. If that patch is acceptable, I can update the CL > > title and changelog... > > Can you explain what you need this for? Just an internal cleanup idea. But it is tricky so I'll go with the Element::rendererIsNeeded removal.
PTAL, I updated the commit log to better reflect the change.
On 2014/06/24 22:31:11, rwlbuis wrote: > PTAL, I updated the commit log to better reflect the change. LGTM
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob.buis@samsung.com/343443003/80001
Message was sent while issue was closed.
Change committed as 176887 |
