669234 - Use AutoRestore to manage depth recursion tracking in nsSMILTimedElement

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

netwerk/base/src/nsIOService.cpp has an RAII-style AutoIncrement class that would be useful elsewhere. I'd like to move it to mfbt.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1a, r=dholbert

Proposed patch. dholbert has already given this r+ in bug 665334 but requesting review from cjones too.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 543842 [details] [diff] [review]
Patch v1a, r=dholbert

Hi Brian,

I'm r-'ing this patch not because there are major problems with the impl, but because I'm not sure this is a common pattern across js/src and gecko.  The pattern I've come across more commonly is [1].  If AutoIncrement starts seeing use outside of the IO service, we can polish up it and revisit.

CC'ing luke for a second opinion.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/AutoRestore.h#163

Comment 3

[Luke Wagner [:luke]]

I agree with the general sentiment that its good to hoist lazily (when needed) vs. eagerly.  I also agree that I can't think of any uses for AutoIncrement in SM; usually the ++/-- is part of some specific guard that does more than just ++/--.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> [1]
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/AutoRestore.h#163

Much as I like the sleekness of AutoIncrement, I think that AutoRestore should serve our purposes equally well.

Brian -- so it looks like we could replace your proposed...
> AutoIncrement<PRUint16> inc(&mUpdateIntervalRecursionDepth);
...with...
> AutoRestore<PRUint16> depthRestorer(mUpdateIntervalRecursionDepth);
> mUpdateIntervalRecursionDepth++;

Perhaps we should re-target this bug to be about that?

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks Chris and Luke. Fair call. I failed to point out that this wasn't just speculative; the reason for requesting it is that we need something like this in SMIL. However, as Daniel points out, the AutoRestore option Chris mentioned will do the trick. Thanks!

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v2a

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/e4a42cc36dc7

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Landed on m-c: http://hg.mozilla.org/mozilla-central/rev/e4a42cc36dc7