[webkit-reviews] review requested: [Bug 28683] Timers from cached pages fire instantly rather than the specified timeout delay : [Attachment 38584] Updated patch.

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has asked  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 38584: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=38584&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
(In reply to comment #3)
> (From update of attachment 38572 [details])
> +PASS with 206 ms delay.
> Will cause this to fail on some systems.

Replaced with just 'PASS', output real delay only if failed - in case reduced
timeout will cause problems it'd be nice to know what it was in a test run.

(In reply to comment #5)
> (From update of attachment 38572 [details])
> +#ifndef NDEBUG
> +    , m_suspended(false)
> +#endif
> 
> Seems like you should guard this with #if !ASSERT_DISABLED instead of the
> NDEBUG check, just in case some crazy person comes along and turns on
> assertions on a Release build, or turns them off on a Debug build.

Done.

(In reply to comment #6)
> Does the delay during the test need to be as large as 200 ms?  Ideally we'd
> like our regression tests to be much quicker than that.

Replaced with 100 ms. I measured time needed on my Mac Pro, debug build, in
DRT, to navigate to another page and back - it was a bit under 50ms. I figured
double this to be sure it fails in most cases. If it doesn't fail on a slow
machine it's probably ok since it will fail on retail build for sure.

(In reply to comment #7)
> (From update of attachment 38572 [details])
> r=me with minor tweaks - prepare for some extreme nit-pickyness ;)
> > +	     Timers from cached pages fire instantly rather than the specified
timeout delay
> "...rather than after the specified timeout delay," maybe.

Done.

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

Done.

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

Done.

(In reply to comment #9)
> Although there's clearly a bug in code, it's not clear to me what the right
> behavior is. If we start a 1 second timer and suspend the page for 5 seconds,

> I'd personally expect it to fire immediately after resuming. As Ada said,
> testing other browsers is the key.

Tested FF3.5, it behaves as this patch. Attached the test page for reference.