Closed Bug 21137 Opened 25 years ago Closed 24 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: fur, Assigned: law)

References

Details

(Keywords: perf, Whiteboard: [nsbeta2-][nsbeta3+][fixed, waiting on blocker])

Attachments

(2 files)

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
Assignee: fur → rpotts
Blocks: 14050
Rick, I'm not sure that you're the right person to give this to, but since you're Mr. Load Group...
Blocks: 14772
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.
> 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.
Depends on: 21216
I opened bug 21216 to report that the event.<x>Key properties are not set properly when clicking on a <titledbutton>.
Summary: Hook up reload/shift-reload buttons to load attributes → Hook up reload/shift-reload/back/forward buttons to load attributes
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.
Bulk move of all Cache (to be deleted component) bugs to new Networking: Cache component.
Blocks: 22176
Target Milestone: M14
Moving Rick's M14 bugs to M13 (since he won't be here for M14). He can triage them to M15 as appropriate.
Assignee: rpotts → gordon
Cache bug => Gordon.
Target Milestone: M13 → M14
Blocks: 20201
Status: NEW → ASSIGNED
*** Bug 25597 has been marked as a duplicate of this bug. ***
Seems non-essential for beta. Moving to M15.
Target Milestone: M14 → M15
> 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.
Pardon the spam, but how can you have a bug be non-beta M15, if it blocks a M14 4xp, beta1 bug (14772)?
Keywords: beta2
*** Bug 30852 has been marked as a duplicate of this bug. ***
> 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
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
Moving what's not done for M15 to M16.
Target Milestone: M15 → M16
*** Bug 34984 has been marked as a duplicate of this bug. ***
Whiteboard: 2d ?
Keywords: nsbeta2
reassign to me since I took 29343 which is related
Assignee: gordon → davidm
Status: ASSIGNED → NEW
Keywords: beta2
Attached patch Proposed fix (deleted) — Splinter Review
*** Bug 29343 has been marked as a duplicate of this bug. ***
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.
Assignee: davidm → travis
*** Bug 32469 has been marked as a duplicate of this bug. ***
the dupe of this bug 32469 had already received the nsbeta2+ blessing. DOes that just automatically carry over?
*** Bug 38049 has been marked as a duplicate of this bug. ***
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.
davidm: just do it, i say.
Blocks: 33503
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.
Assignee: travis → 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).
Status: NEW → ASSIGNED
Putting on [nsbeta2+] radar for beta2 fix.
Whiteboard: 2d ? → [nsbeta2+]2d ?
per pdt's request on status - Shift reload is still not working on today's build. Work is in progress.
*** Bug 39877 has been marked as a duplicate of this bug. ***
Move to M18.
Target Milestone: M16 → M18
No longer blocks: 22176
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.
Whiteboard: [nsbeta2+]2d ? → [nsbeta2-]2d ?
Hey, can you mark this fixed now?
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.
nav triage team: nsbeta3+ perf keyword IF we don't fix this, the performance of forward and back will seem quite slow.
Keywords: perf
Whiteboard: [nsbeta2-]2d ? → [nsbeta2-][nsbeta3+
*** Bug 46351 has been marked as a duplicate of this bug. ***
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.
Whiteboard: [nsbeta2-][nsbeta3+ → [nsbeta2-][nsbeta3+]
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.
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.
> 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.
> 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".
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.
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.
*** Bug 46735 has been marked as a duplicate of this bug. ***
Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so the queries don't get all screwed up.
Keywords: nsbeta3
Depends on: 48243
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.
Posted <news://news.mozilla.org/3992D331.851F8175@bucksch.org> to xpfe about changing the behaviour or naming of the button.
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [nsbeta2-][nsbeta3+] → [nsbeta2-][nsbeta3+][fixed, waiting on blocker]
Priority: P3 → P1
Blocker is fixed, making this one RESOLVED - FIXED.
Status: REOPENED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
verified: WinNT 2000091108 Linux rh6 2000091108 Mac os8.6 2000090820
Status: RESOLVED → VERIFIED
>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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: