[Webkit-unassigned] [Bug 22755] Next step to Timers in Workers: moving timer code from other places to DOMTimer.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Dec 27 14:12:27 PST 2008


darin at apple.com changed:

           What    |Removed                     |Added
  Attachment #26085|review?                     |review+
               Flag|                            |

------- Comment #8 from darin at apple.com  2008-12-27 14:12 PDT -------
(From update of attachment 26085)
> +    double intervalMilliseconds = std::max(oneMillisecond, timeout * oneMillisecond);

Should just be "max", with "using namespace std" at the top of the .cpp file.

>  DOMTimer::~DOMTimer()
>  {
> -    if (m_action) {
> -        JSC::JSLock lock(false);
> -        delete m_action;
> -    }
> +    delete m_action;
> +}

Given that we don't need locking around the delete now, we can move to using an
OwnPtr instead of explicit deletion.

> +int DOMTimer::install(ScriptExecutionContext* context, ScheduledAction* action, int timeout, bool singleShot)
> +{
> +    DOMTimer* timer = new DOMTimer(context, action, timeout, singleShot);
> +    return timer->m_timeoutId;
> +}

This needs a comment to explain ownership. It's always confusing when someone
can just call "new" and then drop the result and somehow know there won't be a
storage leak. So requires a brief comment.

> +    // FIXME: move the timeout map and API to ScriptExecutionContext to be able
> +    // to create timeouts from Workers.

I don't think we need this comment in multiple places. Also, please use
sentence capitalization in comments like this. Thus "Move" rather than "move".

> +    // This is static, so ok to assign after deleting the instance.
> +    m_timerNestingLevel = 0;

I don't think static data members should use the "m_" prefix; the need for this
comment is a good example of why. But further, I'd rather just have global
variables with internal linkage rather than static data members anyway. They
are private to the .cpp file and can be added and removed without touching a
header. Anyway, not new to this patch and not your fault.


Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

More information about the webkit-unassigned mailing list