Closed Bug 78510 Opened 23 years ago Closed 19 years ago

Link should become :visited color if URL is loaded in another window/tab/frame

Categories

(Core Graveyard :: History: Global, defect, P3)

Product:
Core Graveyard
Component:
History: Global
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: caseyperkins, Assigned: roc)

References

Details

(Keywords: fixed1.8, Whiteboard: parity-opera [has patch, has review, needs superreview (bzbarsky)])

Attachments

(2 files, 5 obsolete files)

updated patch #6
69.66 KB, patch
dbaron
: review+
dbaron
: superreview+
jesup
: approval1.8b4+
Details | Diff | Splinter Review
don't leak all of the pres shells
804 bytes, patch
jst
: review+
jst
: review+
Details | Diff | Splinter Review 
This bug is related to 10491 ("Show link as :visited if opened in another
window"), but encompasses more. I was told to file this as a separate bug when I
brought up this issue commenting on another bug.
Basically, Mozilla does not show a link as visited until the page is reloaded.
(This is especially burdensome within framesets.). So the user winds up confused
about which links have visited, though that should be immediately obvious upon a
quick glance at the page.
Opera has the ideal behavior regarding visited links:
1) Clicking a link in a frameset immediately changes that link to visited color.
2) Opening a link in a new window changes the link in the original window to the
visited color.
3) Clicking a link in one window informs all other browser windows containing
the same link that the link should be rendered as visited, which it then is.

This is the best scheme (and the least confusing) for rendering visited links.
It seems to me that making Moz show the link as visited before a reload
shouldn't be too difficult. I'm not acquanted with the intricacies of Mozilla's
source code, but since Mozilla can already change the color of links
onMouseOver, onClick, and on other events, it seems like it would be an easy
matter to apply the same mechanism to do that for links when they are visited -
one would just have to create some new event that recognizes when a link is
visited, then activates the common function.

I would be very surprised if the code to fix bug 10491 didn't also fix this 
bug.
Hopefully you're right about fixing 10491 fixing this one two. But will it fix
notifying the other windows as well (see number 3 in my original description)?
That seems more difficult.
It seems to me that fixing this bug would require either keeping a hash table 
of every link in every window around (substantially increased memory use), or 
checking every link in every window each time a new URL is visited in any 
window (more cpu usage and a likely speed decrease).  Is that worth it when 
fixing bug 10491 would cover the 95% case?

Confirming as a minor bug and sending to History: Global.
Assignee: asa → alecf
Severity: normal → minor
Status: UNCONFIRMED → NEW
Component: Browser-General → History: Global
Ever confirmed: true
QA Contact: doronr → claudius
Whiteboard: parity-opera
Jesse,
I believe it is worth it, because it helps remove all confusion about which
links have been visited.
As for the cost of fixing this bug (in memory or CPU cycles), I would argue
since Opera works in this manner, and is a very fast and light browser, that
there must be a way of doing it in a way that is not overly resource intensive.
As for what way that might be, I leave it in your capable hands, although I'd
say as a fellow programmer that the hash table solution doesn't sound optimal.
it's not resource intensive if done correctly, via some kind of observer
system.. but frankly I don't see the immediate need.
Status: NEW → ASSIGNED
Keywords: helpwanted
Priority: -- → P3
Target Milestone: --- → Future
While it would be nice to update every link in every window, I think that
emulating IE's behaviour in this situation would make most people happy (and it
is an easier fix).  This allows one to right-click -> open-new-window and visit
a foriegn link, and after closing the window the correct 'active' link is still
highligted so that you can continue on to the next link.  I use this method all
the time with any browser, and IE seems to handle it very well.  Plus, as far as
Mozilla is concerned, the 'active' link *should* be the one opened up in the new
window; leaving it blue is misleading.
The "active link" solution is really no solution at all. The reason I open links
in new windows is so that I can use the current window for something else. Thus,
the link doesn't stay active.
Who cares if it's an **easier** fix? It's not a **better** fix. Instead of
trying to match IE, let's exceed it, and make a better browser. In any event,
I'm willing to wait for the harder fix. I'm sorry I can't code it myself; my C++
skills are unfortunately not as good as my Java skills.
Another case which I believe this bug pertains to is links being colored as
visited after hitting the back button to go back to their page.  Right now, if I
click on a link, go to that page, and then hit the back button, the original
page is reloaded and the link I was just at isn't colored as visited.  Hitting
reload updates the visited links colors.  Mozilla should take care of this
automatically.
*** Bug 96987 has been marked as a duplicate of this bug. ***
argh.. take two at reassigning history bugs to new owner
Assignee: alecf → blakeross
Status: ASSIGNED → NEW
Target Milestone: Future → ---
*** Bug 101066 has been marked as a duplicate of this bug. ***
I think this bug should be resummarized as "Link should become :visited color if
URL is loaded in any other window" in order to avoid confusion with bug 10491.
Summary: visited link colors should **always** show if link is visited → Link should become :visited color if URL is loaded in any other window
Whiteboard: parity-opera → [Aufbau-P4] parity-opera
Whiteboard: [Aufbau-P4] parity-opera → parity-opera
*** Bug 10491 has been marked as a duplicate of this bug. ***
Sorry, I am usually very opposed to spamming bugs with personal opinions: But
the dupe bug had 33 votes, this bug has 15 votes now, which makes it one of the
most requested features out there. Even if Alecf doesn't see the need for this
bug to be fixed, it is IMHO not a minor thing: it really prevents me to e.g.
view buglists in a manner that allows me to see which bugs I already opened in
new windows. I hope Blake has its priorities somewhat different and I am willing
to discuss the importance of this topic more detailed in the newsgroups. Please
don't minor and/or future this one which has been filed since August 1999 (as
bug 10491)...
*** Bug 108408 has been marked as a duplicate of this bug. ***
This bug can be fixed without incurring too much memory and CPU usage. Mozilla
could run a chunk of code each time a different window is brought into the
focus. The code would check the history of URI's visited today. The check would
be whether Mozilla visited any URI after loading the page now coming into focus.
If it has, then Mozilla would run the code. The code would check each "a href"
link on the page now in focus. The check would be against the history of URI's
visited today, and would be limited to the URI's visited after the page now in
focus was loaded. A good sorting algorithm would help at this point. (Maybe only
URI's with particular subdomains need to be checked.) If there are any matches,
change those "a href" links to the visited color.

Ideally, this chunk of code would run in its own thread. That way, the window
could be brought into focus immediately. The user might see a few links change
to the visited color while he is looking at the page. Apologies in advance if
this is spam.
*** Bug 23465 has been marked as a duplicate of this bug. ***
*** Bug 114764 has been marked as a duplicate of this bug. ***
spreading the love.
Assignee: blakeross → alecf
*sigh*
This really isn't as easy as all that. First of all, the history is not stored
in a method that provides easy access to "all links visited today" and doing so
would add bloat that I'm not willing to incur. Its also very hacky. I'm not
going to fix this the wrong way just to make 41 people happy.

I'm telling you the ideal solution is to have a listener which gets notified
whenever a url is loaded. The best place to put this listener is probably on a
prescontext or someother per-document structure..then the listener would go
reflow the links. I'm not going to get to this any time soon however, so unless
someone has a patch to present for review, please do not reset the target milestone.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1
*** Bug 116078 has been marked as a duplicate of this bug. ***
*** Bug 117135 has been marked as a duplicate of this bug. ***
*** Bug 118541 has been marked as a duplicate of this bug. ***
*** Bug 118892 has been marked as a duplicate of this bug. ***
*** Bug 119607 has been marked as a duplicate of this bug. ***
*** Bug 119752 has been marked as a duplicate of this bug. ***
*** Bug 123590 has been marked as a duplicate of this bug. ***
On parity: Navigator 3.x on Solaris, recolored a middle-clicked link (the 95%
Open in New Window case). Communicator 4.77 on Solaris recolors any link in a
page with a style-sheet (ex: product forums at www.adobeforums.com) after the
link loads elsewhere; full opera-parity I believe, excepting the style-sheet
dependency.  Without a style-sheet a middle-clicked link temporarily highlights
(redrawn 1st in alternate color, 2nd back to original).

Hence Comm 4.77 is frustratingly close:  If the user could have configured a
preference forcing a user CSS on every page, they'd get opera-parity full-time.
Or if the 2nd redraw to restore the original color could have been disabled by
preference (perform less work), it would achieve the coarse 95% case.

Mozilla 0.9.7 on Solaris, persistently highlights a link just dragged to a new
window.  Moreover the *previously* highlighted link is restored (back to
never-visited).

So modify link-drag via a preference: When highlighting a dragged-link, just
don't unhighlight the previously dragged-link (perform less work), or (best)
rehighlight to visited-link instead of not-visited, or rehighlight in some
alternate recently-dragged color (same work).  Now apply this enhanced
dragged-link highlighting to middle-clicked links as well via a preference.

Does that deliver the low-hanging fruit, the 95% case, at very much less cost
than notify-on-load opera-parity; hence Mozilla is also fustratingly close?
Dragged-link highlight is reverse-video-like in the active window else greyed,
probably a link-area by-pixel color remap, not proper text rerendering or
restyling?  Ugly to litter the page with area highlights; but ok for a user
enable-able stop-gap (the 95% case).

Immediate redraw as visited is rightly criticized as misleading since the load
hasn't completed yet. Doing nothing immediately and never indicating a load
failure also symmetrically misleads, but worse since the link is
indistinguishable from the vast herds of never-tried links.  Reading here, opera
still doesn't address *both* of these, suggesting a full proper solution
surpassing opera even?: Link color state model should include not-visited,
recently attempted (immediate blind change on a drag or middle-click, precisely
so you can choose to *retry* it or not), and visited (load confirmed somewhere).

That degenerates nicely to all desired models: With just the 95% case stop-gap,
it delivers at least immediate attempted-link feedback; and reload converts the
successes to visited. The full solution automatically covers even the Comm 4.77
temporary highlight: as attempted-link becomes an intermediate updated to
visited-link precisely if and when load completes, whether after a normal delay
beyond a first try or some later retry.
What about this simple approach:

 1. user activates the link, and it immediately goes into :active state (what a
surprise)
 2. the link stays :active, while other processing occurs
 3. after everything is settled, the link becomes :visited (from :active)

This allows the user to see some already visited (:visited), some pending
(:active), and the other normal links in a page, and even see which one has
finished loading without switching windows/tabs.

See Bug 102479 and Bug 65213 if interested, and maybe add them to the dependency
tree.
This analysis is really unnecessary. The problem is understood, the solution is
known, the resources simply aren't there - I don't have time for this right now,
there are bigger fish to fry.

The solution which would take the LEAST amount of time is also the correct
solution, and that is to have the observer system that tweaks the CSS state of
visited links in loaded documents.

Please stop filling bugs with WHY we should fix this, and just let it get fixed
(or fix it yourself)
*** Bug 127772 has been marked as a duplicate of this bug. ***
I don't know why Opera is always used as referene for this bug? Mozilla doesn't
behave like Opera, but it also doesn't behave like IE. It doesn't even behave
like Netscape 4.x, in other words, it doesn't behave like any other popular
browser there is.

What is most annoying is not that links aren't recolored if a page is loaded in
any window, they are also not recolored if it's loaded within the /same/ window!
E.g. in the left frame is a thread list and in the right frame are the posts.
You will never know which posts you already read and which ones not, because the
color of thread-links doesn't change unless you manually reload the frame. In
every other browser, the link in the left frame will change in color sooner or
later.

If I click on a link to get to a sub-page and later on hit the back button, the
link will be colored differently, how comes? How does Mozilla know that I've
already been on that page? People here say it will slow down page loading, so
right now it does not slow down page loading? How comes? Right now Mozilla must
check links as well if you load a page, so how about just "enlarging" this check
to also include frames that have not changed by clicking on the link?

This will not solve the problem as a whole, but it will at least color links
within navigation frames.

*** Bug 135370 has been marked as a duplicate of this bug. ***
*** Bug 138691 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
*** Bug 140710 has been marked as a duplicate of this bug. ***
*** Bug 144459 has been marked as a duplicate of this bug. ***
Since it seems that this bug isn't going to get real fix any time soon how about
a quick workaround instead; when any window changes its location force all open
windows to reflow links. Because visited link coloring seems to work pretty much
correctly when I press reload, Mozilla should be able to do that automatically
-- just make sure that no data is lost because of this reflow. Yeah, it's going
to hit performance a little but how fast can user click the links anyway? It's
only a one reflow per window per link click.

*** Bug 149113 has been marked as a duplicate of this bug. ***
*** Bug 151753 has been marked as a duplicate of this bug. ***
*** Bug 155487 has been marked as a duplicate of this bug. ***
*** Bug 156035 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.2alpha → Future
*** Bug 158853 has been marked as a duplicate of this bug. ***
Keywords: mozilla1.1
OS: other → All
Hardware: PC → All
Err, _why_ did we add the mozilla1.1 keyword to this bug?  It's
Future _and_ helpwanted, so it's clearly not going to happen all
that soon.  It doesn't have even a preliminary working patch,
and 1.1 is into the driver-approved-checkins-only freeze period.
There is basically no possibility this can make 1.1  

Please, only add keywords that make a modicum of sense.
Keywords: mozilla1.1
Is there any way of using Sax to fix this? 
who/what/why is Sax?
uhm, no. SAX is irrelevant here. the best solution here is quite gecko-specific
Blocks: majorbugs
With 81 votes, I don't think this bug qualifies as minor.
I was about to file a bug report for this when I came across this bug.
Web browsing frequently involves visiting pages with a list of links. And
knowing which pages you have visited and which you haven't in that list is
crucially important (without having to reload) to the web-surfer. 

Please change severity to major.
With 81 votes, I don't think this bug qualifies as minor.
I was about to file a bug report for this when I came across this bug.
Web browsing frequently involves visiting pages with a list of links. And
knowing which pages you have visited and which you haven't in that list is
crucially important (without having to reload) to the web-surfer. 

Please change severity to major.
Nonsense.

 "Minor - minor loss of function, or other problem where easy workaround is present"

This is exactly what this bug is. Would be nice to fix, but it is NOT a
major bug. In particular, there is no "major loss of function".

Besides, it won't make any difference. If you want it fixed, pay
a developer or do it yourself. Or wait.
Hmm.. now that I've switched to Phoenix, and have been using a cvs snapshot, URL
highliting appears to work as expected.  Does this problem still remain when
loading urls within framesets?

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021026 Phoenix/0.3
Keywords: mozilla1.3, nsbeta1
It's certainly minor in its severity, but it's something a lot of people care 
about.  So really the overall impact of a Mozilla bug (and how high a priority 
it should be given) is probably best judged from a combination of the votes 
and the actual severity.  (Perhaps an additional field -- User Impact or 
something like that, that estimates how often users are inconvenienced -- is 
called for.)

>Besides, it won't make any difference. If you want it fixed, pay
a developer or do it yourself. Or wait.

Any of you Netscape folks want to set up a Paypal account or similar and 
solicit contributions for doing a little overtime?  I'd throw in a five-spot...
This should be a no-brainer now, since this feature is already working in
Phoenix.  I would guess someone just needs to port the code back into the main
Mozilla tree.
No, the Phoenix behaviour is incomplete. Visited links are still not marked
visited in all windows/tabs, and this is what the summary of this bug asks for.
ovk: That's not what the summary says.

>>  Link should become :visited color if URL is loaded in any other window

Seems like there are two steps for this bug.

1) When I open a link to document 'D' from a document in window 'W', into a new
tab/window, the link in window W should immediately turn visited.

