[webkit-reviews] review granted: [Bug 22755] Next step to Timers in Workers: moving timer code from other places to DOMTimer. : [Attachment 26085] updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dmitry Titov <dimich at chromium.org>'s
request for review:
Bug 22755: Next step to Timers in Workers: moving timer code from other places
to DOMTimer.
https://bugs.webkit.org/show_bug.cgi?id=22755

Attachment 26085: updated patch
https://bugs.webkit.org/attachment.cgi?id=26085&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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.

r=me