Closed
Bug 78510
Opened 24 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)
Core Graveyard
History: Global
Tracking
(Not tracked)
RESOLVED
FIXED
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)
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
jesup
:
approval1.8b4+
|
Details | Diff | Splinter Review |
(deleted),
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.
Reporter | ||
Comment 1•24 years ago
|
||
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.
Comment 2•24 years ago
|
||
I would be very surprised if the code to fix bug 10491 didn't also fix this bug.
Reporter | ||
Comment 3•24 years ago
|
||
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.
Comment 4•24 years ago
|
||
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
Reporter | ||
Comment 5•24 years ago
|
||
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.
Comment 6•24 years ago
|
||
it's not resource intensive if done correctly, via some kind of observer system.. but frankly I don't see the immediate need.
Comment 7•23 years ago
|
||
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.
Reporter | ||
Comment 8•23 years ago
|
||
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.
Comment 9•23 years ago
|
||
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.
Comment 10•23 years ago
|
||
*** Bug 96987 has been marked as a duplicate of this bug. ***
Comment 11•23 years ago
|
||
argh.. take two at reassigning history bugs to new owner
Assignee: alecf → blakeross
Status: ASSIGNED → NEW
Target Milestone: Future → ---
Comment 12•23 years ago
|
||
*** Bug 101066 has been marked as a duplicate of this bug. ***
Comment 13•23 years ago
|
||
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.
Updated•23 years ago
|
Summary: visited link colors should **always** show if link is visited → Link should become :visited color if URL is loaded in any other window
Updated•23 years ago
|
Whiteboard: parity-opera → [Aufbau-P4] parity-opera
Updated•23 years ago
|
Whiteboard: [Aufbau-P4] parity-opera → parity-opera
Comment 14•23 years ago
|
||
*** Bug 10491 has been marked as a duplicate of this bug. ***
Comment 15•23 years ago
|
||
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)...
Comment 16•23 years ago
|
||
*** Bug 108408 has been marked as a duplicate of this bug. ***
Comment 17•23 years ago
|
||
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.
Comment 18•23 years ago
|
||
*** Bug 23465 has been marked as a duplicate of this bug. ***
![]() |
||
Comment 19•23 years ago
|
||
*** Bug 114764 has been marked as a duplicate of this bug. ***
Comment 21•23 years ago
|
||
*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
![]() |
||
Comment 22•23 years ago
|
||
*** Bug 116078 has been marked as a duplicate of this bug. ***
Comment 23•23 years ago
|
||
*** Bug 117135 has been marked as a duplicate of this bug. ***
Comment 24•23 years ago
|
||
*** Bug 118541 has been marked as a duplicate of this bug. ***
Comment 25•23 years ago
|
||
*** Bug 118892 has been marked as a duplicate of this bug. ***
Comment 26•23 years ago
|
||
*** Bug 119607 has been marked as a duplicate of this bug. ***
Comment 27•23 years ago
|
||
*** Bug 119752 has been marked as a duplicate of this bug. ***
Comment 28•23 years ago
|
||
*** Bug 123590 has been marked as a duplicate of this bug. ***
Comment 29•23 years ago
|
||
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.
Comment 30•23 years ago
|
||
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.
Comment 31•23 years ago
|
||
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)
Comment 32•23 years ago
|
||
*** Bug 127772 has been marked as a duplicate of this bug. ***
Comment 33•23 years ago
|
||
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.
Comment 34•23 years ago
|
||
*** Bug 135370 has been marked as a duplicate of this bug. ***
Comment 35•23 years ago
|
||
*** Bug 138691 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
Comment 36•23 years ago
|
||
*** Bug 140710 has been marked as a duplicate of this bug. ***
Comment 37•22 years ago
|
||
*** Bug 144459 has been marked as a duplicate of this bug. ***
Comment 38•22 years ago
|
||
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.
Comment 39•22 years ago
|
||
*** Bug 149113 has been marked as a duplicate of this bug. ***
Comment 40•22 years ago
|
||
*** Bug 151753 has been marked as a duplicate of this bug. ***
Comment 41•22 years ago
|
||
*** Bug 155487 has been marked as a duplicate of this bug. ***
Comment 42•22 years ago
|
||
*** Bug 156035 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Target Milestone: mozilla1.2alpha → Future
Comment 43•22 years ago
|
||
*** Bug 158853 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Comment 44•22 years ago
|
||
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.
Updated•22 years ago
|
Keywords: mozilla1.1
Comment 45•22 years ago
|
||
Is there any way of using Sax to fix this?
Comment 46•22 years ago
|
||
who/what/why is Sax?
Comment 47•22 years ago
|
||
http://www.saxproject.org/
Comment 48•22 years ago
|
||
uhm, no. SAX is irrelevant here. the best solution here is quite gecko-specific
Comment 49•22 years ago
|
||
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.
Comment 50•22 years ago
|
||
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.
Comment 51•22 years ago
|
||
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.
Comment 52•22 years ago
|
||
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
Updated•22 years ago
|
Keywords: mozilla1.3,
nsbeta1
Comment 53•22 years ago
|
||
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...
Comment 54•22 years ago
|
||
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.
Comment 55•22 years ago
|
||
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.
Comment 56•22 years ago
|
||
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).
Comment 57•22 years ago
|
||
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.
Comment 58•22 years ago
|
||
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!
Comment 59•22 years ago
|
||
*** Bug 182647 has been marked as a duplicate of this bug. ***
Comment 60•22 years ago
|
||
*** Bug 182709 has been marked as a duplicate of this bug. ***
Comment 61•22 years ago
|
||
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.
Comment 62•22 years ago
|
||
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.
Comment 63•22 years ago
|
||
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.
Comment 64•22 years ago
|
||
> 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.
Comment 65•22 years ago
|
||
regarding broken links: that's already filed - bug 153765
Comment 66•22 years ago
|
||
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
Comment 67•22 years ago
|
||
*** Bug 184320 has been marked as a duplicate of this bug. ***
![]() |
||
Comment 68•22 years ago
|
||
*** Bug 128864 has been marked as a duplicate of this bug. ***
Comment 69•22 years ago
|
||
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.
Comment 70•22 years ago
|
||
*** Bug 198078 has been marked as a duplicate of this bug. ***
Comment 71•22 years ago
|
||
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?
Comment 72•22 years ago
|
||
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.
Comment 74•21 years ago
|
||
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!
Comment 75•21 years ago
|
||
*** Bug 211118 has been marked as a duplicate of this bug. ***
Comment 76•21 years ago
|
||
*** Bug 219804 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
Flags: blocking1.6a?
Keywords: mozilla1.3
![]() |
||
Comment 77•21 years ago
|
||
*** Bug 222805 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
Flags: blocking1.7a?
Updated•21 years ago
|
Flags: blocking1.7a? → blocking1.7a-
![]() |
||
Comment 78•21 years ago
|
||
*** Bug 236050 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
QA Contact: claudius → ian
Comment 79•21 years ago
|
||
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
Comment 80•21 years ago
|
||
Go fix this bug! ADOM!!!1! nt
Comment 81•21 years ago
|
||
re: Comment #79 So, you're volunteering to fix it, right?
Updated•21 years ago
|
Assignee: alecf → nobody
Status: ASSIGNED → NEW
QA Contact: ian → core.history.global
Comment 82•20 years ago
|
||
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).
Comment 83•20 years ago
|
||
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.
Comment 84•20 years ago
|
||
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.
Comment 85•20 years ago
|
||
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?
Updated•20 years ago
|
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
Updated•20 years ago
|
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
Updated•20 years ago
|
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
Comment 86•20 years ago
|
||
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
Comment 87•20 years ago
|
||
Can you now please stop changing the summary all the time? Thank you very much.
Comment 88•20 years ago
|
||
(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.
Comment 89•20 years ago
|
||
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
Comment 90•20 years ago
|
||
*** Bug 250515 has been marked as a duplicate of this bug. ***
Comment 91•20 years ago
|
||
This bug also stops the "visited" bit from feeding back to mail clients like Eudora. Is this the oldest unfixed bug ?
Comment 92•20 years ago
|
||
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).
Comment 93•20 years ago
|
||
*** Bug 224465 has been marked as a duplicate of this bug. ***
Comment 94•20 years ago
|
||
Related to bug 25189?
Comment 95•20 years ago
|
||
(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.
Comment 96•20 years ago
|
||
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.
Comment 97•20 years ago
|
||
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).
![]() |
||
Comment 98•20 years ago
|
||
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.
Comment 99•20 years ago
|
||
*** Bug 25189 has been marked as a duplicate of this bug. ***
Comment 100•20 years ago
|
||
(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.)
Comment 101•20 years ago
|
||
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.
![]() |
||
Comment 102•20 years ago
|
||
> (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.
Comment 103•20 years ago
|
||
(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.
Comment 104•20 years ago
|
||
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.
Comment 105•20 years ago
|
||
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.
Comment 106•20 years ago
|
||
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.
![]() |
||
Comment 107•20 years ago
|
||
> 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.
Comment 108•20 years ago
|
||
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.
Comment 109•20 years ago
|
||
(If we do this properly we'd also want to change :visited back to :link when a link expires from the history.)
Comment 110•20 years ago
|
||
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.
Comment 111•20 years ago
|
||
> 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.
Comment 112•20 years ago
|
||
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.
Comment 113•20 years ago
|
||
Is this working properly or not in the latest Netscape? Just wondering.
Comment 114•20 years ago
|
||
(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.
Comment 115•20 years ago
|
||
(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.
Comment 116•20 years ago
|
||
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.
Comment 117•20 years ago
|
||
*** Bug 287024 has been marked as a duplicate of this bug. ***
Comment 118•20 years ago
|
||
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?
Comment 119•19 years ago
|
||
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.
Comment 120•19 years ago
|
||
*** Bug 295524 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Flags: blocking1.8b4?
Comment 121•19 years ago
|
||
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.
Comment 122•19 years ago
|
||
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.
Comment 123•19 years ago
|
||
Why not?
Updated•19 years ago
|
Flags: blocking-aviary1.1?
*** Bug 169994 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 125•19 years ago
|
||
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.
Comment 126•19 years ago
|
||
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.)
Assignee | ||
Comment 127•19 years ago
|
||
I'm working on it right now.
Updated•19 years ago
|
Flags: blocking-aviary1.1?
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Assignee | ||
Comment 128•19 years ago
|
||
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 | ||
Updated•19 years ago
|
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #189340 -
Flags: superreview?(bzbarsky)
Attachment #189340 -
Flags: review?(bryner)
Assignee | ||
Comment 129•19 years ago
|
||
> 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.
![]() |
||
Comment 130•19 years ago
|
||
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.
Assignee | ||
Comment 131•19 years ago
|
||
(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.
![]() |
||
Comment 132•19 years ago
|
||
> 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*.
Comment 133•19 years ago
|
||
> 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".
Assignee | ||
Comment 134•19 years ago
|
||
(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.
Comment 135•19 years ago
|
||
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.
Comment 136•19 years ago
|
||
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)
Assignee | ||
Comment 137•19 years ago
|
||
(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.
Assignee | ||
Comment 138•19 years ago
|
||
(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.
Comment 139•19 years ago
|
||
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.
Comment 140•19 years ago
|
||
(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.
Comment 141•19 years ago
|
||
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]?
Assignee | ||
Comment 142•19 years ago
|
||
Yes, it fixes bug 293235. That's the main reason I've done it.
Blocks: 293235
Comment 143•19 years ago
|
||
(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 144•19 years ago
|
||
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 145•19 years ago
|
||
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?
Assignee | ||
Comment 146•19 years ago
|
||
(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).
Comment 147•19 years ago
|
||
Please do not forget to change the UUID for nsIDocument.
Comment 148•19 years ago
|
||
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+
Assignee | ||
Comment 149•19 years ago
|
||
(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").
Comment 150•19 years ago
|
||
(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.
Comment 151•19 years ago
|
||
(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.
Assignee | ||
Comment 152•19 years ago
|
||
(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.
Comment 153•19 years ago
|
||
(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.
Assignee | ||
Comment 154•19 years ago
|
||
> 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?
Comment 155•19 years ago
|
||
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.
Assignee | ||
Comment 156•19 years ago
|
||
Simon, the attached patch patches Camino
Assignee | ||
Updated•19 years ago
|
Attachment #189340 -
Flags: superreview?(bzbarsky)
Assignee | ||
Comment 157•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #189340 -
Attachment is obsolete: true
Attachment #189960 -
Flags: superreview?(bzbarsky)
Attachment #189960 -
Flags: review?(bryner)
Updated•19 years ago
|
Attachment #189960 -
Flags: review?(bryner) → review+
Assignee | ||
Comment 158•19 years ago
|
||
Sorry, some changes to nsAppRunner.cpp sneaked in there, they're not part of this patch.
Updated•19 years ago
|
Whiteboard: parity-opera → parity-opera [has patch, has review, needs superreview (bzbarsky)]
*** Bug 301846 has been marked as a duplicate of this bug. ***
Comment 160•19 years ago
|
||
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.
Comment 161•19 years ago
|
||
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/.
Assignee | ||
Comment 162•19 years ago
|
||
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.
Assignee | ||
Comment 163•19 years ago
|
||
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 164•19 years ago
|
||
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.
Assignee | ||
Comment 165•19 years ago
|
||
Will do.
![]() |
||
Comment 166•19 years ago
|
||
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?
Assignee | ||
Comment 167•19 years ago
|
||
(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.
Assignee | ||
Comment 168•19 years ago
|
||
Boris, if you're going to read the code maybe you should just sr it while you're there :-)
Assignee | ||
Comment 169•19 years ago
|
||
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+
![]() |
||
Comment 170•19 years ago
|
||
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.
Comment 171•19 years ago
|
||
(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.
Assignee | ||
Comment 172•19 years ago
|
||
(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.
Comment 173•19 years ago
|
||
Visiting enough links that the hashtable might shrink seems like an edge case.
Assignee | ||
Comment 174•19 years ago
|
||
Trying to avoid one hashtable lookup here is also rather hair-splitting, to me, but I'm happy to call RawRemoveEntry if you wish.
Assignee | ||
Updated•19 years ago
|
Attachment #190500 -
Flags: superreview?(bugmail)
Assignee | ||
Updated•19 years ago
|
Attachment #189960 -
Flags: superreview?(bzbarsky)
Assignee | ||
Updated•19 years ago
|
Attachment #190648 -
Flags: superreview?(bugmail)
Assignee | ||
Comment 175•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #190648 -
Attachment is obsolete: true
Attachment #190774 -
Flags: superreview?(bzbarsky)
Attachment #190774 -
Flags: review+
![]() |
||
Comment 176•19 years ago
|
||
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) ... :(
Comment 177•19 years ago
|
||
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!)
Comment 178•19 years ago
|
||
+ 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 179•19 years ago
|
||
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;
Assignee | ||
Updated•19 years ago
|
Attachment #190774 -
Flags: superreview?(bzbarsky) → superreview?(dbaron)
Assignee | ||
Comment 180•19 years ago
|
||
(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.
Comment 181•19 years ago
|
||
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.
Comment 182•19 years ago
|
||
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.
Comment 183•19 years ago
|
||
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?)
Assignee | ||
Comment 184•19 years ago
|
||
(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.
![]() |
||
Comment 185•19 years ago
|
||
> 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.
Assignee | ||
Comment 186•19 years ago
|
||
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 187•19 years ago
|
||
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+
Comment 188•19 years ago
|
||
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).
Assignee | ||
Comment 189•19 years ago
|
||
Comment on attachment 191928 [details] [diff] [review] updated patch #6 Need approval for this 1.8b4 blocker
Attachment #191928 -
Flags: approval1.8b4?
Assignee | ||
Updated•19 years ago
|
Attachment #190774 -
Flags: superreview?(dbaron)
Comment 190•19 years ago
|
||
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+
Assignee | ||
Comment 191•19 years ago
|
||
checked in. I made the mVisible check in UpdateLinkMap an assertion as well as an early exit.
Assignee | ||
Comment 192•19 years ago
|
||
marking fixed
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 193•19 years ago
|
||
Thank you. Perfect.
Assignee | ||
Comment 194•19 years ago
|
||
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.
Assignee | ||
Comment 195•19 years ago
|
||
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 196•19 years ago
|
||
Comment 197•19 years ago
|
||
Comment on attachment 192324 [details] [diff] [review] don't leak all of the pres shells r=jst
Attachment #192324 -
Flags: review+
Assignee | ||
Comment 198•19 years ago
|
||
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!
Assignee | ||
Comment 199•19 years ago
|
||
I guess not. "Doh!"
Assignee | ||
Comment 200•19 years ago
|
||
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.
Assignee | ||
Comment 201•19 years ago
|
||
I've undone the backout mentioned in comment #194, so this should be fully functional on trunk now.
Comment 202•19 years ago
|
||
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 203•19 years ago
|
||
Comment on attachment 192324 [details] [diff] [review] don't leak all of the pres shells r=jst
Attachment #192324 -
Flags: review+
Assignee | ||
Comment 204•19 years ago
|
||
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.
Assignee | ||
Comment 205•19 years ago
|
||
(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.
![]() |
||
Comment 206•19 years ago
|
||
(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...
Assignee | ||
Comment 207•19 years ago
|
||
(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?
Comment 209•19 years ago
|
||
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.
Blocks: 304434
Assignee | ||
Comment 211•19 years ago
|
||
(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.
Comment 212•19 years ago
|
||
(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.
Comment 213•19 years ago
|
||
(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.
Comment 214•19 years ago
|
||
(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.?
Comment 215•19 years ago
|
||
(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?
Comment 216•19 years ago
|
||
(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..
Assignee | ||
Comment 217•19 years ago
|
||
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.
Comment 218•19 years ago
|
||
(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. :-(
Comment 219•19 years ago
|
||
(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.
Comment 220•19 years ago
|
||
(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
Comment 221•19 years ago
|
||
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.
Comment 222•19 years ago
|
||
And in any case, to repeat what roc said, THAT DISCUSSION DOES NOT BELONG ON THIS BUG.
![]() |
||
Comment 223•19 years ago
|
||
Bug 304759 has addressed the Tp fallout from this, looks like.
Comment 224•19 years ago
|
||
*** Bug 260881 has been marked as a duplicate of this bug. ***
Comment 225•19 years ago
|
||
*** Bug 249890 has been marked as a duplicate of this bug. ***
Comment 226•19 years ago
|
||
Filed bug 305573 as a regression.
Updated•19 years ago
|
Keywords: helpwanted
Comment 227•19 years ago
|
||
If this is fixed on the branch (it's got an associated bonsai comment), please add the fixed1.8 keyword. Thanks.
Comment 228•19 years ago
|
||
*** Bug 307002 has been marked as a duplicate of this bug. ***
Comment 229•19 years ago
|
||
*** Bug 313779 has been marked as a duplicate of this bug. ***
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•