Closed
Bug 669234
Opened 13 years ago
Closed 13 years ago
Use AutoRestore to manage depth recursion tracking in nsSMILTimedElement
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: birtles, Assigned: birtles)
Details
Attachments
(1 file, 1 obsolete file)
2.92 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
netwerk/base/src/nsIOService.cpp has an RAII-style AutoIncrement class that would be useful elsewhere. I'd like to move it to mfbt.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•13 years ago
|
||
Proposed patch. dholbert has already given this r+ in bug 665334 but requesting review from cjones too.
Attachment #543842 -
Flags: review?(jones.chris.g)
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
Attachment #543842 -
Flags: review?(jones.chris.g) → review-
Comment 3•13 years ago
|
||
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•13 years ago
|
||
(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?
Assignee | ||
Comment 5•13 years ago
|
||
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!
Assignee | ||
Updated•13 years ago
|
Component: General → SVG
QA Contact: general → general
Summary: Move AutoIncrement to mozilla/mfbt → Use AutoRestore to manage depth recursion tracking in nsSMILTimedElement
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #543842 -
Attachment is obsolete: true
Attachment #544119 -
Flags: review?(dholbert)
Updated•13 years ago
|
Attachment #544119 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 7•13 years ago
|
||
Whiteboard: [inbound]
Assignee | ||
Comment 8•13 years ago
|
||
Landed on m-c: http://hg.mozilla.org/mozilla-central/rev/e4a42cc36dc7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Updated•13 years ago
|
Target Milestone: --- → mozilla8
You need to log in
before you can comment on or make changes to this bug.
Description
•