21137 - Hook up reload/shift-reload/back/forward buttons to load attributes

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect, P1)

Description

[Scott Furman]

The load attributes for a page's load group need to be affected by the
use of the reload button or the shift key in conjunction with the reload
button.

 Reload:
    FORCE_VALIDATION

 Shift-Reload:
    FORCE_RELOAD

 Default; one of four choices, based on pref setting
    VALIDATE_NEVER
    VALIDATE_ONCE_PER_SESSION
    VALIDATE_HEURISTICALLY (new option, not previously available)
    VALIDATE_ALWAYS

Comment 1

[Scott Furman]

Rick, I'm not sure that you're the right person to give this to, but since
you're Mr. Load Group...

Comment 2

[Bill Law]

It appears that the following things would need to be done.

1. Detect the shift-key being down when Reload is pressed.  This would
seem to be easy (there's a comment to that effect in navigator.xul).  I
tried to check event.shiftKey (the comment says event.shiftDown but the
event object appears to have only a shiftKey property).

I tried to tweak navigator.xul and navigator.js to deal with this but
ran into problem.  The actual event handler would appear to be in a
<broadcaster> node that has the attribute
oncommand="BrowserReallyReload(0)".  I changed that to pass |event|
instead of 0.  Inside that function, if examine the event object (using
"for (prop in event)...") I get:

event.charCode=0
event.keyCode=0
event.altKey=true
event.ctrlKey=true
event.shiftKey=true
event.metaKey=true
event.screenX=0
event.screenY=0
event.clientX=0
event.clientY=0
event.button=0
event.clickCount=0
event.relatedNode=null
JavaScript Error: uncaught exception: [Exception... "Method not
implemented"  co
de: "-2147467263" nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"
location: "
chrome://navigator/content/navigator.js Line: 784"]

The JS exception is generated by doing event["view"] (I think).  I get
the exact same output whether the shift key is pressed or not.  I'm not
sure what's up with this but it doesn't appear that one can
deduce whether the shift key is pressed when a button is clicked.  But
the event might undergo some transformation on its way through the
broadcaster mechanisms.  I'm not sure where to grab it before it morphs,
though, if that's what's happening.

This seems to be a bug (or else I'm operating under faulty assumptions,
which is probably equally likely).

I'll try again with an onclick handler right on the <titledbutton>.

2. Presuming we got past #1, we then would need to pass the proper "load
flags" to the nsBrowserInstance object.  That is easy enough.

3. nsBrowserInstance::Reload calls nsSessionHistory::Reload, which does
not take a "load flag" argument.  Thus, we need to add an argument to
that method (which wouldn't be too hard).  Radha is the session history
expert (I've cc:'d her).

4. nsSessionHistory::Reload calls nsSessionHistory::Goto which also
doesn't have a "load flag" argument.  So we'd probably have to add it
here, too.

5. nsSessionHistory::Goto calls nsHistoryEntry::Load, which also would
need a "reload flag" argument added to it.

6. nsHistoryEntry::Load calls nsWebShell::LoadURL with a "load flag"
argument it calculates (either LOAD_NORMAL or LOAD_HISTORY).  This code
would have to be enhanced to accept the "load flag" argument and
incorporate that into what it passes on to the WebShell.

7. The web shell would have to pass along the "load flag" it gets on the
LoadURL call to the load group (or whomever).  I presume that would all
work OK if we passed in the right value.

I've glossed over some details.  Specifically, I'm not sure how the
flags mentioned in the bug (FORCE_* and VALIDATE_*) relate to the "load
flag" values passed around already (the only values I could see were
LOAD_NORMAL and LOAD_HISTORY).  I'm guessing that the FORCE_* and
VALIDATE_* values are or'd into this or something.  VALIDATE_* probably
isn't pertinent till the request gets to necko.

So my take is that there are a few hoops that have to be jumped through
but no major hurdles, aside from the apparent bug (or misunderstanding
on my part) described under #1.  I'll be glad to tackle the .xul/.js and
browser instance changes.  I'd like assistance from Radha on the changes
to nsSessionHistory.cpp.

Comment 3

[Scott Furman]

> I've glossed over some details.  Specifically, I'm not sure how the
> flags mentioned in the bug (FORCE_* and VALIDATE_*) relate to the "load
> flag" values passed around already (the only values I could see were
> LOAD_NORMAL and LOAD_HISTORY).  I'm guessing that the FORCE_* and
> VALIDATE_* values are or'd into this or something.  VALIDATE_* probably
> isn't pertinent till the request gets to necko.


I didn't know about LOAD_HISTORY.  It's not used by Necko and it's
probably a mistake for it to be combined in the same variable with the
Necko load attributes since its bit pattern could someday collide with Necko
flags.

LOAD_NORMAL should be replaced by one of the six flags (FORCE_*
and VALIDATE_*).  This doesn't necessarily need to happen before the
flags enter the webshell, though.

Comment 4

[Bill Law]

I opened bug 21216 to report that the event.<x>Key properties are not set
properly when clicking on a <titledbutton>.

Comment 5

[Scott Furman]

One very important thing I forgot to mention: Using the back or forward button
should set the load attribute to VALIDATE_NONE, regardless of the cache pref
setting.  This is so that history navigation doesn't cause a network request to
be sent to the server unless, for some reason, the requested page is not in the
cache at all.

Comment 6

[leger]

Bulk move of all Cache (to be deleted component) bugs to new Networking: Cache
component.

Comment 7

[Warren Harris]

Moving Rick's M14 bugs to M13 (since he won't be here for M14). He can triage
them to M15 as appropriate.

Comment 8

[Warren Harris]

Cache bug => Gordon.

Comment 9

[Scott Furman]

*** Bug 25597 has been marked as a duplicate of this bug. ***

Comment 10

[Warren Harris]

Seems non-essential for beta. Moving to M15.

Comment 11

[Scott Furman]

> Seems non-essential for beta. Moving to M15.

Until this bug is fixed, back/forward buttons will always hit the server rather 
than  loading document from the cache and we can't get the once-per-session 
reload behavior that is the default in 4.x.  Seems to me like this could be a 
big performance win for a relatively small amount of effort.

Comment 12

[Oleg Rekutin]

Pardon the spam, but how can you have a bug be non-beta M15, if it blocks a M14 
4xp, beta1 bug (14772)?

Comment 13

[Warren Harris]

*** Bug 30852 has been marked as a duplicate of this bug. ***

Comment 14

[roberts]

> Until this bug is fixed, back/forward buttons will always hit the server 
rather 
> than  loading document from the cache and we can't get the once-per-session 
> reload behavior that is the default in 4.x.  Seems to me like this could be a 
> big performance win for a relatively small amount of effort.

Pardon me, but the bug I reported (30852), which this has been marked as a
duplicate of this bug exhibits the exact opposite of this behavior: the server
is _never_ hit after the image is initially loaded. The cached version of the
image is the only one that gets redisplayed forever after. Reloading does not
cause a new version of the image to loaded from the server. Reloading the source
URL does not cause a fresh version of the image to be loaded from the server.

--Doug

Comment 15

[roberts]

Additional information: This behavior can be seen in a surprising number of web
pages: abcnews.com presents the stock market indices as image data. View the
page once and the market indices are thereafter never updated with subsequent
reloads of the page. http://finance.yahoo.com/q?s=^IXIC&d=1d is another example
of this behavior. 

--Doug

Comment 16

[Warren Harris]

Moving what's not done for M15 to M16.

Comment 17

[Joseph Elwell]

*** Bug 34984 has been marked as a duplicate of this bug. ***

Comment 18

[davidm]

reassign to me since I took 29343 which is related

Comment 19

[davidm]

Attachment: Proposed fix

Comment 20

[davidm]

*** Bug 29343 has been marked as a duplicate of this bug. ***

Comment 21

[davidm]

reassign to travis since this is doc loader work. I have attached a patch which 
solves everything but shift reload since that requires some UI work.

Comment 22

[Radha on family leave (not reading bugmail)]

*** Bug 32469 has been marked as a duplicate of this bug. ***

Comment 23

[Claudius Gayle]

the dupe of this bug 32469 had already received the nsbeta2+ blessing. DOes that just automatically carry over?

Comment 24

[R.K.Aa.]

*** Bug 38049 has been marked as a duplicate of this bug. ***

Comment 25

[davidm]

Who is doing the doc loader work now that travis no longer walks in the light?;) 
I would like to at least check in my patch pretty soon so we are doing somewhat 
correct caching. It should also really help back and forward performance.

Comment 26

[Simon Fraser [no longer active]]

davidm: just do it, i say.

Comment 27

[davidm]

I didn't know I worked at Nike. I checked in my code. What remains to be done is 
the JS to call reload with the proper parameter when shift is down. Bill 
volunteered to do this a couple months back so I am going to reassign this to 
him.  The value passed to the docshell reload is reloadBypassProxyAndCache = 3. 
Let me know if you need any help.

Comment 28

[Bill Law]

Bug 21216 seems to have been fixed so the .js code can work.  I'll have to see 
if the browser instance load functions (and underlying code) have changed a lot 
from when I puzzled this out way back when.  I'm taking the bug and will try to 
get to it ASAFWIC (as soon as feature work is completed).

Comment 29

[leger]

Putting on [nsbeta2+] radar for beta2 fix.

Comment 30

[Tom Everingham]

per pdt's request on status - Shift reload is still not working on today's 
build.  Work is in progress.

Comment 31

[Asa Dotzler [:asa]]

*** Bug 39877 has been marked as a duplicate of this bug. ***

Comment 32

[don]

Move to M18.

Comment 33

[leger]

Putting on [nsbeta2-] per Nav Beta2 review today.

tever, we believe shift-reload is working now.  As long as this is working, a 
minus for rest of workin beta2.

Comment 34

[leger]

Hey, can you mark this fixed now?

Comment 35

[Tom Everingham]

No, this should remain open... shift-reload is pulling from cache and not the 
network.  According to Bill here is still work to be done on this.

Comment 36

[johng]

nav triage team:
nsbeta3+
perf keyword
IF we don't fix this, the performance of forward and back will seem quite slow.

Comment 37

[Asa Dotzler [:asa]]

*** Bug 46351 has been marked as a duplicate of this bug. ***

Comment 38

[Devoti Paolo]

Adding comment from DUP 46351
IMHO if you hit reload (not counting debug) is because you wish to reload the 
page from the net. If is ever needed a reload from cache the use of the button 
and shift button should be exchanged.

Comment 39

[Wayne Davison]

The original description contains an accurate list of what is supposed to be 
done on reload.  A reload by itself checks the web site to see if the page is 
newer than the cached version (FORCE_VALIDATION), if it is not newer, we stick 
with what we have.  Shift-reload is used to discard the cached data and fetch a 
new copy (FORCE_RELOAD), even if the web server would have told us we had the 
most recent version.

Comment 40

[Ben Bucksch (:BenB)]

For the case my opinion counts, I prefer to have reload trigger FORCE_RELOAD.
This is what I expect, since it is called "Reload", not "Reload, if the browser
or the server thinks, this is necessary". If the user does explicitly request a
reload, the cost of a full reload is acceptable IMO.

> The original description contains an accurate list of what is supposed to be 
> done on reload.

This is what previous *Navigators* did, but IIMO, MSIE does a full reload.

So, I support the suggestion to swap the meaning of Reload and Shift-Reload.

Comment 41

[Wayne Davison]

> This is what previous *Navigators* did, but IIMO, MSIE does a full reload.

No, IE optimizes the "refresh" (what they call it) so that it doesn't download
data that hasn't changed.  The only difference that I know of is that IE binds
the force-reload command to ctrl-refresh rather than shift-reload.

I would be quite interested to know in what circumstances you find a forced
reload to be useful.  I know of no servers that get this wrong, so please
enlighten me if there is a problem.

IMO, ignoring the cache would result in unacceptable delays for modem users
since it would require re-downloading every gif, script, style-sheet, etc. on
the page. Usually the user just wants to see if the page has been updated, so
they just want to download what is different from last time.  This is more
optimal for both the server and the user, since it reduces web traffic by only
sending what has changed between the cache and the current data.

Comment 42

[Ben Bucksch (:BenB)]

> I know of no servers that get this wrong, so please
> enlighten me if there is a problem.

I did have occasional problems with normal reloads. If that happens, it is very
bad and equals data loss, because, after a reload, I assume, I really have the
latest version. Data loss causes FUD.

> IMO, ignoring the cache would result in unacceptable delays for modem users

I don't think, they are unacceptable unless you consider the speed of websurfing
in general unacceptable. After all, the user *explicitly* requested a "reload".

> since it would require re-downloading every gif, script, style-sheet, etc. on
> the page.

IIRC, one of the problems I had with normal reload was exactly that the browser
didn't check, if images had changed.

> Usually the user just wants to see if the page has been updated, so
> they just want to download what is different from last time.

Pages at risk to update while they are displayed in the browser window usually
specify a meta-refresh anyway (e.g. <http://my.netscape.com>).

Making sure not to use old data when the page is displayed (after a click or a
meat-refresh) is the job of the browser and shouldn't require a manual reload.

--

IMO, you should do what I said. I said "Re*load*", so, please, *load*. Otherwise
rename the button to "Recheck".

Comment 43

[Don Beusee]

I agree reload should do an unconditional reload of everything.  One problem is
some images are generated on the fly (like counters), and hitting reload should
cause those images to get rebuilt.  If the user hit reload, there is obviously a
good reason.  So obey their wish.  If they don't hit reload, then by all means
use the cache.

Comment 44

[Ben Bucksch (:BenB)]

Suggestion: Either
- Call it "Reload", bind the normal "Reload" to FORCE_RELOAD and a
shift-"Reload" to FORCE_VALIDATION (i.e. the opposite of 4.x), and release note.
- Call it "Check" or "Recheck"* and keep the keybindings as they were in 4.x
(i.e. the opposite to the first suggestion)

In both cases, validate *all* data, i.e. the page, images, other embedded
objects, stylesheets, scripts etc.. Is that the case for Mozilla now? If not,
I'd file a new bug for that.

* IMO, do not call it "Refresh", since that usually means "Redisplay" and is
used for the case the app garbled the display.

Comment 45

[neeti]

*** Bug 46735 has been marked as a duplicate of this bug. ***

Comment 46

[Daniel Veditz [:dveditz]]

Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so 
the queries don't get all screwed up.

Comment 47

[Bill Law]

I have a fix ready, but another bug blocks this one (bug 48243).

I will attach the fix forthwith.  Note that the fix will not work, by itself, 
till bug 4823 is fixed.

However, you can work around this by applying a simple patch:

Change the "oncommand=" to "onclick=" for the <broadcaster id="canReload"> at 
about line 83 in navigator.xul.  Note that this might break some other things 
(like the Reload menu item or reload shortcut keys).

Lastly, I did not relabel the button or change what it does if you don't hold 
the shift-key down.  It didn't seem as if a consensus exists about making those 
changes.  If anybody wants a different default (or to change the button/menuitem 
labels), then please open a separate bug for those problems.

Comment 48

[Bill Law]

Attachment: Fix to add shift-reload support to navigator.js

Comment 49

[Ben Bucksch (:BenB)]

Posted <news://news.mozilla.org/[e-mail]> to xpfe about
changing the behaviour or naming of the button.

Comment 50

[Bill Law]

Fix checked in.

Comment 51

[Bill Law]

Please note that the problem doesn't go away till bug 48243 is fixed.  Should I 
leave this one open till that one's fixed?  Probably, so I'm going to reopen 
this.  Sorry for the spam.

Comment 52

[Bill Law]

Blocker is fixed, making this one RESOLVED - FIXED.

Comment 53

[Tom Everingham]

verified:
WinNT 2000091108
Linux rh6 2000091108
Mac os8.6 2000090820

Comment 54

[Martin Sch]

>For the case my opinion counts, I prefer to have reload trigger FORCE_RELOAD.
>This is what I expect, since it is called "Reload", not "Reload, if the browser
>or the server thinks, this is necessary".

I agree with what Ben says. I normally hit reload because something is wrong
with caching and I want to have a real new version. And mozilla then always
requeststhe page with "If-modified-since" and the cache problem remains.

This causes lots of problems with users that don't know how http works when they
have cache or proxy problems. They are smart enuf to hit reload, but if there is
no real reload, the problem remains.