[webkit-reviews] review granted: [Bug 28683] Timers from cached pages fire instantly rather than the specified timeout delay : [Attachment 38572] Proposed patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has granted Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 28683: Timers from cached pages fire instantly rather than the specified
timeout delay
https://bugs.webkit.org/show_bug.cgi?id=28683

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

------- Additional Comments from Brady Eidson <beidson at apple.com>
r=me with minor tweaks - prepare for some extreme nit-pickyness ;)

> +2009-08-25  Dmitry Titov  <dimich at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Timers from cached pages fire instantly rather than the specified
timeout delay

"...rather than after the specified timeout delay," maybe.

> +This test verifies that when page is loaded from the page cache on
navigation back, the suspended timers are resumed for a duration left when they
were suspended. This is a test for
https://bugs.webkit.org/show_bug.cgi?id=28683.
> +The test navigates to a page, starts a timer and then navigates to another
page and back. It then measures time when the timer is actually fired and makes
sure that it is at least the time set at the beginning. If successful, it
outputs 'PASS' below.
> +PASS with 206 ms delay.

Eric is dead on here - the delay will not always be exactly 206 ms, even on the
same machine over multiple runs.  Also, I think it is possible that a run of
the test could hit 200ms exactly.  
Just "is equal to or greater than 200ms" would be fine.

> +	   (WebCore::CachedPage::restore):
> +	   remove duplicated Frame::restore() call, it should be done only once
in FraeLoader::open(cachedFrame)

Typo "FrameLoader"

> +	   * page/DOMTimer.cpp: added a debug-only flag and ASSERT to catch
out-of-order suspense/restore.
> +	   (WebCore::DOMTimer::DOMTimer): ditto.
> +	   (WebCore::DOMTimer::suspend): ditto.
> +	   (WebCore::DOMTimer::resume): ditto.
> +	   * page/DOMTimer.h: ditto.
> +

You capitalized all statements in the LayoutTest ChangeLog - should capitalize
all the WebCore/ChangeLog stuff, too.