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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 25991: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=25991&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I'm guessing that a lot of these comments are about code that you just moved,
and didn't write, so you can ignore those comments if you like. If it was new
code I'd probably fix all these things!

> +const int cMaxTimerNestingLevel = 5;
> +const double cMinimumTimerInterval = 0.010;

Since these are local to this source file, they should be "static const". Our
coding style doesn't give a specific style for these, but I normally use no
prefix at all (so no "c"), but the names are arguably OK as is. It'd be nice to
be consistently use either "maximum" and "minimum" or "max" and "min". This
uses one of each.

> +DOMTimer::TimeoutsMap& DOMTimer::getTimeoutsMap()
> +{
> +    DEFINE_STATIC_LOCAL(TimeoutsMap, map, ());
> +    return map;
> +}

We never use the word "get" in the name of a function like this. We reserve
"get" for functions that return their results in a non-standard way, such as an
out parameter. We'd just call this timeoutsMap().

> +    ASSERT(!getTimeoutsMap().get(m_timeoutId));

We have "contains" for boolean uses like this. Better to use that instead of
get.

> +    getTimeoutsMap().set(m_timeoutId, this);
> +
> +    // Use a minimum interval of 10 ms to match other browsers, but only
once we've
> +    // nested enough to notice that we're repeating.
> +    // Faster timers might be "better", but they're incompatible.

The comment above describes the "interval" part of the math, but the
immediately succeeding lines of code implement something else (conversion from
ms to s and minimum interval of 1 ms). I find that confusing.

> +    double interval = std::max<double>(0.001, timeout * 0.001);

The 0.001 should be a named constant too. Maybe it's clear to you that they're
milliseconds, but I found it unclear.

Why max<double> instead of just max? Both expressions are doubles, so it should
work without the type being specified explicitly.

> +    typedef HashMap<int, DOMTimer*> TimeoutsMap;
> +    static TimeoutsMap& getTimeoutsMap();

Does this really need to be a static member function? If it was a plain old
non-member function then it could be in the .cpp file and we could avoid adding
new includes to the header. I'd prefer that if possible.

> +    DOMTimer* timer = new DOMTimer(scriptExecutionContext(), a, t,
singleShot);

I'm not really comfortable with this interface. It's ugly that calling new
gives you a pointer that's already owned by the "window" object; sets off all
the alarm bells -- who owns this object? Could we change this to be a function
call rather than new? The "new" part could be done inside the function so it's
encapsulated in the DOMTimer class.

Is a global map really a good idea if we're going to be doing this on multiple
threads?

> +    delete DOMTimer::findById(timeoutId);

It seems cleaner to have the DOMTimer function be more like "removeById".

It's a bug that now one window can cancel timers that were created in another
window if it guesses the timer ID. That needs to be fixed.

review- for now given the comments above.