Plumb shift-reload down into newly-added shift-reload API.

linux: plumb shift-reload down into new shift-reload API

Currently Linux-only, but the Mac/Win bits should now be trivial.

While I was add it, I tweaked a NavigationController function
that took a bare boolean into an enum to make the call sites more
explicit about what they're doing.

(In most places I added new functions that call into a shared
backing function; this is so I don't need to change every single
caller of e.g. Reload() to pass through an extra flag that will
be the same for almost every caller.)

BUG=1906
TEST=visit astronomy picture of the day; hit reload, picture pops
up quickly; hit shift-reload, picture loads slowly

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

[Evan Martin, 2010-02-16 11:07:21]

Randomly picking Peter since I think I remember him being opinionated about
this; if you don't care to look at it, lemme know and I'll find someone else.

http://codereview.chromium.org/594063/diff/1/5
File chrome/browser/gtk/accelerators_gtk.cc (right):

http://codereview.chromium.org/594063/diff/1/5#newcode125
chrome/browser/gtk/accelerators_gtk.cc:125: { GDK_F5, IDC_RELOAD_ALL,
GDK_SHIFT_MASK },
Not sure if these are the right keys for it.

http://codereview.chromium.org/594063/diff/1/6
File chrome/browser/gtk/browser_toolbar_gtk.cc (right):

http://codereview.chromium.org/594063/diff/1/6#newcode631
chrome/browser/gtk/browser_toolbar_gtk.cc:631: modifier_state & GDK_SHIFT_MASK)
{
XXX also check control-click?

[viettrungluu, 2010-02-16 14:40:54]

http://codereview.chromium.org/594063/diff/1/2
File chrome/app/chrome_dll_resource.h (right):

http://codereview.chromium.org/594063/diff/1/2#newcode73
chrome/app/chrome_dll_resource.h:73: #define IDC_HOME                       
33004
If you do this -- renumber things here -- you absolutely *must* get the
appropriate .xib files updated, or else you will break Mac Chromium.

[Nico, 2010-02-16 14:47:12]

On 2010/02/16 14:40:54, viettrungluu wrote:
> http://codereview.chromium.org/594063/diff/1/2
> File chrome/app/chrome_dll_resource.h (right):
> 
> http://codereview.chromium.org/594063/diff/1/2#newcode73
> chrome/app/chrome_dll_resource.h:73: #define IDC_HOME                       
> 33004
> If you do this -- renumber things here -- you absolutely *must* get the
> appropriate .xib files updated, or else you will break Mac Chromium.

And by this, trungl means that you have to ping some mac person when checking
this in, so that the mac person can do that.

[viettrungluu, 2010-02-16 14:51:57]

On 2010/02/16 14:47:12, Nico wrote:
> And by this, trungl means that you have to ping some mac person when checking
> this in, so that the mac person can do that.

Or get one of us to preemptively make the change (to IDC_* values and to the
.xib files), thereby avoiding even momentary breakage. Feel free to ping me,
btw.

[viettrungluu, 2010-02-16 15:46:23]

Other than trivial style questions (and the emergency PSA), the changes here
seem fine.

http://codereview.chromium.org/594063/diff/1/4
File chrome/browser/browser.h (right):

http://codereview.chromium.org/594063/diff/1/4#newcode353
chrome/browser/browser.h:353: void ReloadAll();  // AKA shift-reload.
I'm not a big fan of "ReloadAll" as a name since, in the context of Browser,
it's not obvious what it does (reload all tabs?). I might prefer "ForceReload".
Or I might prefer just using your |ReloadInternal()| as the public interface
(called |Reload(bool force)| or something like that, of course); this would
require other code changes.... This also prevents |Browser|-bloat.

http://codereview.chromium.org/594063/diff/1/5
File chrome/browser/gtk/accelerators_gtk.cc (right):

http://codereview.chromium.org/594063/diff/1/5#newcode125
chrome/browser/gtk/accelerators_gtk.cc:125: { GDK_F5, IDC_RELOAD_ALL,
GDK_SHIFT_MASK },
On 2010/02/16 11:07:21, Evan Martin wrote:
> Not sure if these are the right keys for it.

People have also claimed that Ctrl-Shift-F5 should also work. (I think it's
moderately ridiculous, but whatever.)

http://codereview.chromium.org/594063/diff/1/7
File chrome/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/594063/diff/1/7#newcode182
chrome/browser/tab_contents/navigation_controller.cc:182: void
NavigationController::ReloadInternal(bool check_for_repost, ReloadType reload) {
Ack! 80 columns.

And I think you should rename |reload| |reload_type|.

http://codereview.chromium.org/594063/diff/1/7#newcode950
chrome/browser/tab_contents/navigation_controller.cc:950: void
NavigationController::NavigateToPendingEntry(ReloadType reload) {
Ditto about |reload| -> |reload_type|.

http://codereview.chromium.org/594063/diff/1/8
File chrome/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/594063/diff/1/8#newcode443
chrome/browser/tab_contents/navigation_controller.h:443: void
ReloadInternal(bool check_for_repost, ReloadType reload);
|reload| -> |reload_type|?

http://codereview.chromium.org/594063/diff/1/8#newcode446
chrome/browser/tab_contents/navigation_controller.h:446: void
NavigateToPendingEntry(ReloadType reload);
Ditto.

http://codereview.chromium.org/594063/diff/1/9
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/594063/diff/1/9#newcode190
chrome/browser/tab_contents/tab_contents.cc:190:
NavigationController::ReloadType reload) {
<nag>reload_type?</nag>

http://codereview.chromium.org/594063/diff/1/9#newcode208
chrome/browser/tab_contents/tab_contents.cc:208:
NavigationController::ReloadType reload,
Ditto.

http://codereview.chromium.org/594063/diff/1/9#newcode715
chrome/browser/tab_contents/tab_contents.cc:715:
NavigationController::ReloadType reload) {
I'm pretty good at this nagging thing, if I may say so myself.

http://codereview.chromium.org/594063/diff/1/10
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/594063/diff/1/10#newcode324
chrome/browser/tab_contents/tab_contents.h:324: virtual bool
NavigateToPendingEntry(NavigationController::ReloadType reload);
|reload_type|?

[darin (slow to review), 2010-02-16 18:32:37]

http://codereview.chromium.org/594063/diff/1/8
File chrome/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/594063/diff/1/8#newcode139
chrome/browser/tab_contents/navigation_controller.h:139: RELOAD_ALL  // Reload
all subresources, aka shift-reload.
I think you misunderstand the difference between reload and shift+reload.  It
has nothing to do with subresources.  In both cases, subresources are reloaded.

The difference is that shift+reload says to ignore the locally cached copy as
well as any copies held by proxy caches.  Shift+reload means do an end-to-end
refetched from the origin server.

Normal reload means that we should force an end-to-end validation of any cached
copies before loading them again from cache.  This applies to all resources
including resources.

If you look at net/base/load_flags.h, you can see that "reload" is akin to
LOAD_VALIDATE_CACHE and "shift+reload" is akin to LOAD_BYPASS_CACHE.  They are
even documented as such!

There is one more subtle difference between reload and shift+reload:  reload
preserves form values just like back/forward navigations, whereas shift+reload
resets form values.

[Evan Martin, 2010-02-16 18:47:30]

weep.

I verified with this change that clicking reload on a page with a large image
reloaded the page and the image showed quickly (from cache), where as
shift-reload made the image load slowly (presumably from net).  Is it possible I
got the code right and just the naming wrong?

(What I had meant by "reload subresources" in my descriptions was "forcibly
reload subresources", which now that I see your message I see is not at all a
good description.  The reason I got into this weird definition is that the
reload button on a browser does this "force reload" behavior on the page
itself...)

[Evan Martin, 2010-02-16 18:49:55]

trying that wording one more time:

normal reload:
 refetch page, ignoring all cache control
 load images/subresources using if-modified-since headers if applicable

shift-reload:
 refetch page, ignoring all cache control
 load images/subresources, ignoring all cache control

Can you see why in the second case I was calling that "refresh" on the images? 
I will rename it.

[Evan Martin, 2010-02-16 19:07:09]

I think I still misunderstood you.  :(

One more comment: "This applies to all resources including resources.", I think
you meant "subresources".  So my previous map was wrong.

Normal reload: obeys cache control for page and subresources
Shift-reload: ignores caching headers for page and subresources

Yes?

[Peter Kasting, 2010-02-16 19:11:27]

I'm wrong to review this.

Darin is the right one to say what things should and should not do.  Brett is
probably the person who's done the most work with the NavigationController.

[darin (slow to review), 2010-02-16 19:11:35]

On Tue, Feb 16, 2010 at 10:49 AM, <evan@chromium.org> wrote:

> trying that wording one more time:
>
> normal reload:
>  refetch page, ignoring all cache control
>  load images/subresources using if-modified-since headers if applicable
>

Ah yes, there's a FIXME in FrameLoader.cpp about it not using
if-modified-since headers for the document in this case.

Have you traced down to the network stack level to see what load flags are
being set in each case?  WebCore wants to set HTTP headers to validate its
own cache, but I'm afraid that if it is also setting ReloadIgnoringCacheData
that our disk cache will be evicted even for an otherwise conditional
request.

I think good names for these NavigationController enums are probably
something like:

RELOAD_VALIDATING_CACHE
RELOAD_IGNORING_CACHE

-Darin



>
> shift-reload:
>  refetch page, ignoring all cache control
>  load images/subresources, ignoring all cache control
>
> Can you see why in the second case I was calling that "refresh" on the
> images?
> I will rename it.
>
>
> http://codereview.chromium.org/594063
>

[Evan Martin, 2010-02-18 13:17:45]

trung/estade: can you check that I didn't break mac and that the gtk bits look
ok?

darin: I have one more question for you.


On 2010/02/16 19:11:35, darin wrote:
> Have you traced down to the network stack level to see what load flags are
> being set in each case?  WebCore wants to set HTTP headers to validate its
> own cache, but I'm afraid that if it is also setting ReloadIgnoringCacheData
> that our disk cache will be evicted even for an otherwise conditional
> request.

Darin, can you give me more info and what to check here?

[viettrungluu, 2010-02-18 15:59:43]

LGTM on not breaking Mac (well, that TODO line is longer than 80 characters, but
anyway).

Let me know when it all lands, so I can hook up the Mac side and fix the IDs....

[Evan Stade, 2010-02-18 17:11:03]

gtk lgtm

http://codereview.chromium.org/594063/diff/9003/8016
File chrome/browser/gtk/accelerators_gtk.cc (right):

http://codereview.chromium.org/594063/diff/9003/8016#newcode124
chrome/browser/gtk/accelerators_gtk.cc:124:
GdkModifierType(GDK_CONTROL_MASK|GDK_SHIFT_MASK) },
nit: spaces around |

[darin (slow to review), 2010-02-18 22:55:42]

LGTM

http://codereview.chromium.org/594063/diff/9003/8020
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/594063/diff/9003/8020#newcode192
chrome/browser/tab_contents/tab_contents.cc:192: case
NavigationController::RELOAD_VALIDATING_CACHE:
nit: maybe you should just stick with RELOAD / RELOAD_IGNORING_CACHE everywhere
instead of sometimes using RELOAD_VALIDATING_CACHE to mean the same thing as
RELOAD?