Use AutoRestore to manage depth recursion tracking in nsSMILTimedElement

RESOLVED FIXED in mozilla8

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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

6 years ago
Assignee: nobody → birtles
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
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.
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

6 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 ++/--.
(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

6 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

6 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

6 years ago
Created attachment 544119 [details] [diff] [review]
Patch v2a
Attachment #543842 - Attachment is obsolete: true
Attachment #544119 - Flags: review?(dholbert)
Attachment #544119 - Flags: review?(dholbert) → review+
(Assignee)

Comment 7

6 years ago
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/e4a42cc36dc7
Whiteboard: [inbound]
(Assignee)

Comment 8

6 years ago
Landed on m-c: http://hg.mozilla.org/mozilla-central/rev/e4a42cc36dc7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.