Last Comment Bug 669234 - Use AutoRestore to manage depth recursion tracking in nsSMILTimedElement
: Use AutoRestore to manage depth recursion tracking in nsSMILTimedElement
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla8
Assigned To: Brian Birtles (:birtles)
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2011-07-04 17:03 PDT by Brian Birtles (:birtles)
Modified: 2011-07-15 07:10 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1a, r=dholbert (6.03 KB, patch)
2011-07-04 17:09 PDT, Brian Birtles (:birtles)
cjones.bugs: review-
Details | Diff | Splinter Review
Patch v2a (2.92 KB, patch)
2011-07-05 18:48 PDT, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Splinter Review

Description User image Brian Birtles (:birtles) 2011-07-04 17:03:40 PDT
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 User image Brian Birtles (:birtles) 2011-07-04 17:09:16 PDT
Created attachment 543842 [details] [diff] [review]
Patch v1a, r=dholbert

Proposed patch. dholbert has already given this r+ in bug 665334 but requesting review from cjones too.
Comment 2 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-05 07:16:54 PDT
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.

Comment 3 User image Luke Wagner [:luke] 2011-07-05 13:32:57 PDT
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 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-07-05 14:05:30 PDT
(In reply to comment #2)
> [1]

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);
> AutoRestore<PRUint16> depthRestorer(mUpdateIntervalRecursionDepth);
> mUpdateIntervalRecursionDepth++;

Perhaps we should re-target this bug to be about that?
Comment 5 User image Brian Birtles (:birtles) 2011-07-05 18:36:03 PDT
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 User image Brian Birtles (:birtles) 2011-07-05 18:48:49 PDT
Created attachment 544119 [details] [diff] [review]
Patch v2a
Comment 7 User image Brian Birtles (:birtles) 2011-07-14 19:19:28 PDT
Pushed to m-i:
Comment 8 User image Brian Birtles (:birtles) 2011-07-15 05:18:28 PDT
Landed on m-c:

Note You need to log in before you can comment on or make changes to this bug.