Build: 07-26 Branch build. Steps to reproduce: 1. Launch browser, go to any page. 2. View | Page Source, then will bring up a Page Source window. 3. View | Character Coding, change the charset to something other than the current one. Result: The Page Source page doesn't get reloaded, and the charset still keep the same as the orginal one. Note: Auto-detect menu is working fine here - if you have a wrong charset in Page Source, you can correct it by going auto-detect menu.

Reassign to Shanjian and change the QA contact to myself.

The problem seems exist in /xpfe/browser/resources/content/navigator.js, function "BrowserReloadWithFlags()". The code there does not work in view source. Statement " var sh = getWebNavigation().sessionHistory;" cause a exception and following statements are not executed.

radha, can you explain why the code in http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator .js#485 does not work for view source?

Shajian: I do not know why it is failing for view-source with manually overridden charset changes. May be you are calling getWebNavigation on the chrome window or you are calling getSessionHistory() in a subframe that doesn't have session History. You may want to break up that call and see what is going on. In general only top level docshells with type, "content" have session history in them.

radha, I found out that the problem is caused by failing to get sessionHistory. It seems like sessionHistory does not exist for viewsource window. I am not familiar with the code, so my question might sounds stupid. Why can't we just get nsIWebNavigation interface directly from webNavigation? I assume there is a good reason not to do so. The following lines also seems fixing my problem: try { /* Need to get SessionHistory from docshell so that * reload on framed pages will work right. This * method should not be used for the context menu item "Reload frame". * "Reload frame" should directly call into docshell as it does right now */ var webNavigation = getWebNavigation(); var sh = webNavigation.sessionHistory; var webNav = sh.QueryInterface(Components.interfaces.nsIWebNavigation); webNav.reload(reloadFlags); } catch(ex) { var webNav = webNavigation.QueryInterface(Components.interfaces.nsIWebNavigation); webNav.reload(reloadFlags); } But I am afraid it is not a good idea to put normal code into exception handling. Can you suggest a way to test if sessionHistory exist or not? thanks!

radha, can you review my patch?

shajian, 1) I don't think, it is a good idea to put the new piece of code in the catch() segment of the existing code. I suggest that you add that piece of code in the caller with its own try{} catch() block, OR make a different function and call it 2) I think getWebNavigation() already returns a handle to nsIWebNavigation and there is no need to do a explicit QI to nsIWebNavigation on it again.

radha, I agree with your 2nd point. I tried it and it works fine. However, I am hesitating about your first suggestion. Function "BrowserReloadWithFlags" should do its work, it is unreasonable to me to pop up this inside logic to its caller. If there is a way to detect if "sessionHistory" exit in "webNavigation" or not before "var sh = webNavigation.sessionHistory;" is called, that should be a better choice. If not, I don't see anything wrong with doing a alternative approach in catch segment.

Here are my suggestions at this time: a) Please provide a updated patch (remove the additional QI for nsIWebnavigation on webNav) and see if someone in navigator group blakeross@telocity.com or jaggernaut@netscape.com or alecf@netscape.com will review the code that is in the catch block. OR b) Try something similar to the following, and if it works get reviews from the above mentioned people. If you go with the following, I would also like to try the patch and make sure that it doesn't break anything. function getSessionHistory() { try { return getWebNavigation().sessionHistory; } catch (e) { return null; } } BrowserReloadWithFlags(reloadFlags) { /* Whenever available, get sessionHistory and docshell and call * reload() on sessionHistory. This is required so that reload * on frameset pages will work right. However,this should not to be * done for "reload Frame" context menu. */ var sh = getSessionHistory(); if (sh) { try { var webNav = sh.QueryInterface(Components.interfaces.nsIWebNavigation); webNav.reload(reloadFlags); } catch (e) { } }else { // no session History. Call reload() directly on nsIWebNavigation var webNav = getWebNavigation(); if (webNav) { try { webNav.reload(reloadFlags) } catch(e) { } } // webnav } }

I like this idea. New patch is following.

blake, could you review my fix for this bug? Thx.

shanjian, 1) Please remove the dump statements 2) The name of the BrowserReloadWithFlags() is garbled as BrowsebbrReloadWithFlags(). I bet your patch will cause lots of breakage with this typo.

This is very embrassing. It is my carelessness to let those junk into my diff. I will post a new diff.

mark 0.9.4

Peter, could you review my patch? thanks.

Shanjian, a few comments on your patch: function BrowserReloadWithFlags(reloadFlags) { + /* Whenever available, get sessionHistory and docshell and call + * reload() on sessionHistory. This is required so that reload You are using 3-space indent here while (most of) the rest of the file uses 2-space indent. If you're uncertain which indentation mode to use, look at the first line for "tab-width": /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- ------ + var webNav = getWebNavigation(); + if (webNav) { + webNav.reload(reloadFlags) Here you have a tab before the if. One of our coding guidelines is not to use tabs, but instead expand them to spaces. Most editors can do this for you. Other than those nits, your code looks good, but let me attach another patch with a slightly different approach.

By the way, that try/catch around var sh = webNav.sessionHistory shouldn't be needed I think. Session History is optional, and in such cases a getter shouldn't throw an error if returns null. Let me talk to some people and file a bug on this if needed. In the mean time, what do you think of this patch?

I like your patch better than mine. Let's fix it that way. I need to seek sr now.

alec, can you sr this one?

Is alecf back from sabbatical yet? Blake could sr= this I think.

I found out that fact after an email is sent to alecf. I asked waterson to take a look.

So attachment 47463 [details] [diff] [review] could use a bit of explanation, because it sure looks squirrely. It looks like we try to get the ``session history'' object from the ``web navigation'' object, but this might fail sometimes (why?). When it does fail, we'll just go ahead and use the web navigation object (what are the consequences of doing this?). So, could somebody explain: - Under what circumstances ``getting'' the session history object (or QI'ing it to nsIWebNavigation) might fail? - When we fail to get the SH object, what are the consequences of ``just using'' the original web navigation object? And then update the patch with appropriate comments that would answer these questions for the next person that has to read this code?

Radha explained this in bug 68847. Radha, if I read you correctly there, what we should be aiming for is that we eliminate nsIWebNavigation from the nsDocShell implementation and call a bunch of methods, like goBack(), goForward() etc. directly on the nsIWebNavigation implemented by Session History? And as part of that, all normal reloads should also go through Session History (when available), except for per-frame reloads, which still would need to go through nsDocShell (for lack of a better/cleaner mechanism?) As for when Session History is and isn't available: a browser (window) will generally have support for session history, a view source window won't.

Yes. QI of session history to nsIWebNavigation will not fail. However, access to session history from a docshell will fail in a view-source window. That's why we first try to access SH, and call reload() off of it. This is done to provide proper behavior on frameset pages. If it fails then we call straight in to docshell. This is needed for cases like view-source. (this bug). Reload off of docshell is primarily used by layout when it detects a different charset in a page. It is identical to nsISHistory::Reload() on a simple page, but lacks functionality on frameset pages.

> Yes. QI of session history to nsIWebNavigation will not fail. Why do we need a |try| block then? > However, access to session history from a docshell will fail in > a view-source window. Will it hard-fail? I.e., return an error code? If not, let's remove the |try| block. > That's why we first try to access SH, and call reload() off of it. This is > done to provide proper behavior on frameset pages. Or any page, yes? > If it fails then we call straight in to docshell. This is needed for > cases like view-source. (this bug). Reload off of docshell is primarily used > by layout when it detects a different charset in a page. It is identical to > nsISHistory::Reload() on a simple page, but lacks functionality on frameset > pages. Okay, so why not add some comments to that effect in the patch. ``First, we'll try to use the session history object to reload so that framesets are handled properly. If we're in a special window (such as view-source) that has no session history, fall back on using the web navigation's reload method.'' Also, convince me you need to use this tortured try/catch logic instead of something simple like |if (!webnav.sessionHistory) {...}|, and I'll be happy.

jag, radha, thanks for providing the information. Jag, could you review my new patch (basically just some additional comment)?

waterson: getting the session history will give you a hard error if it's not there. Hence the try/catch, not around the QI but around the .sessionHistory, and to keep the code somewhat readable I opted for this grouping. Whether it should or shouldn't be a hard error is a different issue. I don't think it should be (as perfectly demonstrated by viewsource).

jag: ok, thanks. I still think the comment in attachment 47680 [details] [diff] [review] is pretty cryptic, but if you guys understand it, I'm okay with it. sr=waterson

reset milestone to 0.9.5.

I think I like waterson's comment better: ``First, we'll try to use the session history object to reload so that framesets are handled properly. If we're in a special window (such as view-source) that has no session history, fall back on using the web navigation's reload method.'' I believe the context menu item "Reload Frame" comment is better placed near that code, instead of here. r=jag with that change.

Now waterson's comment is a consensus, I have no objection either. I update my patch as such. A question to radha, can we hide the logic of session history inside docshell? I mean let docshell worry about either session history should be called or not, and we can just simply call reload on nsIWebNavigation.

Point out the obvious, why don't you ;-) I was wondering that myself, but I figured radha had good reasons not to.

fix checked in.

Verified on 10-05 trunk build, this feature is functional working, however the charset menu is not marked as the updated one. Opened a new bug 104474 for that, and mark this one as verified.