[webkit-reviews] review denied: [Bug 41178] Timed refresh in subframes isn't stopped when going into b/f cache : [Attachment 59706] proposed fix

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Alexey Proskuryakov
<ap at webkit.org>'s request for review:
Bug 41178: Timed refresh in subframes isn't stopped when going into b/f cache
https://bugs.webkit.org/show_bug.cgi?id=41178

Attachment 59706: proposed fix
https://bugs.webkit.org/attachment.cgi?id=59706&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>

>  
> Index: WebCore/loader/FrameLoader.cpp
> ===================================================================
> --- WebCore/loader/FrameLoader.cpp	(revision 61698)
> +++ WebCore/loader/FrameLoader.cpp	(working copy)
> @@ -491,10 +491,7 @@ void FrameLoader::stopLoading(UnloadEven
>  #endif
>      }
>  
> -    // tell all subframes to stop as well
> -    for (Frame* child = m_frame->tree()->firstChild(); child; child =
child->tree()->nextSibling())
> -	   child->loader()->stopLoading(unloadEventPolicy);
> -
> +    // FIXME: This will cancel redirection timer, which really needs to be
restarted when restoring the frame from b/f cache.
>      m_frame->redirectScheduler()->cancel();

Don't we restore redirect timers for the main frame when restoring from the
page cache?

If so, is it acceptable to have this outstanding FIXME for subframes?


>  
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 61786)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> +2010-06-24  Alexey Proskuryakov  <ap at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=41178
> +	   Timed refresh in subframes isn't stopped when going into b/f cache
> +
> +	   This is a slow test, because a fast redirect results in replacing
the current item in b/f
> +	   list, so back/forward cache doesn't get involved. But this code path
must be tested.
> +

Even if it's an inconvience to everyone while folks are still working at
parallelizing and otherwise speeding up the layouttests, I agree we absolutely
need this type of test, even if it's slow.

> +	   * fast/history/timed-refresh-in-cached-frame-expected.txt: Added.
> +	   * fast/history/timed-refresh-in-cached-frame.html: Added.

Maybe we should to create a new TLD in LayoutTests 'slow' for this type of
test?

r- because of my question on the FIXME