Comment 54 for bug 345207

(In reply to comment #33)
> > Should be ok since the calling code checks the list length (as a shortcut)
> > before calling the function.
>
> I wouldn't bother with the shortcut check. This code is only called when things
> change; either once on load or if you do something in DOM so I think simplicity
> is better.

Without the shortcut check there is no way to know if the return value is valid or not (similar to discussion for GetSingleValue() in nsSVGTextFrame.cpp)

I take your point about making it simpler. I made an SVGLengthListHelper class to hide the list length checking etc.

> > > >+already_AddRefed<gfxFlattenedPath>
> > > >+nsSVGTextPathFrame::GetFlattenedPath(nsIFrame *path)
> > > >+{
> > > >+ if (!path) {
> > > >+ path = GetPathFrame();
> > > >+ if (!path)
> > > >+ return nsnull;
> > > >+ }
> > >
> > > This change is then not necessary and can be removed.
> >
> > Shouldn't there at least be
> >
> > if (!path)
> > return nsnull;
>
> How about
>
> NS_ASSERTION(path, NS_ERROR_INVALID_POINTER);
>
> instead? Since this is a private function, it's reasonable for it to assume its
> callers know not to call it with nsnull.

Sounds good.