Closed Bug 41973 Opened 24 years ago Closed 24 years ago

Redirected links are not marked Visited

Categories

(Core :: DOM: Navigation, defect, P4)

defect

Tracking

()

VERIFIED FIXED
mozilla0.8

People

(Reporter: pierre, Assigned: radha)

References

()

Details

(Keywords: embed, top100, Whiteboard: [nsbeta2-][nsbeta3-][PDTP4])

Attachments

(1 file)

- Go to Yahoo - Click almost any link - Go back ==> The link is not marked as visited I guess it is related to the fact that these links are redirected. For instance, if you click "Business & Economy", it goes to http://www.yahoo.com/r/bu which is redirected to http://dir.yahoo.com/Business_and_Economy/
Marking beta2/top100
Keywords: nsbeta2, top100
waterson, Do you have any clue on this one? Thanks,
it's probably not being added to the global history. check the flow of control in the docshell stuff.
Putting on [nsbeta2-] radar. Not critical to beta2. Adding nsbeta3 keyword.
Keywords: nsbeta3
Whiteboard: [nsbeta2-]
Moving some of my comments from bug 12493... ---- Bug 41973 comes from the assumption that a page that causes a redirection isn't added to the History before causing the redirection. But as I'm writing this, I'm realizing that Marc and I could have been wrong because the URLs on Yahoo are not canonized either. We need to write a testcase with canonized URLs that cause a redirection. If it works, then bug 41973 is in fact a dup of bug 12493.
nav triage team: if this also effects Back / Fwd, we will definitely nsbeta3+ it. By the way, this effects almost all portals like Yahoo, Netscape.com, etc.
Whiteboard: [nsbeta2-] → [nsbeta2-][NEED INFO]
Depends on: 42236
nav triage team: Claudius says it does not effect Back / Forward button - doesn't appear to. However, this does mean that links on about every portal will nto change colors to show that you have visited the page. That is bad, marking nsbeta3+ (borderline, so if it is hard to fix, we might revisit this).
Whiteboard: [nsbeta2-][NEED INFO] → [nsbeta2-][nsbeta3+]
The fix for it needs change in docshell. Not in Session History. Changing component.
Assignee: radha → valeski
Component: History → Embedding: Docshell
Radha, do we change the color of links based on history entries? If so, I don't think the docshell will ever know about a redirect as they happen transparently (right gagan?) in necko. This would imply that http would need to set the history :-/.
visited link color change is done by global history owned by rjc. But I think the code that lets global history know about such links exists in docshell. There is a inherent problem with redirected links though. Redirected links are passed to docshell with loadtype "loadNormal" which gives rise to a bunch of sesion History related bugs. Gagan has a bug on this. May be this problem is related to the bug that gagan has.
Target Milestone: --- → M18
I don't see how the loadNormal bug is related to this bug. The essential problem as I see it is that history is not updated for visited links (thru redirects) and the main reason is becuz webshell/docsheel needs to update that just as they would update the location bar on a redirect. cc'ing mscott.
Okay so on a whim I implemented nsIHTTPEventSink tonight 'cause we seem to have a set of bugs that we think would be fixed by such an implementation. Looking at this bug after implementing http event sink, I'm not sure how that's supposed to fix this. The docshell gets told of the location change. It looks like the new url is added correctly to history. The problem is really that we don't add urls to history until after we load them. i.e. until after the redirection happens....so we only end up adding the final url.
so who's impl'ing nsIHTTPEnventSink? I suspect whoever's doing that should add the original url (the one that caused the redirect) to history when the HTTPEventSink gets redirect notification (maybe this is in the form of checking the HTTP status code?). Scott, from your comments I can't tell if this is already happening or not?
*** Bug 44560 has been marked as a duplicate of this bug. ***
Priority: P3 → P1
If this is truly only a lack of visual indication, it can wait for the next release cycle.
Priority: P1 → P4
Whiteboard: [nsbeta2-][nsbeta3+] → [nsbeta2-][nsbeta3+][PDTP4]
Not holding PR3 for this; marking nsbeta3-.
Whiteboard: [nsbeta2-][nsbeta3+][PDTP4] → [nsbeta2-][nsbeta3-][PDTP4]
Keywords: embed
*** Bug 59304 has been marked as a duplicate of this bug. ***
*** Bug 58642 has been marked as a duplicate of this bug. ***
*** Bug 60211 has been marked as a duplicate of this bug. ***
Target Milestone: M18 → mozilla0.8
Blocks: 64833
Looking thro' the comments, I see that the problem described here occurs for all server side redirects. Necko handles the redirection and docshell has absolutely no info on it. So the original url never gets added to global history, which means no color change for the original url. nsDocLoader implements nsIHTTPEventSink and it sends out a nsIWebProgressListener::onStateChange() notification when there is a redirect on the main document channel. DocShell implements nsIWebProgressListener. Little debugging should provide info on whether this notification comes on time to update the history. Re-assigning to self.
Assignee: valeski → radha
*** Bug 61223 has been marked as a duplicate of this bug. ***
Attached patch patch to docshell (deleted) — Splinter Review
(r=rpotts) hey Radha, This patch looks fine... You're correct that when the document URI gets redirected, OnStateChange(...) is called with STATE_REDIRECTING | STATE_IS_DOCUMENT flags set... -- rick
sr=waterson; looks good to me.
Rick's right, you are using the correct status flags. sr=mscott
Not raising any flag or demanding any change, but trying to be consistent: I recently persuaded/nagged adam lock into optimising (flags & F1) && (flags & F2) into (~flags & (F1|F2)) == 0. Same applies here: + else if ((aStateFlags & STATE_IS_DOCUMENT) && (aStateFlags & STATE_REDIRECTING)) { could be, maybe should be: + else if ((~aStateFlags & (STATE_IS_DOCUMENT | STATE_REDIRECTING)) == 0) { saves a branch and second mask test, compares to zero for faster test on many architectures. jst used this pattern recently, too. /be
This was fixed last week
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
VERIFIED Fixed 20010207 builds
Status: RESOLVED → VERIFIED
No longer blocks: 64833
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: