Closed Bug 598852 Opened 14 years ago Closed 14 years ago

Use SHistory's index to load current history entry from nsDocShell::Reload

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: zpao, Assigned: zpao)

Details

Attachments

(1 file, 1 obsolete file)

I figured the best way to fix bug 597757 is to make is possible to set the index on the browser's webnavigation without actually causing a load. That way reload will use that index.

I'm going to take a stab at it, but I might need to pull in the big guns since I don't know this code at all.

Bug 597757 is a b7 blocker, so that makes this a b7 blocker.
Ok, so it's already possible to do what I said above: (webNavigation.sessionHistory.getEntry(index, true) will modify the current index.

However, that doesn't actually fix the problem. webNavigation.reload doesn't ever look at sessionHistory. So I'm mutating this bug into what I actually need
Summary: Make it possible to set webnavigation's index without navigating → Use SHistory's index to load current history entry from nsDocShell::Reload
Attached patch Patch v0.1 (obsolete) — Splinter Review
This works. No idea if it breaks anything.
Attachment #478061 - Flags: feedback?(sdwilsh)
Comment on attachment 478061 [details] [diff] [review]
Patch v0.1

>+        // In case a reload happened before any real loads, but after session
>+        // history is set up with entries.
Pretty sure this is a sentence fragment.

>+        if (rootSH) {

>+            PRInt32 count;
>+            rv = rootSH->GetCount(&count);
Would be really handy to add comments here since the API methods are rather vague.  Comments in docshell are smiled upon, I can assure you.

>+            NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
Why not just return rv here? (two places)

>+            if (count) {
>+                PRInt32 index;
>+                rv = rootSH->GetIndex(&index);
>+                NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+
>+                nsCOMPtr<nsIWebNavigation> webnav(do_QueryInterface(rootSH));
>+                NS_ENSURE_TRUE(webnav, NS_ERROR_FAILURE);
>+                rv = webnav->GotoIndex(index);
Adding a comment explaining this would be handy.  I know what you are doing given the bug title, but just looking at the code makes me say to myself "wtf?"

You should address these, and then get r from bz.
Attachment #478061 - Flags: feedback?(sdwilsh) → feedback+
This is needed to fix a beta 7 blocker, so this needs to block beta 7 as well.

(In reply to comment #2)
> This works. No idea if it breaks anything.
FWIW, I don't think it does, but bz would be the one to ask.
blocking2.0: --- → beta7+
Attached patch Patch v0.2Splinter Review
Now with better comments & less NS_ERROR_FAILURE
Attachment #478061 - Attachment is obsolete: true
Attachment #478086 - Flags: review?(bzbarsky)
You confused me with smaug!  ;)
Comment on attachment 478086 [details] [diff] [review]
Patch v0.2

Olli, are you happy reviewing this?  If not, please bounce back to me and I'll try to recall how this stuff works.
Attachment #478086 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
Comment on attachment 478086 [details] [diff] [review]
Patch v0.2

I know I suggested this kind of approach, but...
The problem with this is that if we have a "transient" about:blank document
in the docshell, this would reload something wrong.

Seems like we don't need any changes to C++.
Just use nsISHistoryListener and cancel reload if we haven't loaded anything
to the  tab yet. You may need to use nsIDocShellHistory::getCurrentSHEntry
to check that there is no nsISHEntry.
When cancelling the reload, you can call gotoIndex in JS.

Note, nsISHistoryListener needs to support weakref, so based on MDC 
documentation QI should have nsISupportsWeakReference
https://developer.mozilla.org/en/Weak_reference

I hope the JS based solution doesn't affect too badly to performance.
Attachment #478086 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #8)
> Comment on attachment 478086 [details] [diff] [review]
> Patch v0.2
> 
> I know I suggested this kind of approach, but...
> The problem with this is that if we have a "transient" about:blank document
> in the docshell, this would reload something wrong.
>
> Seems like we don't need any changes to C++.

Granted, I don't know much about the intricacies of docshell, hell barely the basics, but...

Doesn't this sort of change make sense? I feel like the APIs are allowing us to get to this point, but then it doesn't do what you want it to do. This makes sense to me...

webnav.sessionHistory.purgeHistory(all);
webnav.sessionHistory.addEntry(entry1);
webnav.sessionHistory.addEntry(entry2);
webnav.sessionHistory.getEntryAtIndex(1, TRUE (modify index, yes please));
webnav.reload()

Could we say that if what SHistory thinks is the current entry != mOSHE nor lOSHE, then we load what's in SHistory and fall back to mCurrentURI? Would that fix the transient case? (I don't know anything about the transient about:blank except that it causes other issues in session restore)

> Just use nsISHistoryListener and cancel reload if we haven't loaded anything
> to the  tab yet. You may need to use nsIDocShellHistory::getCurrentSHEntry
> to check that there is no nsISHEntry.
> When cancelling the reload, you can call gotoIndex in JS.
> 
> Note, nsISHistoryListener needs to support weakref, so based on MDC 
> documentation QI should have nsISupportsWeakReference
> https://developer.mozilla.org/en/Weak_reference
> 
> I hope the JS based solution doesn't affect too badly to performance.

I have an idea that reuses the progress listener we have already have, so we'll see how that goes. (STATE_START && flag on browser == pre-restore reload)
Clearing blocking since this isn't the right way to do it.
No longer blocks: 597757
blocking2.0: beta7+ → ---
Based on the comment #8, it seems like this isn't wanted at all, so WONTFIXing (in an attempt to prevent bugs piling up).
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: