425563 - Clicked links not colored as visited, or visited color forgotten after hard refresh

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032523 Minefield/3.0b5pre ID:2008032523

1. New profile, start firefox
2. Visit http://tinderbox.mozilla.org/showbuilds.cgi?tree=
3. In the 'guilty' column, left click on a name. A translucent popup appears
4. Left click on some white-space in the main page to dismiss the popup. Note the name you clicked on has changed colour to indicate it has been visited
5. Press CTRL+R

Expected:
 - Page reloads from web server, but the name you clicked remains coloured as per a visited link

Actual:
 - Page reloads but the name you clicked is not coloured to indicate you've clicked it previously.

Works: 20080326_2045_firefox-3.0pre.en-US.win32
Broken: 20080326_2223_firefox-3.0pre.en-US.win32

Checkins to module PhoenixTinderbox between 2008-03-26 20:45 and 2008-03-26 22:22 :
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1206589500&maxdate=1206595379

Bug 423514? Bug 416313? CC'ng Jesse and Mak77.

(I also see a similar problem on http://hourly-archive.localgho.st/ when right-clicking on one of the .zip files and choosing Save As. Most of the time the file starts downloading, but the link isn't coloured appropriately.)

Comment 1

[Marco Bonardo [:mak]]

(In reply to comment #0)
> (I also see a similar problem on http://hourly-archive.localgho.st/ when
> right-clicking on one of the .zip files and choosing Save As. Most of the time
> the file starts downloading, but the link isn't coloured appropriately.)

this is normal since downloads are not considered in visit_count

Comment 2

[Marco Bonardo [:mak]]

the first is a popup with a dhtml content, so again i doubt it is considered a visit by the current system... previously visit_count was bogus, while now it counts real visits.
link coloring is based on that

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

There is something wrong with the concept of marking 'read' linked to visit count.

1. Go here: http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox
2. Find a checkin and in the 'Guilty' column click the Name
3. Note you get the dhtml pop-up you refer to with 4 links...Click any link to 'visit' then when done click 'back' note that the link is marked 'read' 
4. click outside the box to dismiss and note that the Name is marked 'read'
5. Now refresh the page and note the Name is not marked 'read' anymore 
6. click the Name and note that the pop-up still has the link marked "read"

The Guilty column is the only column losing the 'read' marker.  If you click the time of the checkin it is marked 'read' and a refresh does not clear the 'read' marker... so something is not right IMO, its been working this way for as long as I've been working with Firefox/Minefield and to remove the 'read' marker makes looking for checkins from the last time you looked, i.e. relying on the 'read' name the last time you looked, to find new checkins is counter-productive as to where I last looked...now I can't find it...

Same argument goes for downloads, not marking them as 'read' is counter-productive, how are we to tell if we have already downloaded a certain file in a long list of files ?  

Marking as 'read' has never cared about visit count before AFAIK since its something new with 'Places', so IMO the 'visit count' should not be making the determinination if the item has been visited.  Any click on anything is 'visited', you clicked it... mark it so!

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch

we could replace the query with

SELECT h.id FROM moz_places h WHERE h.url ?1 
AND EXISTS
(SELECT id FROM moz_historyvisits WHERE place_id = h.id LIMIT 1)
;

from my testing the actual query speed is about the same
SELECT id, visit_count, typed, hidden FROM moz_places WHERE url = ?1
;

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch

forgot an header

this will check for every transition type, should be ok also for transition_embed since we are asking if an uristring has been already visited, so also an embed visit could be valid here

Comment 6

[Dietrich Ayala (:dietrich)]

requesting in-litmus?, we need regression tests for link coloring for each of the transition types.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 312259 [details] [diff] [review]
patch

looks fine, r=me.

Comment 8

[Reed Loden [:reed]]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.283; previous revision: 1.282
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.154; previous revision: 1.153
done

Comment 9

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008032900 Minefield/3.0pre ID:2008032900
--> VERIFIED

Thanks Marco!

Comment 10

[Dietrich Ayala (:dietrich)]

this is easily tested via xpcshell, and should've been caught in tests for bug 416313.. marco, please add an automated test for this fix.

Comment 11

[Marco Bonardo [:mak]]

Attachment: unit test

here is the unit test, check that all transitions are marked as visited, andthat visit_count does not count embed and download

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 312874 [details] [diff] [review]
unit test

r=me, thanks!

Comment 13

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/tests/unit/test_425563.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_425563.js,v  <--  test_425563.js
initial revision: 1.1
done

Comment 14

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv