Bug 23025 - Simplifying DOMTimer lifetime management code
Summary: Simplifying DOMTimer lifetime management code
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Nobody
Depends on:
Reported: 2008-12-29 14:22 PST by Dmitry Titov
Modified: 2009-01-02 18:27 PST (History)
2 users (show)

See Also:

Proposed patch (3.78 KB, patch)
2008-12-29 14:35 PST, Dmitry Titov
darin: review+
Details | Formatted Diff | Diff
Patch with changes after review (3.51 KB, patch)
2009-01-02 17:11 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 2008-12-29 14:22:02 PST
Following IRC discussion...

DOMTimer lifetime is a bit confusing - it is created from DOMTimer::install() but then destroyed sometimes by direct 'delete' (when Document is destroyed) and sometimes as a side-effect (in Document::removeTimeout which should only remove an ID from the Document's timeout map).

Proposed change makes this code a bit more straightforward. The timeoutMap methods on the Document now do not delete the timers, they only add/remove/find timers in the map.
The timers are added to the map in their constructor and removed from the map in their destructor, so consistency is guaranteed.
Timers are destroyed by directly calling 'delete' in 3 places:
- when Document is destroyed while timers are still alive, in DOMTimer::contextDestroyed()
- when single-shot timer fired, in DOMTimer::fired()
- when JS calls clearTimer, in DOMTimer::removeById()
Comment 1 Dmitry Titov 2008-12-29 14:35:40 PST
Created attachment 26300 [details]
Proposed patch
Comment 2 Darin Adler 2009-01-02 10:58:55 PST
Comment on attachment 26300 [details]
Proposed patch

> -    static_cast<Document*>(context)->removeTimeout(timeoutId);
> +    DOMTimer* timer = static_cast<Document*>(context)->findTimeout(timeoutId);
> +    delete timer;

I personally would like this to just be a one liner without a local variable.

> -    ActiveDOMObject::contextDestroyed();
> +    ActiveDOMObject::contextDestroyed();  // Clears m_scriptExecutionContext.

Is adding this comment really an improvement?

Normally we use one space before the "//".

Comment 3 Dmitry Titov 2009-01-02 17:11:59 PST
Created attachment 26387 [details]
Patch with changes after review

Thanks Darin!!
All comments addressed. Seems ready to be landed.
Comment 4 Darin Adler 2009-01-02 18:27:48 PST