197205 - remove null checks from GetStyleData callers

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 1

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

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

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

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?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

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

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

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

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

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 12

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

Comment on attachment 122808 [details]
patch

wonderful. r+sr=roc+moz

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 14

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

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

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 123099 [details]
patch

a=[e-mail] for 1.4final.

/be

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: clean up callers of old api that have crept back in (checked in 2004-12-03 22:31 -0800)