2) links to document D in other windows should be marked visited.

Seems like phoenix implements 1) but not 2).  I appreciate that Alec wants
to do 2) as well, but I would think that a good many people would be satisfied
with 1), and it would seem to me to be much more easy to implement just 1).

Steve: I'm aware of the two possibilities you mentioned (that's why I said
"incomplete") and that the summary could also be understand as "link should get
:visited color in window A when clicked in window A to load in window B", and I
agree with you and the others that the current implementation in Phoenix is
better than nothing. But reading the bug description (especially point 3 about
Opera's scheme of handling this) still makes me think that this bug clearly
calls for more and this aim shouldn't be lost.
well, there is a lot of talk here but no action. Sure it would be easy for
someone to "port the code back into the main Mozilla tree" - don't tell me
everyone on the CC of this e-mail is so unfamiliar with the mozilla code that
they can't just do this themselves. C'mon folks, this is open-source. Contribute
something, or stop complaining!
*** Bug 182647 has been marked as a duplicate of this bug. ***
*** Bug 182709 has been marked as a duplicate of this bug. ***
  I am using 1.3.0.20021123 on w2k, sp3.  Hmmm.  Right clicking on the file and 
left clicking on "Save Link Target As" does change the link from blue to red.  It 
is the (normal) left click and save that does not change colors.
Re: Comment#29.  Would it not also make sense to provide a visual indication of
known-broken links; i.e. those the browser has tried to open and incurred an error?

General: Should the user have some control over how far back into the past the
"visited" state persists?  Frankly, I rarely care whether I visited some page
three months ago: by now it's new to me anyway.  Or, the "visited" state would
need to check expiration of the target.  A link that I visited yesterday that
has not changed is less interesting than one I visited last week but has since
been updated.

Apologies if this is just noise.  I've been off in another world (called work)
lately.
visual indication of known-broken links:
I would not implement this as long as there is no CSS standard for such a
feature. It (especially its colour) would not be configurable or worse
non-standard from a webmasters point of view.

"visited" state persistence:
It would come natural to mark something as 'visited' as long as it is still in
the browser history. I guess this is the current behaviour, too.
> It would come natural to mark something as 'visited' as long as it is 
> still in the browser history. I guess this is the current behaviour, too.

That is the current behavior largely because it is what NCSA did in 1994,
and nobody has implemented anything better.  If something better can be 
done, that might be a good thing (provided it is indeed better; a pref can
be used if there is no consensus which behavior is better).  However, that
would be a different bug; this bug is about when a page becomes visited in
the first place; when it becomes no longer (recently) visited is really a
separate question.  One issue, one bug.

Regarding broken links:  interesting, but definitely not related to this bug.
Discuss in a newsgroup (n.p.m.ui seems right) and maybe file separately.
regarding broken links: that's already filed - bug 153765
Re: Comment#63
> "visited" state persistence:
> It would come natural to mark something as 'visited' as long as it is still in
> the browser history. I guess this is the current behaviour, too.

Filed "RFE: User Control of History Aging"

http://bugzilla.mozilla.org/show_bug.cgi?id=184282
*** Bug 184320 has been marked as a duplicate of this bug. ***
*** Bug 128864 has been marked as a duplicate of this bug. ***
I guess this bug to be important to me.

Just my 2c: It seems to me that link should be marked as visited not instant
after clicking, but only after target window/tab URL will be fully loaded or
stopped by user. If target URL couldn't be loaded (e.g. because of fortuitous
disconnection), link shouldn't have a "visited" look.
*** Bug 198078 has been marked as a duplicate of this bug. ***
As for Comment #69 This is definitely an issue.  Right now, with moz 1.3, if I
right click a link, it _is_ immediately marked as visited, even if I never
actually click anything inside the context menu.  I actually feel this is a
separate bug, but have been unable to find one already existing for it, so far
all roads lead back to this bug.  Does anyone know of the bug for that?
that's a totally seperate issue, and its not even a bug - its an intended
feature - when you right click on a link the link lights up to let you know that
you're about to do something with it - if you click away the link is no longer
lit up.

in any case, all of that is a seperate bug, just file it. worst comes to worst
someone else tracks it down and it gets marked as a dupe - better than
cluttering up other bugs with unrelated behavior questions.
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-
Hey, I just wanted to add that at least in firebird 0.6 for windows, it seems
like there's still this bug when you click links that the _webmaster_ has set to
open up in a new window (as opposed to ctrl-clicking or
middle-mouse-button-clicking).

That is, the link doesn't change to the visited color in the original window
when you click it if it opens a new window. Try http://www.newsfromme.com/ for
example.  Later, if you reload the page, the links show up in the visited color,
but not immediately upon clicking!
*** Bug 211118 has been marked as a duplicate of this bug. ***
*** Bug 219804 has been marked as a duplicate of this bug. ***
Flags: blocking1.6a?
Keywords: mozilla1.3
*** Bug 222805 has been marked as a duplicate of this bug. ***
Flags: blocking1.7a?
Flags: blocking1.7a? → blocking1.7a-
*** Bug 236050 has been marked as a duplicate of this bug. ***
QA Contact: claudius → ian
If anybody thinks this bug is not that important: I have signed up at bugzilla
just to take care of this bug.

Because this bug affects my number one web site severely. It's a discussion
board I usually visit several times a day. The software used is a new kind of
HTML based forum; unlike the dominating UBB and similar software, which are not
much more than fudged guestbooks, this new system is absolutely capable to
conduct _real_ discussions. For this, the essence of the board system is the 3
frames layout: The Screen is divided horizontally with the first frame on the
top containing the topics, the second frame in the middle containing the thread
with sub(subsubsub...)threads and the third frame containing the message. And
_there_ the bug filed here does major damage to the experience in reading and
contributing to this board, when using the Mozilla family (Mozilla 1.4/1.6,
Firefox 0.8 and older versions):

To use the forum, you click on one of the topics in the upper frame. Then the
thread of the selected topic is loaded in the middle frame and the starting
message/text in the lower frame. If you want to read answers to this topic, you
click on the answer you wish to read in the middle frame and the the text of it
will be loaded in the lower frame and so on with every post in the thread. Now
with this bug in the Mozilla family, unlike Internet Explorer and the newest
version of Opera, the already read answers are not marked immediately after
clicking them by changing the color to a visited link; so especially in threads
with lots of answers, subthreads and subsubthreads an so on, you loose the
overview about what you've read and what not. You have to reload the upper and
middle frame to get the visited links marked.

And as this kind of forum is slowly, constantly expanding on the Internet, the
Mozilla family will loose users, because only the Internet Explorer an Opera are
able to handle it correctly.

Direct link into the board I'm talking about:
http://www.pcx-forum.de/pcx/pxmboard.php?mode=board&brdid=1

The programmer of the forum software has a list on his page of the pages using
his software, all affected the same way with this bug:
http://www.torsten-rentsch.de/pxm.html
Go fix this bug! ADOM!!!1! nt
re: Comment #79

