Changes session restore to use a normal load rather than preferring...

Changes session restore to use a normal load rather than preferring
the cache. We need to do this else we don't honor page expiration and
end up showing stale data for some sites.

BUG=21195
TEST=make sure session restore works.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30892

[sky, 2009-10-29 21:51:34]

Darin, could you review the renderer side and Brett the browser?

Thanks!

[Peter Kasting, 2009-10-29 21:59:28]

I really really really don't want to see this happen.  Comment left on bug.

[brettw, 2009-10-30 17:26:27]

LGTM on the implementation. I'll stay out of "whether this is a good idea" for
now.

http://codereview.chromium.org/341043/diff/2012/2022
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/341043/diff/2012/2022#newcode187
Line 187: if (entry.restore_type() == NavigationEntry:: RESTORE_LAST_SESSION)
Space after the ::

[darin (slow to review), 2009-10-30 18:13:38]

http://codereview.chromium.org/341043/diff/2012/2029
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/341043/diff/2012/2029#newcode2339
Line 2339: if (frame->provisionalDataSource()) {
note: this only changes the cache policy for the top level document.  are you OK
with that?

[sky, 2009-10-30 21:18:27]

On 2009/10/30 18:13:38, darin wrote:
> http://codereview.chromium.org/341043/diff/2012/2029
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/341043/diff/2012/2029#newcode2339
> Line 2339: if (frame->provisionalDataSource()) {
> note: this only changes the cache policy for the top level document.  are you
OK
> with that?

I've changed this as discussed. We now look at the data source of the top most
frame.
I also fixed the style issue from Brett.
New snapshot uploaded.

  -Scott

[darin (slow to review), 2009-10-30 21:23:15]

LGTM for common/ and renderer/ side.  It'll be so easy to follow this same
approach to implement Shift+Reload :-)

http://codereview.chromium.org/341043/diff/3015/2072
File chrome/renderer/navigation_state.h (right):

http://codereview.chromium.org/341043/diff/3015/2072#newcode169
Line 169: void set_cache_policy(WebKit::WebURLRequest::CachePolicy cache_policy)
{
might be nicer to call this "set_cache_policy_override" to make it clear that
this is an optional thing.

[brettw, 2009-10-30 22:31:44]

LGTM with some comment changes.

http://codereview.chromium.org/341043/diff/3015/2057
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/341043/diff/3015/2057#newcode74
Line 74: // Is this entry from the last session?
Can you expand on this comment a bit. What does it mean when this is false?

http://codereview.chromium.org/341043/diff/3015/2068
File chrome/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/341043/diff/3015/2068#newcode368
Line 368: // The entry is from the current session.
This comment isn't actually correct. IT's an entry that has been restored from
the current session. Can you explicitly mention "undo close tab"?

[sky, 2009-11-02 18:00:23]

All updated. I've also changed the semantics so that we only do this new
behavior if the last session didn't crash.

[sky, 2009-11-03 16:09:11]

Ping

  -Scott

On Mon, Nov 2, 2009 at 10:00 AM,  <sky@chromium.org> wrote:
> All updated. I've also changed the semantics so that we only do this new
> behavior if the last session didn't crash.
>
> http://codereview.chromium.org/341043
>

[brettw, 2009-11-03 17:01:25]

LGTM