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

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Alex Fritze]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why can't you use nsChangeHint_RepaintFrame to trigger invalidation and
nsChangeHint_ReflowFrame to trigger geometry update?

Comment 4

[Alex Fritze]

(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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Alex Fritze]

What about incremental reflows that are targeted at frames below the <svg>,
though? Isn't this what style changes do?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right. But you can traverse the reflow command tree the same way that ::Reflow does.

Comment 8

[Alex Fritze]

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)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Alex Fritze]

(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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 12

[Alex Fritze]

(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.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 14

[Robert Longson [:longsonr]]

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