So, you're volunteering to fix it, right?
Assignee: alecf → nobody
Status: ASSIGNED → NEW
QA Contact: ian → core.history.global
one more re: Comment #79
Current Firefox builds have this bug fixed. When I open the link in new window
or tab, the original link changes color. Someone pls correct me if I don't
understand this bug (since I am more interested in Firefox, I want to remove
myself out of this cc list).
Re Comment #82 see Comment #55

I apologise for adding noise to this bug. It's as if everything that could
possibly be said has already been said and we are doomed to only have the same
things repeated indefinitely as new people add their "me toos", "why not do
blah" and "fix this" comments.
Sitsofe, thanks for pointing me the comment #55. Now I understand that the
remaining issue of of this bug is the requirement no.3 : "Clicking a link in one
window informs all other browser windows containing the same link that the link
should be rendered as visited, which it then is".
FYI, IE (6.0.2800.1106) on my Win2K machine does less than what Firefox can do.
So I can understand the reduced severity for a Firefox user.
Frames are used in a number of online forums that I visit daily.  This issue
makes it difficult to see which threads and/or messages have been read and which
have not.  So, this bug fix would be high priority for me.

If you do a shift-click to open the link in a new instance of Firefox, the color
of the link on the current page immediately changes as the new firefox instance
renders the new page.  It would seem the code that works for that behavior could
be leveraged to work in a frame situation.  What would keep this from solving
the issue?
Summary: Link should become :visited color if URL is loaded in any other window → Link should become :visited color if URL is loaded in any another window/tab
Summary: Link should become :visited color if URL is loaded in any another window/tab → Link should become :visited if URL is loaded in any another window/tab
Summary: Link should become :visited if URL is loaded in any another window/tab → Link should become :visited if URL is loaded in another window/tab
Re-adding color to summary: Easier to search for this bug.
Summary: Link should become :visited if URL is loaded in another window/tab → Link should become :visited color if URL is loaded in another window/tab
Can you now please stop changing the summary all the time? Thank you very much.
(In reply to comment #87)
> Can you now please stop changing the summary all the time? Thank you very much.

It is still missing something though.
Now:
"Link should become :visited color if URL is loaded in another window/tab"
Should be:
"Link should become :visited color if URL is loaded in another window/tab/frame"

The weird thing is, the bug (frame related atleast) is there in MS-Windows
Firefox v1.0, but not in Linux Firefox v1.0. I believe they use the same
Mozilla-code.

This bug really bothers many MS Windows Firefox users. Many would have stayed
with Firefox also after the Iframe bug was corrected in IE (2004/12/03), but now
switched back to IE.

Using my powers to make the change requested in comment #88. Please take note
that it is the first time I change this over-abused summary.
Summary: Link should become :visited color if URL is loaded in another window/tab → Link should become :visited color if URL is loaded in another window/tab/frame
*** Bug 250515 has been marked as a duplicate of this bug. ***
This bug also stops the "visited" bit from feeding back to mail clients like Eudora.

Is this the oldest unfixed bug ?
Tabbrowser-extension seems to make some tricks which fixes, or go around, this bug.
I made a new profile "firefox -ProfileManager". Created new profile.
Then started firefox and installed the Tabbrowser extension 
< http://piro.sakura.ne.jp/xul/tabextensions/index.html.en >
Restarted Firefox. It asks what preconfigure profile to use. I chose the
expert-mode. After that the visited links get colored correctly when the link is
opened on another frame (at least).
*** Bug 224465 has been marked as a duplicate of this bug. ***
Blocks: 25189
(In reply to comment #94)
> Related to bug 25189?
It seems to be a lot older and deeper. It's a core bug from the last millenium. 
The "visited" bit is not properly fed back to the application that called a 
link. Tab extensions that band-aid on a workaround for certain aspects of this 
bug are not a fix. It would take a core programmer, not just chrome polishers, 
to take care of that. 
Since nobody is capable or interested in fixing this bug, the only solution  
seems to be to upgrade to MSIE or Opera, if you need to know which links have 
been visited.
I'm not sure this is terribly hard. All pres shells (or docshells or something)
would need to get a notifcation when a URL loads in other tabs or windows. Then
they just need to cause style to be re-resolved for links that match the URL.
The style system will go and look up the link on global history to see if it's
visited.
It's not that hard to do, but it's not trivial either, because it requires being
quite careful about both performance and memory use.  We'd probably need to
maintain a per-document or per-presentation hashtable (probably raw use of
pldhash is best, since it allows 8-byte entries, with the key reachable from the
value) that contains all elements that implement nsILink (keyed by their URI).
Yeah, the hard part isn't doing it, but doing it such that clicking a link when
you have 50-60 windows/tabs open doesn't freeze up your computer for 5-10 seconds.
*** Bug 25189 has been marked as a duplicate of this bug. ***
(And the other part that may be fun is adding an observer mechanism to global
history without modifying any frozen interfaces or breaking people we "care
about" who depend on unfrozen interfaces.)
I don't think global history itself should be the one to fire off 'page added to
global history' notifications (partly for selfish reasons, because camino now
uses a different GH impl), because that places a burdern on all implementors of GH.

I think the notifications should be fired from the spot where we call into GH to
add a visited link (but I'm not sure how many there are).

> we'd probably need to
> maintain a per-document or per-presentation hashtable (probably raw use of
> pldhash is best, since it allows 8-byte entries, with the key reachable from the
> value) that contains all elements that implement nsILink (keyed by their URI)

Yeah, I was thinking along the same lines, or wondering if we could have some
kind of smart NodeList type thing that maintains a live list of anchor nodes.
Note that a link can occur more than once on a page, so your hash would have to
map one to many.
> (but I'm not sure how many there are).

Brief count in lxr shows 6 callers through nsIGlobalHistory (3 in chatzilla, one
in exthandler, 2 in the embedding test apps) and 2 callers through
nsIGlobalHistory2 (docshell and the Firefox UI).  There are also extensions that
may want to flag pages as visited, of course, the way chatzilla and the Firefox
UI do.
(In reply to comment #101)
> Note that a link can occur more than once on a page, so your hash would have to
> map one to many.

Oh, right.  Which means that the storage becomes much uglier (and bigger),
although we could still avoid storing the key by just getting it off the first link.
There isn't really a need for a look-up table in order to come up to standard. 
Other applications, for example Eudora, don't keep looking up anything, IF they 
have received back the "visited" bit. Once Mozilla sends back that feedback to 
whatever called a link, no matter whether it is a different application or a 
different frame on the same page, or a pop-up menu, then this bug and all the 
bugs caused by it, will be fixed.
Helmut: no, it' more complex than that.

If you have a page that has 3 links to http://www.mozilla.org, and you happen to
load http://www.mozilla.org in another window (through any means), then those 3
links should change to the visited color. The link color shows that you have
visited the URL, not where you visited it from.
That is not really Mozilla's concern, but the responsibility of whatever 
application called the link. Since standard applications, and frames, know how 
to take care of that, we don't have to worry about it. At one time Mozilla used 
to feed back the "visited" bit quite properly, and in the early 90's the links 
in Pegasus used to turn color after you clicked on them to browse to some place. 
That became the standard. I don't know when Mozilla stopped doing that, but I 
have a hunch it was in the late 90's.

> and frames, know how to take care of that

No, frames don't.  That's what this bug is about.  There are no applications
involved in this bug other than Mozilla itself.
Firefox frames are chrome. Symptoms of the bug appearing there are just 
symptoms, not causes.
Naturally chrome can't act on a "visited" bit that has not been sent by core.
Frames are just one of many different end-uses that suffer from not getting that 
data.
(If we do this properly we'd also want to change :visited back to :link when a
link expires from the history.)
If you feel like supporting Hixie, watch your answer, nsGlobalHistory implements 
Unassert. It does not implement Assert though (or Move or Change). These are from 
nsIDataSource, which the seamonkey history implements, same goes for the toolkit 
fork.
The Camino history sadly doesn't implement nsIRDFDataSource, so we can't watch
that through a nsIRDFObserver.
> sadly doesn't implement nsIRDFDataSource
nay, joyfully. Sucking in RDF made Camino's global history Most Painful (O(N^2)
tree building...).

> If we do this properly we'd also want to change :visited back to :link when a
link expires from the history

I think we only expire the history on launch or quit (I forget which), so you
wouldn't need to live update in this case.

Hoewever, we do allow items to be removed from the history, so I guess we need
to deal with live removal for this. It's an edge case; I would be fine with that
not being live.

No, that is the calling application's responsibility, nothing global.
For example, traditionally, in mail readers a visited link remains "visited" 
forever. All we need is the "visited" signal coming back.


Is this working properly or not in the latest Netscape? Just wondering.
(In reply to comment #113)
> Is this working properly or not in the latest Netscape? Just wondering.

Why should it? If it would work, this bug here would have RESOLVED FIXED as
resolution and noone would argue in this bug. btw: This here is Mozilla, not
Netscape ;), maybe Netscape has included some hack so that it works, but that's
not the fix we want.
(In reply to comment #114)
> (In reply to comment #113)
> > Is this working properly or not in the latest Netscape? Just wondering.
> 
> Why should it? If it would work, this bug here would have RESOLVED FIXED as
> resolution and noone would argue in this bug. btw: This here is Mozilla, not
> Netscape ;), maybe Netscape has included some hack so that it works, but that's
> not the fix we want.

Why would or should it? Maybe that "hack" you speak of IS what is wanted. They
have some other features users of Mozilla and Firefox look for, so why not this
basic feature? Thanks for letting me clear that up.

I used Netscape for many years until they messed it up after 4.7
However, I don't remember EVER seeing this bug until Firefox.
*** Bug 287024 has been marked as a duplicate of this bug. ***
How about updating the link-status for each link on a page when the tab/window
gets focus? 
Is this easy to do?
Would it cost a lot in cpu time?
Could it be done as an extension?

Well I don't know how Tabbrowser extension does it, but I have 700 Mhz Duron and
automatic visited link coloring is working well and I cannot see any significant
CPU usage or slowing down.

Go ahead and do not fix this bug, and debate forever what is the philosophical
right way to do it, but I know many IE users who used to try Firefox, but moved
back to IE because this BUG.

It doesn't have to be perfect, if it works there were it works on IE also. Maybe
Tabbrowser extension should be integrated to default Firefox, the fix is already
there.
*** Bug 295524 has been marked as a duplicate of this bug. ***
No longer blocks: majorbugs
Flags: blocking1.8b4?
Gentlemen;

Is there any possiblity that this bug could be given more priority?  
This bug is holding up the possible fix for bug :
https://bugzilla.mozilla.org/show_bug.cgi?id=293235

Bug 293235 is a history issue with bfcache enabled.  Links are NOT marked as
read until the page is refreshed.  This is bothersome when working in forums
and the links are not marked as read.  One cannot easily keep track of the ones
you have looked at, or need to revisit. Manually 'refreshing' the page is 
not convenient as it defeats the purpose of bfcache. 

The new feature bfcache proposed for release with 1.1 to gain parity with
Opera will be seriously impaired if this 'read links' issue is not corrected.
Many will just turn the feature off..and complain about another 
'Useless Firefox feature'. 

This bug needs to be set to a higher priority and addressed soon, as we 
are quickly approaching the release of 1.8b3, and once 1.1 branches, I'm not
sure of this getting the attention to correct the problem. 

Thanks for you time. 

It's an old core bug. The chrome polishers can't fix that. 
You'll just have to use a different browser when you need the link colors to 
work in standard fashion.
Blocks: 293235
Why not?
Flags: blocking-aviary1.1?
No longer blocks: 293235
*** Bug 169994 has been marked as a duplicate of this bug. ***
Here's a possible approach:

Keep in each nsDocument a list of URIs that have been visited since we last
updated the links-visited status. Also keep in nsDocument an *optional* map from
URIs to (set of content nodes). By default this map is not populated.

In nsDocShell:AddToGlobalHistory, if the URI was not previously in the history,
notify "link-visited" observers with the URI as the subject. nsDocuments observe
that topic and respond by checking their visiblity (as determined by calls to
nsDocument::OnPageHide/OnPageShow). If the page is not visible, the URI is added
to the list. Otherwise, build the URI-to-nodes map (if it's not already built)
and use it to figure out which nodes need to be restyled, and restyle them in
all the document's presshells. Also, in OnPageShow, if the dirty URI list is
nonempty, use the map to restyle relevant nodes.

Add and remove nodes from the URI-to-nodes map as nodes enter and leave the
document --- but only if the map has been populated.

This approach would have the property that as you browse, links in hidden pages
do not need to be updated. In particular if you visit a new page, and then click
through to another page and never go back to the old page, we won't ever build
the URI-to-nodes map. We will accumulate a list of "dirty URIs" in each hidden
document but that's probably okay since all hidden documents will be sharing the
URIs on that list, so we're only using one pointer per visited URI per hidden
document, so an average of 1.5 pointers per (dirty URI, hidden document) if the
list is implemented using an nsVoidArray.
roc, is this something you'd be willing to take on? What kind of risk would that
pose (we're getting pretty late in the game now.)
I'm working on it right now.
Flags: blocking-aviary1.1?
Flags: blocking1.8b4? → blocking1.8b4+
Attached patch patch (obsolete) — — Splinter Review 
This fixes the bug and removes the Javascript hack that Firefox was using.

The strategy is to keep around in nsDocument a map from URIs to content
elements that are unvisited links. We don't always have a map; when we need the
map, we recreate it by crawling the DOM. Links are added to the map during
style resolution, so elements without frames (e.g., display:none) don't get
added.

When an unvisited link becomes visited, we fire an observer notification.
PresShell picks it up and tells the document, which triggers restyling via a
new content state change type. I modified the style system to consider
":visited" a state-dependent selector.

While a document is hidden, visited URIs are accumulated in a list. If the
document is shown again, we rebuild the map and process the list. This reduces
overhead for documents in the bfcache.

We don't create a map for a document until it's showing and a link becomes
visited. This means if you just surf from one page to the next in the same
window we often don't ever build its URI map (even if bfcache is on). The
effectiveness of this optimization is limited because documents containing
subdocuments (e.g. IFRAMES) will often get their map created, to mark any links
visited that happen to point to the loaded subdocument. On the other hand, as
you surf around pages already in your history, we won't be building any maps.

The map data structure is a hashmap from (URI hash) to (set of nsIContent*
which are unvisited links to URIs with that hash). The URI hash is just the
hash of the URI's spec string. By keying off the string hash we don't have to
store URIs or strings in the map. Two URIs with the same hash are not a problem
because we explicitly check the URI of each nsIContent* when we do a lookup.
The nsIContent* sets use the "cheapset" pattern to optimize for the case when
there is just one nsIContent* for a given URI hash.

I have made the nsIContent* pointers be owning pointers. This may or may not be
the right thing to do. They shouldn't really need to be owning pointers ---
only in-DOM content should be added to the map, and when we unbind content from
the tree any unvisited link elements should be removed from the map. I'm a bit
worried about the possibility that elements might not be removed from the map
properly --- e.g. if something (e.g. an attribute) changes that changes the
result of GetLinkURI but we fail to notice it, we won't find the content in the
map, and later we might crash if the old URI gets visited and we try to update
a link element that has been deleted. Making the nsIContent pointers be owning
pointers converts this situation into a simple leak of the content --- and only
a temporary leak until the document goes away. I assume this won't create
cycles on the grounds that that if nsIContent elements have strong references
to their documents we would already leak.

I've tried hard to minimize the performance impact on normal operation.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #189340 - Flags: superreview?(bzbarsky)
Attachment #189340 - Flags: review?(bryner)
> If the document is shown again, we rebuild the map and process the list.

This comment is slightly misleading. We don't rebuild the map if there already
is one.
I'm not going to be able to read this in detail for a bit, but some questions
offhand:

1)  Why is Visit(nsIContent* aContent) using string matches for URI comparisons?
    Please use nsIURI::Equals; that's what we have it for.
2)  Why not just use an honest strong ref in the hashtable instead of depending
    on when things are destroyed?

The assumption that nsIContent doesn't hold strong refs to the document is
almost right; we have some cases where this is not the case and we explicitly
have to break the cycles...  Should be ok for now, at least.

(In reply to comment #130)
> I'm not going to be able to read this in detail for a bit, but some questions
> offhand:
> 
> 1)  Why is Visit(nsIContent* aContent) using string matches for URI
> comparisons?
>     Please use nsIURI::Equals; that's what we have it for.

There's no easy way to get a hashcode from an nsIURI so I use the hash of the
spec string, which means it makes sense to use string equality here for
consistency --- URIs with different specs that are "equal" were probably hashed
to different sets anyway. Plus, global history stores and matches on strings,
not URIs.

> 2)  Why not just use an honest strong ref in the hashtable instead of
> depending on when things are destroyed?

Not sure which hashtable you mean. In nsUint32ToContentHashEntry, void* does
double duty as a pointer to a HashSet and a pointer to single nsIContent* so I
don't think I can use nsCOMPtr there. The HashSet does use nsCOMPtr for its
elements via nsISupportsHashKey.
> Plus, global history stores and matches on strings, not URIs.

OK, that's a good argument...

> Not sure which hashtable you mean.

The one that has:

+      // Pathetic attempt to not die: clear out the other mValOrHash so we're
+      // effectively stealing it. If toCopy is destroyed right after this,
+      // we'll be OK.
+      NS_CONST_CAST(nsUint32ToContentHashEntry&, toCopy).mValOrHash = nsnull;
+      NS_ERROR("Copying not supported. Fasten your seat belt.");

I guess you don't hit this code much, given the NS_ERROR.  But then why bother
with the "pathetic attempt" thing?  Alternately, have this actually NS_ADDREF as
needed instead of just copying the ptr when it's an nsIContent*.
> We don't always have a map; when we need the map, we recreate it by crawling
> the DOM. Links are added to the map during style resolution, so elements
> without frames (e.g., display:none) don't get added.

Not wanting to chip in uninvitedly, but this sounds like a bug. I can already
see myself filing it: "links do not become :visited colour if they were hidden
using 'display:none' and become visible via JavaScript".
(In reply to comment #132)
> > Not sure which hashtable you mean.
> 
> The one that has:
> 
> +      // Pathetic attempt to not die: clear out the other mValOrHash so we're
> +      // effectively stealing it. If toCopy is destroyed right after this,
> +      // we'll be OK.
> +      NS_CONST_CAST(nsUint32ToContentHashEntry&, toCopy).mValOrHash = nsnull;
> +      NS_ERROR("Copying not supported. Fasten your seat belt.");
> 
> I guess you don't hit this code much, given the NS_ERROR.

We shouldn't hit it since we allow nsTHashtable to use MEMMOVE instead of
copying. We certainly don't hit it in my testing.

> But then why bother with the "pathetic attempt" thing?

In case I'm wrong :-). And the nsTHashtable copy code does in fact destroy the
original immediately afterward, so it would work.

> Alternately, have this
> actually NS_ADDREF as needed instead of just copying the ptr when it's an
> nsIContent*.

We could do that. But copying still wouldn't work according to spec since the
hashset case would require us to clone the entire hashset, which is a pain to
implement for something that never actually gets called.
Can someone tell me how to stop getting CCed on this stupid bug?  I'm not the submitter, I'm not the 
assignee, and I'm not on the CC list.  Why am I still getting mail about it?

I know I could change my bugzilla options to stop getting mail about *all* bugs, but really I just want to 
stop hearing about this one.
hmm, don't we have
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsURIHashKey.h for
URI hash keys?

jwz: I guess you voted for this bug.

(In reply to comment #133)
> Not wanting to chip in uninvitedly, but this sounds like a bug. I can already
> see myself filing it: "links do not become :visited colour if they were hidden
> using 'display:none' and become visible via JavaScript".

I'm pretty sure that that's not what'd happen, since changing display should
resolve style for that link and thus be styled correctly (like today)
(In reply to comment #133)
> > We don't always have a map; when we need the map, we recreate it by crawling
> > the DOM. Links are added to the map during style resolution, so elements
> > without frames (e.g., display:none) don't get added.
> 
> Not wanting to chip in uninvitedly, but this sounds like a bug. I can already
> see myself filing it: "links do not become :visited colour if they were hidden
> using 'display:none' and become visible via JavaScript".

Good thought! But actually I oversimplified the explanation :-). We add a link
to the map if/when the style system tests its visitedness for the first time and
finds that it's unvisited. So if a link isn't added to the map then the style
system doesn't care if it's visited or not. In particular if Javascript changes
the DOM to make a link element visible somehow, then the style system will query
its visitedness and it will be added to the map if it's unvisited.
(In reply to comment #136)
> hmm, don't we have
> http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsURIHashKey.h for
> URI hash keys?

We do, but if I used the URIs as hash keys, we'd keep around lots of URI
objects. This way, we just have to keep around a PRUint32.
Can someone tell me how to stop getting CCed on this stupid bug?  I'm not the submitter, I'm not the 
assignee, and I'm not on the CC list.  Why am I still getting mail about it?

I know I could change my bugzilla options to stop getting mail about *all* bugs, but really I just want to 
stop hearing about this one.
(In reply to comment #139)
> Can someone tell me how to stop getting CCed on this stupid bug?

https://bugzilla.mozilla.org/votes.cgi?action=show_bug&bug_id=78510
shows that you voted for it, and
https://bugzilla.mozilla.org/userprefs.cgi?tab=email probably says that
you get email for bugs you voted for.

But please avoid adding such comments to bugs; there are other forums for them,
and this bug is long enough already.
Am I right in assuming as others think so that by fixing this bug it would also
cure [url=https://bugzilla.mozilla.org/show_bug.cgi?id=293235]#293235
[/url][Core]-when using the back button (or keyboard), visited links are not
marked as visited [All]?
Yes, it fixes bug 293235. That's the main reason I've done it.
Blocks: 293235
(In reply to comment #142)
> Yes, it fixes bug 293235. That's the main reason I've done it.

Thankyou, that bug imo was one of the main reasons for BFcache not being fully
useable to the mainstream. Do you think the patch will be checked in by next
week, or is there alot more work? I will just read your reply and accept as I
dont want to fill this bug up with a questionnaire lol.
Comment on attachment 189340 [details] [diff] [review]
patch

roc: Nice!

As far as I can tell, the patch doesn't handle resetting the :link/:visited
state of all links when the global history is emptied or when entries are
evicted from it. Could this result in inconsistent display of link colouring?
Comment on attachment 189340 [details] [diff] [review]
patch

roc: Nice!

As far as I can tell, the patch doesn't handle resetting the :link/:visited
state of all links when the global history is emptied or when entries are
evicted from it. Could this result in inconsistent display of link colouring?
(In reply to comment #145) 
> As far as I can tell, the patch doesn't handle resetting the :link/:visited
> state of all links when the global history is emptied or when entries are
> evicted from it. Could this result in inconsistent display of link colouring?

Yes. I ignored that based on comment #111. Assuming Simon is correct, removing
items from history while the browser is running is very rare so it could be
handled as a completely separate issue (we can probably just recrawl every DOM
to check and update all visited links).
Please do not forget to change the UUID for nsIDocument.
Comment on attachment 189340 [details] [diff] [review]
patch

General comments:

It seems like most documents will have at least one unvisited link.  Maybe it
would make sense to have mVisitedLinks be part of the nsIDocument object
instead of a separate heap-allocation.	This would allow you to avoid
null-checking it on each URI we insert.


+    struct Visitor {
+      virtual void Visit(nsIContent* aContent) = 0;
+    };

I'm not clear on why this needs to be virtual... can't you just forward-declare
the struct and static_cast to URIVisitNotifier?


>--- content/base/src/nsDocument.cpp	22 Jun 2005 01:53:57 -0000	3.560
>+++ content/base/src/nsDocument.cpp	14 Jul 2005 19:15:59 -0000
>@@ -126,24 +126,215 @@ static NS_DEFINE_CID(kDOMEventGroupCID, 

>+  private:
>+    const PRUint32 mValue;
>+    /** A hash or nsIContent ptr, depending on the lower bit (0=hash, 1=string) */

Is this supposed to be 1=ptr ?	There are several other places as well where
you use "string" when referring to the nsIContent pointer.


--- toolkit/components/history/src/nsGlobalHistory.cpp	9 Jun 2005 20:51:17
-0000	    1.57
+++ toolkit/components/history/src/nsGlobalHistory.cpp	14 Jul 2005 02:04:58
-0000
-  rv = FindRow(kToken_URLColumn, URISpec.get(), nsnull);
+  nsCOMPtr<nsIMdbRow> row;
+  rv = FindRow(kToken_URLColumn, URISpec.get(), getter_AddRefs(row));	

Not using the XPCOM mork wrapper actually saved a fair amount of malloc
overhead here for checking link-visited state.	Do you think you could do
something where you use the OID directly to check whether the TypedColumn cell
is present?

--- toolkit/content/contentAreaUtils.js 28 Jun 2005 14:41:11 -0000	1.71
+++ toolkit/content/contentAreaUtils.js 14 Jul 2005 02:04:58 -0000
+
+// linkNode is not used anymore

Hm, care to remove it?

Looks really good to me otherwise, thanks for doing this.  r=me with those
changes.
Attachment #189340 - Flags: review?(bryner) → review+
(In reply to comment #148)
> (From update of attachment 189340 [details] [diff] [review] [edit])
> General comments:
> 
> It seems like most documents will have at least one unvisited link.  Maybe it
> would make sense to have mVisitedLinks be part of the nsIDocument object
> instead of a separate heap-allocation.	This would allow you to avoid
> null-checking it on each URI we insert.

See comment #128:
We don't create a map for a document until it's showing and a link becomes
visited. This means if you just surf from one page to the next in the same
window we often don't ever build its URI map (even if bfcache is on). The
effectiveness of this optimization is limited because documents containing
subdocuments (e.g. IFRAMES) will often get their map created, to mark any links
visited that happen to point to the loaded subdocument. On the other hand, as
you surf around pages already in your history, we won't be building any maps.

> +    struct Visitor {
> +      virtual void Visit(nsIContent* aContent) = 0;
> +    };
> 
> I'm not clear on why this needs to be virtual... can't you just
> forward-declare the struct and static_cast to URIVisitNotifier?

We could but I did it this way in case we ever want to reuse this structure.

> >--- content/base/src/nsDocument.cpp	22 Jun 2005 01:53:57 -0000	3.560
> >+++ content/base/src/nsDocument.cpp	14 Jul 2005 19:15:59 -0000
> >@@ -126,24 +126,215 @@ static NS_DEFINE_CID(kDOMEventGroupCID, 
> 
> >+  private:
> >+    const PRUint32 mValue;
> >+    /** A hash or nsIContent ptr, depending on the lower bit (0=hash,
1=string) */
> 
> Is this supposed to be 1=ptr ?	There are several other places as well where
> you use "string" when referring to the nsIContent pointer.

Sorry, incomplete copy-paste. I'll fix.

> --- toolkit/components/history/src/nsGlobalHistory.cpp	9 Jun 2005 20:51:17
> -0000	    1.57
> +++ toolkit/components/history/src/nsGlobalHistory.cpp	14 Jul 2005 02:04:58
> -0000
> -  rv = FindRow(kToken_URLColumn, URISpec.get(), nsnull);
> +  nsCOMPtr<nsIMdbRow> row;
> +  rv = FindRow(kToken_URLColumn, URISpec.get(), getter_AddRefs(row));	
> 
> Not using the XPCOM mork wrapper actually saved a fair amount of malloc
> overhead here for checking link-visited state.	Do you think you could do
> something where you use the OID directly to check whether the TypedColumn cell
> is present?

I'll look into this and post a new patch.

> --- toolkit/content/contentAreaUtils.js 28 Jun 2005 14:41:11 -0000	1.71
> +++ toolkit/content/contentAreaUtils.js 14 Jul 2005 02:04:58 -0000
> +
> +// linkNode is not used anymore
> 
> Hm, care to remove it?

Maybe, but I'm afraid that these functions might be used by extensions or
something. I don't know where the API boundaries are. Furthermore I can imagine
that it might be useful to check the node for some other reason (like if we want
to do something fancy with "rel").
(sorry for duplication of my last comment)

> Yes. I ignored that based on comment #111. Assuming Simon is correct, removing
> items from history while the browser is running is very rare so it could be
> handled as a completely separate issue (we can probably just recrawl every DOM
> to check and update all visited links).

Interesting. I guess that explains why even when I set my global history size to
very small, I have to restart the browser to see link become unvisited again.

Fair enough. If we ever address this it can, as you say, be dealt with later.
(In reply to comment #149)
> See comment #128:

My concern is this case:

1. Start up
2. Go to a page

We won't build up any uri-to-content map during initial load, since the document
hasn't yet seen a link-visited notification.

3. Click a link

If my reading of the docshell code is correct, this will happen after the
original document is marked non-visible, because it happens from the STATE_STOP
notification which is fired from a PLEVent.  So, this action won't cause the
original document to build up a uri-to-content map either, right?

4. Go back  <-- we want this to be near-instantaneous

It seems like here, OnPageShow will result in constructing the uri-to-content
map for the first time, which will crawl the DOM looking for links.  This is the
thing I wanted to avoid... some previous tests I did showed that for large
pages, this cost is significant enough to make fastback not feel instantaneous.
 The main cost is from the call to nsContentUtils::GetLinkURI, because it has to
construct a nsStandardURL and normalize the spec.  This is why I would love to
save the result of this work done by IsHTMLLink when style is resolved initially.

Does this seem like an accurate statement of what happens for the above case?

> > +    struct Visitor {
> > +      virtual void Visit(nsIContent* aContent) = 0;
> > +    };
> > 
> > I'm not clear on why this needs to be virtual... can't you just
> > forward-declare the struct and static_cast to URIVisitNotifier?
> 
> We could but I did it this way in case we ever want to reuse this structure.
> 

This is called for every unvisited link in a page, so I'm a little worried about
unneeded virtual function call overhead for pages with lots of links (like lxr).

> > --- toolkit/content/contentAreaUtils.js 28 Jun 2005 14:41:11 -0000	1.71
> > +++ toolkit/content/contentAreaUtils.js 14 Jul 2005 02:04:58 -0000
> > +
> > +// linkNode is not used anymore
> > 
> > Hm, care to remove it?
> 
> Maybe, but I'm afraid that these functions might be used by extensions or
> something. I don't know where the API boundaries are. Furthermore I can imagine
> that it might be useful to check the node for some other reason (like if we want
> to do something fancy with "rel").

Could be.  You should check with ben or mconnor.

(In reply to comment #151)
> (In reply to comment #149)
> > See comment #128:
> 
> My concern is this case:
> 
> 1. Start up
> 2. Go to a page
> 
> We won't build up any uri-to-content map during initial load, since the document
> hasn't yet seen a link-visited notification.
> 
> 3. Click a link
> 
> If my reading of the docshell code is correct, this will happen after the
> original document is marked non-visible, because it happens from the STATE_STOP
> notification which is fired from a PLEVent.  So, this action won't cause the
> original document to build up a uri-to-content map either, right?

Correct, that's by design.

> 4. Go back  <-- we want this to be near-instantaneous
> 
> It seems like here, OnPageShow will result in constructing the uri-to-content
> map for the first time, which will crawl the DOM looking for links.  This is
> the thing I wanted to avoid... some previous tests I did showed that for large
> pages, this cost is significant enough to make fastback not feel
> instantaneous.  The main cost is from the call to nsContentUtils::GetLinkURI,
> because it has to construct a nsStandardURL and normalize the spec.  This is
> why I would love to save the result of this work done by IsHTMLLink when style
> is resolved initially.

Sure. Here we have a tradeoff between eagerly building the map and lazily
building it. The eager construction is faster but the lazy construction may mean
we never have to build it. It's easy to make this patch work either way. I'll
make it work eagerly.

> > > +    struct Visitor {
> > > +      virtual void Visit(nsIContent* aContent) = 0;
> > > +    };
> > > 
> > > I'm not clear on why this needs to be virtual... can't you just
> > > forward-declare the struct and static_cast to URIVisitNotifier?
> > 
> > We could but I did it this way in case we ever want to reuse this structure.
> 
> This is called for every unvisited link in a page, so I'm a little worried
> about unneeded virtual function call overhead for pages with lots of links
> (like lxr).

I thought a lot about LXR :-). This doesn't get called for every unvisited link
on a page, though. Assuming no hash collisions, it only gets called for links in
pages that are actually turning from unvisited to visited.

> > > --- toolkit/content/contentAreaUtils.js 28 Jun 2005 14:41:11 -0000	1.71
> > > +++ toolkit/content/contentAreaUtils.js 14 Jul 2005 02:04:58 -0000
> > > +
> > > +// linkNode is not used anymore
> > > 
> > > Hm, care to remove it?
> > 
> > Maybe, but I'm afraid that these functions might be used by extensions or
> > something. I don't know where the API boundaries are. Furthermore I can imagine
> > that it might be useful to check the node for some other reason (like if we want
> > to do something fancy with "rel").
> 
> Could be.  You should check with ben or mconnor.

I think I'll just leave it if that's OK.
(In reply to comment #152)
> Sure. Here we have a tradeoff between eagerly building the map and lazily
> building it. The eager construction is faster but the lazy construction may mean
> we never have to build it. It's easy to make this patch work either way. I'll
> make it work eagerly.

Let's start off with that and see what the Tp impact is.  If it's unacceptably
large, we can start tweaking it.

> > This is called for every unvisited link in a page, so I'm a little worried
> > about unneeded virtual function call overhead for pages with lots of links
> > (like lxr).
> 
> I thought a lot about LXR :-). This doesn't get called for every unvisited link
> on a page, though. Assuming no hash collisions, it only gets called for links in
> pages that are actually turning from unvisited to visited.
> 

Ah, fair enough.

> I think I'll just leave it if that's OK.

Sure.

> Not using the XPCOM mork wrapper actually saved a fair amount of malloc
> overhead here for checking link-visited state. Do you think you could do
> something where you use the OID directly to check whether the TypedColumn cell
> is present?

I can't see how to do this through the mdb interfaces.

A cheaper approach might be to keep in the global history object (in memory) a
hashset of the URI specs that have been "marked as typed and hidden", removing a
URI spec from the set if the URI is successfully loaded in
AddExistingPageToDatabase, and testing this set in IsVisited.

This would effectively leak all URI strings that the user typed in but didn't
successfully load. I think this is not a problem.

What do you think?
Just be aware that Camino has its own history implementation (which is still
mork-based, but without RDF). It will need the sGlobalHistory.cpp patch.
Simon, the attached patch patches Camino
Attachment #189340 - Flags: superreview?(bzbarsky)
Attached patch fix #2 (obsolete) — — Splinter Review 
Okay, this patch addresses all the comments. Using a hashset to remember which
URLs have been typed-but-not-visited seems to work well. One leaked string per
typed-URL-that-never-turned-visited seems fine to me.
Attachment #189340 - Attachment is obsolete: true
Attachment #189960 - Flags: superreview?(bzbarsky)
Attachment #189960 - Flags: review?(bryner)
Attachment #189960 - Flags: review?(bryner) → review+
Sorry, some changes to nsAppRunner.cpp sneaked in there, they're not part of
this patch.
Whiteboard: parity-opera → parity-opera [has patch, has review, needs superreview (bzbarsky)]
*** Bug 301846 has been marked as a duplicate of this bug. ***
Comment on attachment 189960 [details] [diff] [review]
fix #2

>Index: layout/style/nsCSSStyleSheet.cpp
...
> // This function should return true only for selectors that need to be
> // checked by |HasStateDependentStyle|.
> inline
> PRBool IsStateSelector(nsCSSSelector& aSelector)
...
>-        (pseudoClass->mAtom == nsCSSPseudoClasses::target)) {
>+        (pseudoClass->mAtom == nsCSSPseudoClasses::target) ||
>+        (pseudoClass->mAtom == nsCSSPseudoClasses::visited)) {
>       return PR_TRUE;

This seems wrong, since a change in a links visited state affects the matching
of both :link and :visited, so at the very least you need to add :link as well.
 But for this to have the right affect you'd also need to change
SelectorMatches to handle the aStateMask argument for this state.  And you
probably also need to fix nsHTMLStyleSheet::HasStateDependentStyle.
That said, an alternative that's probably reasonable considering that the pref
stylesheet is almost always present would be to hard-code a true result for the
visited state in nsCSSStyleSheet's HasStateDependentStyle.  Perhaps we don't
care about the set of people who want to optimize away link coloring performance
issues but still want global history (and the way we construct the pref
stylesheet probably already hurts them).

And apologies for the horrible usage in my previous comment: s/links/link's/,
s/affect/effect/.
New patch coming up.

I thought I could shrink the hashentry down to 8 bytes by using the keyHash
stored in the PLD header and not storing the URI spec hash at all, but the
pldhash documentation says I can't do that. So we're looking at bloat of about
12 bytes per unvisited link times whatever the normal hash table occupancy is.
Attached patch fix #3 (obsolete) — — Splinter Review 
Incorporates David's comments ... we decided on IRC that comment #161 wouldn't
work.
Attachment #189960 - Attachment is obsolete: true
Attachment #190500 - Flags: superreview?(bugmail)
Attachment #190500 - Flags: review+
Comment on attachment 190500 [details] [diff] [review]
fix #3

The new changes to nsCSSStyleSheet.cpp and nsHTMLStyleSheet.cpp look good to
me, although it might be good (in the latter) to put the |aData->mContentTag ==
nsHTMLAtoms::a| check first in the long chain of &&-ed conditions, since it's
much more likely to fail than the things before it.
Is there any way we can make the ForgetUnvisitedLink() call in
NotifyURIVisited() not have to do the hashtable lookup again?  That is, why not
just have VisitContent() remove the link from the hashtable after storing it in
the Visitor?
(In reply to comment #166)
> Is there any way we can make the ForgetUnvisitedLink() call in
> NotifyURIVisited() not have to do the hashtable lookup again?  That is, why not
> just have VisitContent() remove the link from the hashtable after storing it in
> the Visitor?

Perhaps we could, but I would have check the safety of doing that from within
the iterator. It's not a performance issue since the cost of the extra hashtable
lookup will be swamped by the cost of changing the style --- not to mention the
cost of loading the document that's being visited.
Boris, if you're going to read the code maybe you should just sr it while you're
there :-)
Attached patch fix #4 (obsolete) — — Splinter Review 
I noticed that the previous patch actually never removed anything from the
mUnvisitedLinks table. This patch fixes that so that empty sets are removed
from the map. It also removes elements as we iterate through a set, so we don't
need a list of content to forget.

There is no way to avoid the hashlookup in mUnvisitedLinks->mTable, though...
pldhash doesn't have a way to combine a GetEntry with an optional RemoveEntry.
Attachment #190500 - Attachment is obsolete: true
Attachment #190648 - Flags: superreview?(bugmail)
Attachment #190648 - Flags: review+
The document being loaded is one; the style changes, etc, could happen in all
sorts of windows...  I agree the style change is probably more expensive than
the hash lookup (or rather lookups if we have a hashset).

Another option is to factor out the part after we do the hash lookup there into
a separate function that takes a hash entry and to call that from both places...

And yes, if I read enough of the code to sr I'll mark it.  Haven't gotten even
close yet, but I'm trying to.
(In reply to comment #169)
> There is no way to avoid the hashlookup in mUnvisitedLinks->mTable, though...
> pldhash doesn't have a way to combine a GetEntry with an optional RemoveEntry.

pldhash most certainly does.  It's just that nsTHashtable (etc.) doesn't expose it.
(In reply to comment #171)
> pldhash most certainly does.  It's just that nsTHashtable (etc.) doesn't expose
> it.

Are you talking about PL_DHashTableRawRemove? nsTHashtable does expose it, but
it's not what we want here because we want the hashtable to shrink after
removal, if appropriate, and PL_DHashTableRawRemove never shrinks.
Visiting enough links that the hashtable might shrink seems like an edge case.
Trying to avoid one hashtable lookup here is also rather hair-splitting, to me,
but I'm happy to call RawRemoveEntry if you wish.
Attachment #189960 - Flags: superreview?(bzbarsky)
Attached patch fix #5 (obsolete) — — Splinter Review 
Fixed UnbindFromTree in nsHTMLAnchorElement, nsHTMLLinkElement and
nsHTMLAreaElement to clear the link state cache ... this is necessary for
correctness in case they're reinserted under a different xml:base. Also, it
ensures that on reinsertion, the style system will recompute the state and the
link will be added back to the unvisited map if necessary.

This also does a RawRemoveEntry in NotifyLinkVisited. I also added RemoveEntry
to remove empty sets in ForgetUnvisitedLink. I think this should permit
hashtable resizing because it's quite plausible that a chunk of links will get
removed from the document or their hrefs changed.
Attachment #190648 - Attachment is obsolete: true
Attachment #190774 - Flags: superreview?(bzbarsky)
Attachment #190774 - Flags: review+
So my review time has been almost completely taken up by bug 296639 and will be
for the foreseeable future (till that patch lands, basically) ... :(
In nsGlobalHistory::IsVisited, you don't need this change anymore:

-  rv = FindRow(kToken_URLColumn, URISpec.get(), nsnull);
+  nsCOMPtr<nsIMdbRow> row;
+  rv = FindRow(kToken_URLColumn, URISpec.get(), getter_AddRefs(row));

You should also explain that the mTypedVisitedURIs check is equivalent to
checking the "typed" and "hidden" status of the row, except you want to avoid
getting the row to avoid memory allocations.  Still, depending on
"typed"+"hidden" URIs not being saved to disk seems a little odd, since it seems
like perhaps that status *ought* to be saved to disk.

(And make sure to keep all the copies of global history in sync!)
+  mTypedHiddenURIs.Init(3);

There's not much point to initializing to less than PL_DHASH_MIN_SIZE, which is
16.  Of course, a whole bunch of other examples do it too.  Also, this
nsHashSets.h stuff uses nsDoubleHashtable, which I think is considered
deprecated (I sure hope so).  Does nsTHashtable give you a good way to do it? 
Then again, this particular use may not be so bad, since it's defined in XPCOM.
Comment on attachment 190774 [details] [diff] [review]
fix #5

I'm not sure how portable the definition of URIToContentMap outside the
definition of its enclosing class is, but we'll see -- it's something worth
trying.  However, when dealing with nested classes you should always declare
them friends of the enclosing class to deal with portability (compilers that
strictly follow the old 1998 version of the C++ standard), so the forward
declaration of URIToContentMap should be immediately followed by:

friend struct URIToContentMap;
Attachment #190774 - Flags: superreview?(bzbarsky) → superreview?(dbaron)
(In reply to comment #177)
> You should also explain that the mTypedVisitedURIs check is equivalent to
> checking the "typed" and "hidden" status of the row, except you want to avoid
> getting the row to avoid memory allocations.  Still, depending on
> "typed"+"hidden" URIs not being saved to disk seems a little odd, since it
> seems like perhaps that status *ought* to be saved to disk.

They may in fact be written to disk, they just get expired instantly whenever
expiration is checked.

> (And make sure to keep all the copies of global history in sync!)

Will do.

(In reply to comment #178)
> +  mTypedHiddenURIs.Init(3);
> 
> There's not much point to initializing to less than PL_DHASH_MIN_SIZE, which
> is 16.  Of course, a whole bunch of other examples do it too.

I'd rather not know/care about that implementation detail :-)

> Also, this nsHashSets.h stuff uses nsDoubleHashtable, which I think is
> considered deprecated (I sure hope so).  Does nsTHashtable give you a good way
> to do it? Then again, this particular use may not be so bad, since it's 
> defined in XPCOM.

At some stage someone should reimplement nsHashSets on top of nsTHashtable but
for now this can stay, as per IRC.

(In reply to comment #179)
> I'm not sure how portable the definition of URIToContentMap outside the
> definition of its enclosing class is, but we'll see -- it's something worth
> trying.  However, when dealing with nested classes you should always declare
> them friends of the enclosing class to deal with portability (compilers that
> strictly follow the old 1998 version of the C++ standard), so the forward
> declaration of URIToContentMap should be immediately followed by:
> 
> friend struct URIToContentMap;

Will do.

Let me know if you want to see a new patch.
The potential nullness of nsDocument::mUnvisitedLinks seems odd.  It's
constructed eagerly, but then destroyed in some edge cases.  This seems like a
bad idea, since having mUnvisitedLinks null will be very poorly tested.  Why not
just have it around all the time?

It looks like you're adding some code to handle changes to xml:base, but we
probably don't handle that properly in other cases (e.g., cached link state on
link elements).  Are bugs filed?  Is there much point in the handling you've
added given what isn't there?  Should changing xml:base instead trigger an
UnbindFromTree and BindToTree, since the handling is similar?

The interface documentation for ForgetUnvisitedLink should mention what nullness
of the URI parameter means.  Or perhaps the URI parameter should be removed,
since it appears to be always null.

Is there a good reason to want to store only unvisited links?  What if links
change from visited to unvisited due to history expiration or history being
cleared?  It seems simple enough to store all links, and not much more
expensive, since generally most links, especially on pages with large numbers of
links, are unvisited.
nsDocument::UpdateVisitedStateIfNecessary should assert that mVisible is true. 
Otherwise it just clears the table.

Should URIVisitNotifier be using nsIURI::Equals or nsCString::Equals?  If we use
the latter for other global history checks, it's probably worth commenting at least.
I don't see why you call ForgetAllUnvisitedLinks if any attribute in the xlink
namespace changes.  Why not just the one link?  (But in
nsGenericElement::UnbindFromTree, you check for the type attribute.  Why that
attribute specifically?)
(In reply to comment #181)
> The potential nullness of nsDocument::mUnvisitedLinks seems odd.  It's
> constructed eagerly, but then destroyed in some edge cases.  This seems like a
> bad idea, since having mUnvisitedLinks null will be very poorly tested.  Why
> not just have it around all the time?

That's mainly to handle an xml:base change where I don't know which URIs will be
affected, and I don't want to reconstruct the entire link map by scanning the
DOM right now. I suppose I could just scan the subtree rooted at the current node.

> It looks like you're adding some code to handle changes to xml:base, but we
> probably don't handle that properly in other cases (e.g., cached link state on
> link elements).  Are bugs filed?

Not as far as I can tell.

> Is there much point in the handling you've
> added given what isn't there?  Should changing xml:base instead trigger an
> UnbindFromTree and BindToTree, since the handling is similar?

I can take that out, and that will remove the need for the code that destroys
the link map and recreates it. Of course, if/when those xml:base bugs get fixed,
someone will need to re-add some of that.

> The interface documentation for ForgetUnvisitedLink should mention what
> nullness of the URI parameter means.  Or perhaps the URI parameter should be
> removed, since it appears to be always null.

I'll remove it.

> Is there a good reason to want to store only unvisited links?  What if links
> change from visited to unvisited due to history expiration or history being
> cleared?  It seems simple enough to store all links, and not much more
> expensive, since generally most links, especially on pages with large numbers
> of links, are unvisited.

True, but see comment #111 and comment #146. I think only manual removals from
history are relevant, and I think a solution that crawls all DOMs would be
reasonable there. I'm undecided; I'd be happy to put all (style-relevant) links
in the map if you think it's the right thing to do.

(In reply to comment #182)
> nsDocument::UpdateVisitedStateIfNecessary should assert that mVisible is true. 
> Otherwise it just clears the table.

OK.

> Should URIVisitNotifier be using nsIURI::Equals or nsCString::Equals?  If we
> use the latter for other global history checks, it's probably worth commenting
> at least.

nsCString, see comment #131.

(In reply to comment #183)
> I don't see why you call ForgetAllUnvisitedLinks if any attribute in the xlink
> namespace changes.  Why not just the one link? 

Paranoia. I'll fix that.

> (But in
> nsGenericElement::UnbindFromTree, you check for the type attribute.  Why that
> attribute specifically?)

Because checking for any XLink attribute would be slow (maybe) and I believe any
valid XLink has a 'type' attribute.
> and I believe any valid XLink has a 'type' attribute.

Not true for <svg:a>, and going to become false for XLink in general soon --
that WG is redefining the default type to be "simple" if there is no type
attribute set.
Attached patch updated patch #6 — — Splinter Review 
this brings the patch up to date with all comments. In particular the map is
now always maintained and contains all links, visited or not.
Attachment #190774 - Attachment is obsolete: true
Attachment #191928 - Flags: superreview?(dbaron)
Attachment #191928 - Flags: review?(dbaron)
Comment on attachment 191928 [details] [diff] [review]
updated patch #6

nsUint32ToContentHashEntry::DestroyContent should just be called Destroy so
it's not confused with RemoveContent.

I'd probably slightly prefer the UpdateLinkMap check to be an assertion.

r+sr=dbaron
Attachment #191928 - Flags: superreview?(dbaron)
Attachment #191928 - Flags: superreview+
Attachment #191928 - Flags: review?(dbaron)
Attachment #191928 - Flags: review+
Also, I filed bug 303478 on the handling of dynamic changes to xml:base (which I
asked roc to remove from this patch, since the changes here for that case
weren't sufficient even for just this limited issue).
Comment on attachment 191928 [details] [diff] [review]
updated patch #6

Need approval for this 1.8b4 blocker
Attachment #191928 - Flags: approval1.8b4?
Comment on attachment 191928 [details] [diff] [review]
updated patch #6

a=rjesup@wgate.com

I'm willing to approve this given other driver's comments.  Please be very
careful to monitor Tp and memory use, and if possible craft some edge-condition
tests (LXR, Back (from/to a big LXR page), etc) and measure load time and
memory use before and after this patch (though this could be done after
commit).  If it wasn't marked as a blocker I'd probably say no given the
potential impact of the patch and that we're at a b4 release.

My apologies for any paranoia; I spent a lot of time scrubbing perf and mem use
in Gecko.
Attachment #191928 - Flags: approval1.8b4? → approval1.8b4+
checked in. I made the mVisible check in UpdateLinkMap an assertion as well as
an early exit.
marking fixed
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Thank you. Perfect.
In Seamonkey, this didn't affect Tp significantly. It went down a little in some
places and up a little on others. Tdhtml on luna was also not significantly
affected. In Camino, Tp increased by perhaps 15ms (from ~800) - 2%, acceptable
IMHO. In Firefox, however, it went from about 220 to 258 - 17%, on beast; on
pacifica, from about 128 to about 142 - 11%. Not good at all. The increase seems
to be across the board, not focused on any particular page.

As an experiment I tried backing out the nsDocShell changes so that no visited
notifications are triggered. This reduced the Camino Tp hit to about zero. It
doesn't seem to have had much effect on beast or pacifica. So it appears the
cost of building/maintaining the link map is at fault ... but only on Firefox,
for reasons that remain unclear.
One difference between Firefox and Seamonkey that may be relevant is that
Firefox displays favicons by default, and also has Live Bookmarks. Whenever a
LINK element is touched by Javascript, the XPC wrapper code resolves style on
the element to see if it has an XBL binding, and this causes the element to be
added to the link map. So on pages with RSS feeds and/or favicons, Firefox is
adding a few more links to the link map than Seamonkey does. But I don't see how
this could result in such a large performance impact.
Comment on attachment 192324 [details] [diff] [review]
don't leak all of the pres shells

r=jst
Attachment #192324 - Flags: review+
Comment on attachment 192324 [details] [diff] [review]
don't leak all of the pres shells

I could've sworn that was in the patch at one point!
bryner's fix fixed the Firefox performance problems. Seamonkey now shows 2-3% Tp
slowdown which to me seems reasonable but we can discuss it if people want to.
Thanks Brian.
I've undone the backout mentioned in comment #194, so this should be fully
functional on trunk now.
Do the Tp tests use Back (last I remember I didn't think they did)?

And do the Tp tests have really big pages-of-links like nasty LXR pages? (though
there are worse pages than that.)  And combining them, going back to a nasty page.  

What about mem usage?

2-3% in "normal" browsing is a noticable hit.  It may be worth it for the
feature, but now that we have Tp measurements there should be discussion of
whether it's worth 2-3%.  (If every major feature cost 2-3% we'd be in deep shit.)

You might also profile loading some LXR pages (and Back) and some "normal" pages
with/without the patch and look at the differences.  That would actually be very
enlightening, may give a good pointer on how to cut that 2-3%, and give an idea
how linear the impact is.
Comment on attachment 192324 [details] [diff] [review]
don't leak all of the pres shells

r=jst
Attachment #192324 - Flags: review+
The final Tp numbers are roughly as follows:
== Seamonkey ==
btek (Linux): 930 + 30 (3%)
luna (Linux): 880 + 25 (3%)
monkey (Mac): too much variance to detect any significant change
creature (Win32): no change
== Firefox ==
beast (Win32): no signficant change, possibly up by 1ms or so
pacifica (Win32): no significant change
== Camino ==
pawn: lots of variance, but up by 10-20ms (1-2%)

btek is known to be unreliable.

Memory numbers are also interesting. The Seamonky numbers have tons of variance,
but balsa (Linux Firefox) shows max-heap going from 7.96MB to 8.36MB.
(In reply to comment #202)
> Do the Tp tests use Back (last I remember I didn't think they did)?

They don't. I'm not sure why this is relevant though.

> And do the Tp tests have really big pages-of-links like nasty LXR pages?
> (though there are worse pages than that.)

There's one LXR page but it's only about 800 lines (say 2000 links).

> 2-3% in "normal" browsing is a noticable hit.  It may be worth it for the
> feature, but now that we have Tp measurements there should be discussion of
> whether it's worth 2-3%.  (If every major feature cost 2-3% we'd be in deep'
> shit.)

Apparently it's about 3% for Seamonky Linux, 1-2% for Camino and nothing much
anywhere else. I would argue that's a pretty clear win:
-- it doesn't hurt the products we care about most
-- it's a bug fix as much as a feature; displaying a visited link as not-visited
is just incorrect
-- this is the sort of fix/feature where we simply have to do more work on every
page to get it right. It's not realistic to expect we can do it with zero
impact. In fact I think we've done rather well to see no visible impact on Firefox.
(In reply to comment #204)
> btek is known to be unreliable.

On the contrary, btken is known to be very reliable - as long as noone touches
the machine itself. It's known to react with wild increases if someone changes
tinderbox configuration, but that's the only unreliability of btek I know of.
Addtitionally, btek and luna being consistent about the change is supporting
that being reliable.

I noticed we have no Linux or Mac machines for Firefox that do Tp, only Windows
machines, so it also could mean that Windows shows no Tp changes but Linux does.
Given that the OSes organize memory differently, but SeaMonkey and Firefox
basically use the same Gecko, this approach even sounds more probable to me.
Given that Camino shows an slight increase and is unix-ish as well, it sounds
even more like this is OS-specific rather than app-specific.

BTW, monkey (SeaMonkey Mac) points in the direction of a small increase as well,
but it's hard to tell correctly, as the usual noise is almost as high as that
signal might be.
 
> Memory numbers are also interesting. The Seamonky numbers have tons of variance,
> but balsa (Linux Firefox) shows max-heap going from 7.96MB to 8.36MB.

brad (SeaMonkey) max-heap is known to be erratic since www.mozilla.org started
dynamically loading big image via JS, true. Unfortunately that benchmark is
barely useable due to that.
brad showed the appearance and fix of leaks very well though :)

Interstingly, while we were leaking, Windows boxes (creature/beast) had worse Tp
and btek on Linux showed a significant decrease during that time before the
rising numbers after the leak fix...
(In reply to comment #206)
> (In reply to comment #204)
> > btek is known to be unreliable.
> 
> On the contrary, btken is known to be very reliable - as long as noone touches
> the machine itself. It's known to react with wild increases if someone changes
> tinderbox configuration, but that's the only unreliability of btek I know of.
> Addtitionally, btek and luna being consistent about the change is supporting
> that being reliable.

It depends what you mean by "reliable". btek showed a 100ms Tp increase when we
changed the Seamonkey branding. Maybe that's reliable, but it's not useful.

> I noticed we have no Linux or Mac machines for Firefox that do Tp, only
> Windows machines, so it also could mean that Windows shows no Tp changes but
> Linux does.

Quite possibly. It would be nice to have a Linux Firefox tinderbox. Of course
the harsh fact (coming from a Linux developer) is that Firefox on Windows is the
single most important product.

> Interstingly, while we were leaking, Windows boxes (creature/beast) had worse
> Tp and btek on Linux showed a significant decrease during that time before the
> rising numbers after the leak fix...

That is another reason why I don't trust btek.

Even after all this has been fixed up, we're still seeing a significant but
small reduction in Tdhtml on luna, which is also very strange.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050811 Firefox/1.0+

I've noticed that with this fix applied, middle-clicking links into background
tabs shows an interesting quirk - the link is not marked as visited until after
that tab's <browser/> begins painting the content it is receiving.

Is this the way the fix is designed to work, or another bug?
From a UI point of view, that's quite reasonable: links shouldn't seem 
"visited" until the page actually starts appearing. But if that means links 
to downloads aren't marked as visited, report a bug. (BTW, Robert: Thanks.)
(In reply to comment #209)
> From a UI point of view, that's quite reasonable: links shouldn't seem 
> "visited" until the page actually starts appearing. But if that means links 
> to downloads aren't marked as visited, report a bug.

Unfortunately enough, left-clicked links to downloadable files aren't marked
visited until the page is reloaded.

Bug 304434 opened.
(In reply to comment #208)
> Is this the way the fix is designed to work, or another bug?

It was designed to work that way. There's a delay before the load is underway
and the page is added to your history; during that delay, the link is not marked
visited.
(In reply to comment #205)
> (In reply to comment #202)
> > Do the Tp tests use Back (last I remember I didn't think they did)?
> 
> They don't. I'm not sure why this is relevant though.

Because part of the issue was that once we take the patch for faster "back" we
won't be reloading pages, so the lack of updating links would be more noticable.
 Back (and reload) can also highlight load-time issues.

> > And do the Tp tests have really big pages-of-links like nasty LXR pages?
> > (though there are worse pages than that.)
> 
> There's one LXR page but it's only about 800 lines (say 2000 links).

That's really not enough to test the edge-cases here.  Tp (not surprisingly)
tests an (OLD) set of "normal" cases.  That is useful and somewhat indicative of
 real-world performance (modulo that at least last I looked, the pages were very
old, and don't stress CSS/JS/DOM/etc as much as current popular pages (outside
of Google) do).

For patches that can have non-linear effects like this one, we really should
time a few edge cases.  Better yet would be a suite of edge-cases tests that are
automated, but ad-hoc tests would do (and honestly, I'd normally prefer they be
done before submission for review/approval).

These edge cases can be important.  For example, look at the bug we used to have
with pages with LARGE numbers of drop-downs (I don't see the bug open, so I
assume it's fixed).  I.e. Slashdot when you have moderator points.  It used to
take circa 20+ seconds to render a slashdot page with dropdowns for each
comment, when it would be circa 1 sec without them.  But pages with "normal"
numbers of dropdowns weren't a problem, and it wouldn't have been noticed in Tp.

Ditto the problems we used to have searching for text in huge LXR pages, which
required a major redesign to fix.

Not saying this has that sort of problem - but it's good to prove it doesn't, or
that any problem doesn't run away from you.

> > 2-3% in "normal" browsing is a noticable hit.  It may be worth it for the
> > feature, but now that we have Tp measurements there should be discussion of
> > whether it's worth 2-3%.  (If every major feature cost 2-3% we'd be in deep'
> > shit.)
> 
> Apparently it's about 3% for Seamonky Linux, 1-2% for Camino and nothing much
> anywhere else. I would argue that's a pretty clear win:
> -- it doesn't hurt the products we care about most

On Tp pages.  On LXR pages or even more modern pages it might.  (And might not)

> -- it's a bug fix as much as a feature; displaying a visited link as
not-visited is just incorrect

Yes.

> -- this is the sort of fix/feature where we simply have to do more work on every
> page to get it right. It's not realistic to expect we can do it with zero
> impact. In fact I think we've done rather well to see no visible impact on
Firefox.

Agreed that you can expect it to take some additional time (and memory).  That's
why we need to consider the tradeoff (improved function vs. time), and that it
doesn't go exponential on us, in time or memory.

The Tp values don't look bad (2-3% is a fair bit, though - I fought hard to find
and commit fixes to get 1% or less Tp wins).  Memory has me a little worried,
though less - I rather imagine it will be linear to links, while CPU could more
likely be exponential.  I worry more about mem-allocator fragmentation and (CPU)
overhead than about memory use.
(In reply to comment #211)
> (In reply to comment #208)
> > Is this the way the fix is designed to work, or another bug?
> 
> It was designed to work that way. There's a delay before the load is underway
> and the page is added to your history; during that delay, the link is not marked
> visited.

Maybe logically it seems right to mark a link as fixed only after the page is
added to the history, but this has 2 problems:

1. Links that point to non-existant URLs aren't marked as visited, even though
you clicked on them and got an error page.
2. This is really bad usability-wise. If you click many links on one site (open
them in new tabs), you don't get the feedback right away!
IE seems to do this (open a link, and once a window is opened for it, mark it as
visited), I think we should do this aswell.
(In reply to comment #213)
> (In reply to comment #211)
> > (In reply to comment #208)
> > > Is this the way the fix is designed to work, or another bug?
> > 
> > It was designed to work that way. There's a delay before the load is underway
> > and the page is added to your history; during that delay, the link is not marked
> > visited.
> 
> Maybe logically it seems right to mark a link as fixed only after the page is
> added to the history, but this has 2 problems:
> 
> 1. Links that point to non-existant URLs aren't marked as visited, even though
> you clicked on them and got an error page.
> 2. This is really bad usability-wise. If you click many links on one site (open
> them in new tabs), you don't get the feedback right away!
> IE seems to do this (open a link, and once a window is opened for it, mark it as
> visited), I think we should do this aswell.


Also, what would happen if you used an extension to open the link? Like Launchy,
ViewInIE, DownloadWith, etc.?
(In reply to comment #213)
> 1. Links that point to non-existant URLs aren't marked as visited, even though
> you clicked on them and got an error page.
Well, if the URL doesn't exist, you can't visit it, can you? So I really think
the link should not be marked visited (and certainly not when you get the error
page instead).

> 2. This is really bad usability-wise. If you click many links on one site (open
> them in new tabs), you don't get the feedback right away!
> IE seems to do this (open a link, and once a window is opened for it, mark it as
> visited), I think we should do this aswell.
Personally, I like current Mozilla's behavior more. But you could file a new bug
about this.

> Also, what would happen if you used an extension to open the link? Like Launchy,
> ViewInIE, DownloadWith, etc.?
If an external program is opening the link, why should Mozilla mark the link as
visited? Mozilla hasn't visited the link then, has it?
(In reply to comment #215)
> (In reply to comment #213)
> > 1. Links that point to non-existant URLs aren't marked as visited, even though
> > you clicked on them and got an error page.
> Well, if the URL doesn't exist, you can't visit it, can you? So I really think
> the link should not be marked visited (and certainly not when you get the error
> page instead).

The reason it isn't marked as visited is because error pages don't go into
history. It doesn't matter that the page does not exist, you clicked the link
didn't you? So you _tried_ to go there.. it should be enough to mark the link as
visited.
The whole point here it to show which links you tried to/have clicked before.

> > 2. This is really bad usability-wise. If you click many links on one site (open
> > them in new tabs), you don't get the feedback right away!
> > IE seems to do this (open a link, and once a window is opened for it, mark it as
> > visited), I think we should do this aswell.
> Personally, I like current Mozilla's behavior more. But you could file a new bug
> about this.

You may like it, but this is not what most users are accustomed to. 
Try this page in both Opera and IE6: http://ddj.com/ftp/

Middle click on the website links and see the results.. Opera & IE both mark the
links as visited as soon as the tab opens.


> > Also, what would happen if you used an extension to open the link? Like Launchy,
> > ViewInIE, DownloadWith, etc.?
> If an external program is opening the link, why should Mozilla mark the link as
> visited? Mozilla hasn't visited the link then, has it?

"Mozilla" hasn't, but you have been in that page... that should be considered as
a visit. But this issue is not really important at the moment..
It is very important to keep link coloring consistent with what's actually in
history. Otherwise you get the situation where reloading a page (or just using
the DOM to remove and re-add a link element) can change the colors of links in
the page, which we want to avoid. Assuming you agree, there is no reason to
change the code here.

If you want to change when (or which) links are actually added to history,
that's fine, but please file it as a separate bug.
No longer blocks: 304434
Depends on: 304434
(In reply to comment #216)
> The whole point here it to show which links you tried to/have clicked before.
 
More precisely, it is to assist users who wish to avoid repeat clicking on links
already clicked, particularly those that opened error pages. If only we had a
practical way to prevent authors from styling visited and unvisited links the
same color. :-(
(In reply to comment #218)
> More precisely, it is to assist users who wish to avoid repeat clicking on links
> already clicked, particularly those that opened error pages.

I disagree. For me, it's a way to avoid repeat clicking links whose contents I
have already seen. Clearly, I didn't see the content at a link that resulted in
an error page yet.

(In reply to comment #219)
> (In reply to comment #218)
> > More precisely, it is to assist users who wish to avoid repeat clicking on links
> > already clicked, particularly those that opened error pages.
 
> I disagree. For me, it's a way to avoid repeat clicking links whose contents I
> have already seen. Clearly, I didn't see the content at a link that resulted in
> an error page yet.

I guess you prefer repeating what doesn't work to trying new things that might
work.  IIUC, Jakob would agree with me: http://www.useit.com/alertbox/20040503.html
I agree with biesi (in that I think the coloring is for knowing what I've
already read, potentially a long-term memory issue, not for remembering what I
clicked on in the past 3 seconds), and I don't think Nielsen's column takes a
position on this issue.
And in any case, to repeat what roc said, THAT DISCUSSION DOES NOT BELONG ON
THIS BUG.
Bug 304759 has addressed the Tp fallout from this, looks like.
Depends on: 304747
*** Bug 260881 has been marked as a duplicate of this bug. ***
*** Bug 249890 has been marked as a duplicate of this bug. ***
Filed bug 305573 as a regression.
If this is fixed on the branch (it's got an associated bonsai comment), please
add the fixed1.8 keyword. Thanks.
Keywords: fixed1.8
*** Bug 307002 has been marked as a duplicate of this bug. ***
*** Bug 313779 has been marked as a duplicate of this bug. ***
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: