David Baron :dbaron: Assignee
	Description • 22 years ago

Now that bug 154751 has landed, we should remove all the null-checks after calls
to GetStyleData.

(It also might be worth converting to the templatized form, although I should
probably double-check that that doesn't change codesize.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 1 • 22 years ago

I think whoever does this should get r+sr-in-advance and then just go do it.

	David Baron :dbaron: Assignee
	Comment 2 • 22 years ago

This patch removes the null checks and also converts all callers to use the
typesafe template.  It modifies the template so that it works with nsRefPtr. 
It also changes the return type of nsIFrame::GetStyleData so that the result is
returned instead of being an out parameter.

	David Baron :dbaron: Assignee
	Comment 3 • 22 years ago

Comment on attachment 122808 [details]
patch

In this patch, there are nontrivial changes in nsStyleContext.h,
nsIFrame.h, and nsFrame.{h,cpp}.

I also cleaned up the SVG stuff in nsRuleNode.{h,cpp} -- I commented the
struct in nsRuleNode.h in the style of other structs, and I applied the
fix to bug 111815 to ComputeSVGData in nsRuleNode.cpp.

Pretty much everything else was conversion to the templatized forms and
removal of null checks.

	David Baron :dbaron: Assignee
	Comment 4 • 22 years ago

Oh, there were also some places where I removed |.get()| from the end of the
first parameter to the function template |GetStyleData| (no longer needed with
this patch) or added |::| to the beginning of the call (good style everywhere,
since it's needed within nsIFrame implementations).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 22 years ago

I should have said this before you did all this work, but ...

Since we're trying to get away from unnecessary out parameters, why not have a
family of methods
template <class Source> const nsStyleBorder* GetBorderStyle(const Source& source);
template <class Source> const nsStylePadding* GetPaddingStyle(const Source& source);
etc?

	David Baron :dbaron: Assignee
	Comment 6 • 22 years ago

If we went that way, I'd rather they were just methods on nsIFrame and
nsStyleContext (since there's no need for template type deduction).  I might
rather they be:

  const nsStyleBorder* StyleBorder() { return [...]; }

etc.

It's a little less general, but I guess I do prefer it.  Maybe I'll do another
pass over the tree.  But first, what do others think of the idea?

	David Baron :dbaron: Assignee
	Comment 7 • 22 years ago

(It's worth noting that with the current code at least some compilers will
optimize away the performance cost of having an out parameter rather than a
return value.  However, I think I like the syntax I propose in my previous
comment better.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 22 years ago

Yes, it's only a syntactic issue, not a performance issue.

I would slightly prefer the "GetBorderStyle()" naming scheme.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 22 years ago

And generate the function signatures using preprocessing?  Sure, that
could work....  We could also template GetStyleData on the return type,
no?  (That will mean simpler changes to callers, I guess; not sure whether
I prefer it to having names like GetBorderStyle or whatever).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 22 years ago

> We could also template GetStyleData on the return type

No, you can't do that.

> And generate the function signatures using preprocessing?

We could, although we're not talking about much code.

	David Baron :dbaron: Assignee
	Comment 11 • 22 years ago

This implements what is described above -- inline methods on nsIFrame and
nsStyleContext called GetStyleBorder, GetStyleColor, etc.  Otherwise it's
pretty similar to the previous patch, although there was an additional spot or
two where I couldn't resist a little cleanup.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 21 years ago

Comment on attachment 122808 [details]
patch

wonderful. r+sr=roc+moz

	David Baron :dbaron: Assignee
	Comment 13 • 21 years ago

Comment on attachment 123099 [details]
patch

This is relatively low-risk (and I'll double-check it again before I land), and
I think it's worth landing before we branch to make it easier to move things
back and forth between branch and trunk after we branch.

(I assume roc noted r+sr on the wrong attachment.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 21 years ago

woops, yeah, I meant to r+sr the latest patch

	Brendan Eich [:brendan]
	Comment 15 • 21 years ago

Comment on attachment 123099 [details]
patch

a=brendan@mozilla.org for 1.4final.

/be

	David Baron :dbaron: Assignee
	Comment 16 • 21 years ago

Fix checked in to trunk, 2003-05-14 20:42 PDT.