196273 - Using nsRefPtr on nsCSSDeclaration may improve code readability

Status: RESOLVED WONTFIX

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

Description

[Boris Zbarsky [:bzbarsky]]

In particular, the AddRef/Release rules in nsCSSStyleRule could go if it used an
nsRefPtr member; we may also be able to eliminate the various RuleAbort() calls
(replacing them with a simple AddRef/Release pair by passing a getter_AddRefs to
the NS_NewWhatever function)....

Comment 1

[rbs]

bz, I don't quite get it why there can't be a leak.

With |GetCSSDeclaration(decl, /*aAllocate=*/PR_TRUE)|, there is a code path that
does a |new|, and so with the early return that I pointed in bug 171830 comment
37 the |decl| will be left dangling, am I missing something?

Comment 2

[rbs]

Never mind... got it. SetCSSDeclaration() passes the ownership to somebody along
the way.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah; I should just comment the ownership model here more clearly....

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the real question here is whether we should stick to the whole "only
nsCSSStyleRule can refcount nsCSSDeclaration" thing....

Comment 5

[Boris Zbarsky [:bzbarsky]]

I should just take this.  I'll be making nsCSSDeclaration a normal refcounted
object, a la nsStyleContext.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Zack, is this at all still relevant after your changes?

Comment 7

[Zack Weinberg (:zwol)]

I don't think this makes sense as is after my changes, no.  We might be able to do nsAutoPtr instead for the CSSStyleRuleImpl->css::Declaration pointer, though.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, not, going to worry about this then.