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)
Core
Networking: Cache
Tracking
()
VERIFIED
FIXED
M18
People
(Reporter: fur, Assigned: law)
References
Details
(Keywords: perf, Whiteboard: [nsbeta2-][nsbeta3+][fixed, waiting on blocker])
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•25 years ago
|
||
Rick, I'm not sure that you're the right person to give this to, but since
you're Mr. Load Group...
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.
Reporter | ||
Comment 3•25 years ago
|
||
> 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.
I opened bug 21216 to report that the event.<x>Key properties are not set
properly when clicking on a <titledbutton>.
Reporter | ||
Updated•25 years ago
|
Summary: Hook up reload/shift-reload buttons to load attributes → Hook up reload/shift-reload/back/forward buttons to load attributes
Reporter | ||
Comment 5•25 years ago
|
||
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.
Updated•25 years ago
|
Target Milestone: M14
Comment 7•25 years ago
|
||
Moving Rick's M14 bugs to M13 (since he won't be here for M14). He can triage
them to M15 as appropriate.
Updated•25 years ago
|
Assignee: rpotts → gordon
Comment 8•25 years ago
|
||
Cache bug => Gordon.
Updated•25 years ago
|
Target Milestone: M13 → M14
Comment 11•25 years ago
|
||
> 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•25 years ago
|
||
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•25 years ago
|
||
*** Bug 30852 has been marked as a duplicate of this bug. ***
Comment 14•25 years ago
|
||
> 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•25 years ago
|
||
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 17•25 years ago
|
||
*** Bug 34984 has been marked as a duplicate of this bug. ***
Comment 18•25 years ago
|
||
reassign to me since I took 29343 which is related
Comment 19•25 years ago
|
||
Comment 20•25 years ago
|
||
*** Bug 29343 has been marked as a duplicate of this bug. ***
Comment 21•25 years ago
|
||
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
Comment 22•25 years ago
|
||
*** Bug 32469 has been marked as a duplicate of this bug. ***
Comment 23•25 years ago
|
||
the dupe of this bug 32469 had already received the nsbeta2+ blessing. DOes that just automatically carry over?
Comment 24•25 years ago
|
||
*** Bug 38049 has been marked as a duplicate of this bug. ***
Comment 25•25 years ago
|
||
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•25 years ago
|
||
davidm: just do it, i say.
Comment 27•25 years ago
|
||
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
Assignee | ||
Comment 28•25 years ago
|
||
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
Comment 29•25 years ago
|
||
Putting on [nsbeta2+] radar for beta2 fix.
Whiteboard: 2d ? → [nsbeta2+]2d ?
Comment 30•25 years ago
|
||
per pdt's request on status - Shift reload is still not working on today's
build. Work is in progress.
Comment 31•25 years ago
|
||
*** Bug 39877 has been marked as a duplicate of this bug. ***
Comment 33•24 years ago
|
||
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 ?
Comment 34•24 years ago
|
||
Hey, can you mark this fixed now?
Comment 35•24 years ago
|
||
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•24 years ago
|
||
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+
Comment 37•24 years ago
|
||
*** Bug 46351 has been marked as a duplicate of this bug. ***
Comment 38•24 years ago
|
||
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.
Updated•24 years ago
|
Whiteboard: [nsbeta2-][nsbeta3+ → [nsbeta2-][nsbeta3+]
Comment 39•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
> 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•24 years ago
|
||
> 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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
*** Bug 46735 has been marked as a duplicate of this bug. ***
Comment 46•24 years ago
|
||
Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so
the queries don't get all screwed up.
Keywords: nsbeta3
Assignee | ||
Comment 47•24 years ago
|
||
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.
Assignee | ||
Comment 48•24 years ago
|
||
Comment 49•24 years ago
|
||
Posted <news://news.mozilla.org/3992D331.851F8175@bucksch.org> to xpfe about
changing the behaviour or naming of the button.
Assignee | ||
Comment 50•24 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 51•24 years ago
|
||
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]
Assignee | ||
Comment 52•24 years ago
|
||
Blocker is fixed, making this one RESOLVED - FIXED.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 53•24 years ago
|
||
verified:
WinNT 2000091108
Linux rh6 2000091108
Mac os8.6 2000090820
Status: RESOLVED → VERIFIED
Comment 54•22 years ago
|
||
>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.
Description
•