[webkit-reviews] review denied: [Bug 53733] Timers can fire after a frame has been put into the page cache : [Attachment 124708] Patch

[bugzilla-daemon at webkit.org]

Mihai Parparita <mihaip at chromium.org> has denied Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 53733: Timers can fire after a frame has been put into the page cache
https://bugs.webkit.org/show_bug.cgi?id=53733

Attachment 124708: Patch
https://bugs.webkit.org/attachment.cgi?id=124708&action=review

------- Additional Comments from Mihai Parparita <mihaip at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124708&action=review


Almost there.

> Source/WebCore/dom/ActiveDOMObject.cpp:63
> +    , m_suspendUptodate(false)

The name of this is slightly awkward. Maybe call it m_suspendIfNeededCalled?

> Source/WebCore/dom/ScriptExecutionContext.cpp:208
> +	   ASSERT(iter->first->scriptExecutionContext() == this &&
iter->first->m_suspendUptodate);

Can you move the new check to its own ASSERT statement, so that that if it
fails it's easier to see which one was responsible (applies below too)?

Also, there should be a getter for the property (which should be private),
since it should not be mutable by others.

> Source/WebCore/page/SuspendableTimer.cpp:39
> +#if !ASSERT_DISABLED

Why this change (making m_active be available for release builds)?