Closed Bug 233406 Opened 21 years ago Closed 2 years ago

some SVG frame impls do not call nsFrame::Init()

Categories

(Core :: SVG, defect)

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: bzbarsky, Unassigned)

References

Details

In particular, nsSVGGlyphFrame and nsSVGPathGeometryFrame This latter caused me a bunch of grief because http://www.croczilla.com/svg/svgtetris.xml was broken in my build (since I have some changes to nsFrame::Init that obviously weren't present in this code). Is there a good reason not to call Init() on the parents here?
Yeah, there is a reason actually. We want to squeeze some SVG initialization in between nsIFrame::Init's actions (see layout/svg/base/src/nsSVGPathGeometryFrame.cpp): The initialization process for SVG frames requires that an SVG frame's own virtual Init() method (no parameters; different from the nsIFrame method of the same name) gets called after mContent and mParent are set but *before* SetStyleContext() is being called. (The latter is because we currently drive the updating of the SVG frame's graphical content from DidSetStyleContext, where we assume that the element is fully initialized.) I suppose the easy but ugly fix would be: nsSVGPathGeometryFrame::Init(nsIPresContext ...) { mContent = aContent; // deliberately no NS_IF_ADDREF(mContent); mParent = aParent; Init(); return nsSVGPathGeometryFrameBase::Init(aPresContext,...) } or maybe pass mContent and mParent into nsSVGPathGeometryFrame::Init()? What do you reckon?
> or maybe pass mContent and mParent into nsSVGPathGeometryFrame::Init()? This seems better. nsSVGPathGeometryFrame::GetOuterSVGFrame could take an optional aParent parameter that it would use if provided, falling back to mParent otherwise. That said, maybe you should talk to roc about having style hints that meet your needs?
Why can't you use nsChangeHint_RepaintFrame to trigger invalidation and nsChangeHint_ReflowFrame to trigger geometry update?
(In reply to comment #3) > Why can't you use nsChangeHint_RepaintFrame to trigger invalidation and > nsChangeHint_ReflowFrame to trigger geometry update? Yeah, in many ways reflow would be the right place from which to drive things. The reason why I've shyed away from this so far is that SVG doesn't require reflow in any real sense, so the long code path taken by a reflow event seems somewhat excessive for SVG. To make reflow worthwhile for SVG you'd really want to pass parameters like the current transform matrix down the tree - but this would mean adding members to nsHTMLReflowState. At the moment the CTM is acquired by SVG frames individually when they update their geometries, so reflow will probably be better for deep trees. But I don't know how important a use-case this is; most SVG trees I've come across tend to be shallow. What complicates matters is that the whole geometry construction/updating process is currently lazy / on-demand driven, so that would need to be changed.
It seems like you could make the outermost SVG frame implement ::Reflow in a special way that doesn't involve recursively calling ::Reflow on its children, but some SVG-specific Reflow variant.
What about incremental reflows that are targeted at frames below the <svg>, though? Isn't this what style changes do?
Right. But you can traverse the reflow command tree the same way that ::Reflow does.
Hmm, so you are suggesting routing all reflows through the outer <svg> element? At the moment it works like this: <svg> <path id="the_path" stroke="none"/> </svg> the_path.setAttribute("stroke", "red"); -> nsSVGPathGeometryFrame::DidSetStyleContext() -> UpdateGraphic() -> outerSVGFrame->InvalidateRegion(dirty_region) You are suggesting: the_path.setAtrribute("stroke", "red"); -> nsCSSFrameCTOR::StyleChangeReflow() -> NS_NewHTMLReflowCommand() -> presShell->AppendReflowCommand() -> PostReflowEvent(new event) -> ProcessReflowCommands() -> create rendering context -> IncrementalReflow::AddCommand() -> IncrementalReflow::Dispatch() -> nsSVGOuterSVGFrame::Reflow() -> nsSVGPathGeometryFrame::SVGReflow() -> UpdateGraphic() -> outerSVGFrame->InvalidateRegion(dirty_region) For the case of SVG, it just seems overcomplicated to me. What further complicates things is that updates often happen in response to changes in non-style properties, such as the radius "r.baseVal.value" of a circle. Should these changes be routed through reflow as well? At the moment it works like this: circle_element.r.baseVal.value = 10; -> nsSVGValue::NotifyObservers(WillModify) -> nsSVGValue::NotifyObservers(DidModify) -> nsSVGCircleFrame::DidModifySVGObservable() -> UpdateGraphic(UPDATEMASK_PATH) -> outerSVGFrame->InvalidateRegion(dirty_region)
It's more complicated but I think the reflow path gives you a few benefits: 1) You can distinguish changes that require geometry update from changes that only require a repaint. 2) Coalescing of multiple changes. If you make a geometry-changing style change to both a parent and a child, the reflow approach gives you the opportunity to recalculate the parent first and then the child, or the child first and then the parent (whatever makes sense for the frame type), and avoid doing redundant work. 3) The reflow approach delays updates until processing of the reflow event, which means there's more scope for coalescing multiple changes. If I'm not mistaken, changing five attributes on the same element currently does five UpdateGeometries. Up to you really.
(In reply to comment #9) > It's more complicated but I think the reflow path gives you a few benefits: > > 1) You can distinguish changes that require geometry update from changes that > only require a repaint. All of our style attribs potentially require geometry updates rather than just repaints. Even seemingly simple attribs like "stroke" can potentially take a paint-server rather than a color and might have some cached renderer structures which need to be built/torn down. > 2) Coalescing of multiple changes. If you make a geometry-changing style > change to both a parent and a child, the reflow approach gives you the > opportunity to recalculate the parent first and then the child, or the child > first and then the parent (whatever makes sense for the frame type), and avoid > doing redundant work The only 'calculated' value for SVG that I can think of is the current transform matrix (CTM) and - granted - we currently do redundant work calculating it, especially if the tree has lots of deep children. > 3) The reflow approach delays updates until processing of the reflow event, > which means there's more scope for coalescing multiple changes. If I'm not > mistaken, changing five attributes on the same element currently does five > UpdateGeometries. No, there is a mechanism for coalescing changes during redraw suspension. Each frame has a integer member 'mUpdateFlags' where it ORs together change notifications while redraw is suspended. Once redraw is unsuspended the frame tree is traversed from the top. > Up to you really. Looks like a lot of work, and I'm not sure about the performance gains we can expect for typical use cases. One benefit of getting rid of the current notification scheme would be a saving of maybe 24bytes/frame on average, but I just wonder whether that is worth the effort.
(In reply to comment #10) > (In reply to comment #9) > > It's more complicated but I think the reflow path gives you a few benefits: > > > > 1) You can distinguish changes that require geometry update from changes > > that only require a repaint. > > All of our style attribs potentially require geometry updates rather than just > repaints. Even seemingly simple attribs like "stroke" can potentially take a > paint-server rather than a color and might have some cached renderer > structures which need to be built/torn down. Can't you compare the old and new values to get more information? That's what we do in the style system. > > 3) The reflow approach delays updates until processing of the reflow event, > > which means there's more scope for coalescing multiple changes. If I'm not > > mistaken, changing five attributes on the same element currently does five > > UpdateGeometries. > > No, there is a mechanism for coalescing changes during redraw suspension. > Each frame has a integer member 'mUpdateFlags' where it ORs together change > notifications while redraw is suspended. Once redraw is unsuspended the frame > tree is traversed from the top. Ah, okay. > > Up to you really. > > Looks like a lot of work, and I'm not sure about the performance gains we can > expect for typical use cases. > One benefit of getting rid of the current notification scheme would be a > saving of maybe 24bytes/frame on average, but I just wonder whether that is > worth the effort. Maybe it's not worth the effort now, but 24 bytes per frame sounds worth getting eventually.
(In reply to comment #11) > Can't you compare the old and new values to get more information? That's what > we do in the style system. With the pluggable renderer architecture that we have now (which I'm sure will be replaced by something else at some point - but that's a different issue), it is only the rendering engine that knows which changes require a reflow and which only a repaint. The backend just passes change flags to the rendering engine at very fine granularity and the engine decides what to do. > > One benefit of getting rid of the current notification scheme would be a > > saving of maybe 24bytes/frame on average, but I just wonder whether that is > > worth the effort. > > Maybe it's not worth the effort now, but 24 bytes per frame sounds worth getting > eventually. Agreed. Just need to find the time :-) I'd also love to have some performance measurements in place before we make any huge changes - just out of curiosity.
Blocks: 277146
Depends on: 380448
QA Contact: general

The bug assignee didn't login in Bugzilla in the last 7 months.
:jwatt, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: alex → nobody
Flags: needinfo?(jwatt)
Severity: normal → S3
Flags: needinfo?(jwatt)

Fortunately they all call nsIFrame::Init either directly or indirectly now.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.