Last Comment Bug 320831 - Expose visit ID in result nodes.
: Expose visit ID in result nodes.
Status: RESOLVED WONTFIX
:
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: P3 normal with 9 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
: 392281 (view as bug list)
Depends on: 323492
Blocks: 260405 318653 382187 392844 554414
  Show dependency treegraph
 
Reported: 2005-12-19 10:31 PST by Brett Wilson
Modified: 2013-02-11 14:30 PST (History)
24 users (show)
dietrich: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for unit tests to use RESULTS_AS_FULL_VISIT (2.37 KB, patch)
2008-10-30 03:33 PDT, David Adam
no flags Details | Diff | Splinter Review
Implement support for RESULTS_AS_FULL_VISIT with tests (12.53 KB, patch)
2008-11-04 02:01 PST, David Adam
no flags Details | Diff | Splinter Review
sql to construct visit path (3.64 KB, text/x-sql)
2010-01-12 08:57 PST, James Andrewartha
no flags Details

Description Brett Wilson 2005-12-19 10:31:12 PST
 
Comment 1 Ben Goodger (use ben at mozilla dot org for email) 2006-01-30 09:40:57 PST
Looks like this is fixed. Correct me if I'm wrong. 
Comment 2 Brett Wilson 2006-01-30 10:10:24 PST
Not fixed. The interface is there, but RESULTS_AS_FULL_VISIT is not implemented yet. Should be easy, though.
Comment 3 Phil Ringnalda (:philor) 2007-08-21 18:11:10 PDT
*** Bug 392281 has been marked as a duplicate of this bug. ***
Comment 4 AndrewM 2007-11-28 21:20:17 PST
Is this still targeted for Firefox 3?
Comment 5 Dietrich Ayala (:dietrich) 2008-03-17 16:40:21 PDT
Drivers: We should consider blocking on this, so that extensions can utilize it for walking visit trails.
Comment 6 Jesse Ruderman 2008-03-17 17:54:53 PDT
For example, https://www.squarefree.com/extensions/high/ needs this API in order to be ported to trunk.  It has 3000 daily active users according to preview.addons.mozilla.org.
Comment 7 Brett Wilson 2008-03-17 22:58:49 PDT
Note that the visit trails are fairly poor quality. There are a number of situations where the trail will get "lost" and so an extension author wanting to count on this might be disappointed. However, it should be at least as reliable as the former "referrer" field in history, so maybe it's good enough for users like this.

Not that it's also somewhat non-trivial to use these IDs since you can't query for them, so you would have to suck down all visits for a period of history and search for them manually. It should be easy and low-risk to take if somebody wants, but I wouldn't block on it.
Comment 8 Jesse Ruderman 2008-03-17 23:10:22 PDT
> Note that the visit trails are fairly poor quality. There are a number of
> situations where the trail will get "lost" and so an extension author wanting
> to count on this might be disappointed.

Can you elaborate?  I might want to document this as part of the extension's web page.

> Not that it's also somewhat non-trivial to use these IDs since you can't query
> for them, so you would have to suck down all visits for a period of history and
> search for them manually.

My extension wants to query by URI, so I don't think I'd have that problem.
Comment 9 Brett Wilson 2008-03-17 23:34:24 PDT
I recall (a bit misty now) that it keys off of the HTTP referrer, so any time you don't have that like http->https and certain JavaScript stuff.

We used to have lines in the history view that separated contiguous trails, and there would often be superfluous lines. I think in addition to the issues mentioned above, there may also be other problems. Nobody has used this information for anything before, so it's difficult to tell.

Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-20 08:09:31 PDT
This will not block the final release of Firefox 3.
Comment 11 Ash 2008-04-04 05:30:36 PDT
Brett:
> I recall (a bit misty now) that it keys off of the HTTP referrer, so any time
> you don't have that like http->https and certain JavaScript stuff.

Why not improve this to do it based on actual page request source to get better trails? What's required to do this (some rough pointers, please)?

Mike:
> This will not block the final release of Firefox 3.

Is there any chance someone will implement the RESULTS_AS_FULL_VISIT before FF3 release? (If someone does agree to do this soon, will it be approved in time?)

The fact that the trails are of poor quality for now can possibly be fixed in the future to improve user experience for those extensions that use the new places API - as long as the API is fully implemented *now* to reveal referringId (and the other stuff in RESULTS_AS_FULL_VISIT).

Without this, the whole move to Places will have little impact on extension developers.
Comment 12 Brett Wilson 2008-04-04 06:33:29 PDT
> Why not improve this to do it based on actual page request source to get better
> trails?

I don't know how to do this. It would require changing some scary internals like the docshell.
Comment 13 John Abbe 2008-06-23 10:15:26 PDT
The "How'd I Get Here?" extension depends on this (it's a button you click on to go to the first web page from which you go to the page you're on). Also required would be https://bugzilla.mozilla.org/show_bug.cgi?id=392844

https://www.squarefree.com/extensions/high/
https://addons.mozilla.org/en-US/firefox/addon/1196
Comment 14 Casper Gripenberg 2008-06-26 03:43:00 PDT
I do not think the quality of results is as important as just getting SOME sort of trail information.

I find the "How'd I Get Here" is one of my most frequently used extensions. It's invaluable when opening multiple links in new tabs, as you can go to the tab and still get back to where you came from in that new tab. Something you cannot do otherwise (the back button is grayed out on new pages opened in tabs). 

And not only that, but it works from pages opened through bookmarks too. Why did I bookmark this page? From where did I find it? Click on the "How'd I Get Here" and you have your answer.

Is there any way to get this API implemented? I'd take low-quality trails any day, compared to no trails.

Comment 15 Jesse Ruderman 2008-06-26 17:02:31 PDT
You don't need an extension for that use case.  A bookmarklet suffices:

javascript:void(location=document.referrer);
Comment 16 Casper Gripenberg 2008-06-26 17:10:45 PDT
Great, but it doesn't work with bookmarks.
Comment 17 bomfog 2008-06-30 10:03:00 PDT
Another addon work-around that might partially satisfy Casper's case is Tab History https://addons.mozilla.org/en-US/firefox/addon/1859
Comment 18 David Adam 2008-10-29 06:57:27 PDT
Is this still targeted for 3.1? If there's no-one actively working on it, and it is the sort of thing that an external contributor could take a crack at, would it be possible to get a quick outline of what needs to change to fully support RESULTS_AS_FULL_VISIT?
Comment 19 Dietrich Ayala (:dietrich) 2008-10-29 09:47:05 PDT
Hi David,

This bug does not currently have an owner, so you're welcome to it. It'd be great to get this in 3.1.

* here's the interface: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/public/nsINavHistoryService.idl#209

* add support in the query builder: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#3086

* create individual result nodes: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#6343

* write tests (example: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_history.js#103)

probably a few other places, but that should be enough to get started.
Comment 20 David Adam 2008-10-30 03:33:32 PDT
Created attachment 345468 [details] [diff] [review]
Patch for unit tests to use RESULTS_AS_FULL_VISIT

How does this look for the unit tests?

I've switched the first tests to check for RESULTS_AS_FULL_VISIT, and added a new test to check for referrer visit ID.

The comment "using full visit crashes in xpcshell" seems to no longer be true, so the first tests pass ok. Obviously the referrer check will fail until the interface is implemented properly.

It might also be worth adding a unit test to check if the referring visit ID contains the correct URI but I'm not quite sure how to look up a specific visit ID in the Places JS API yet.
Comment 21 David Adam 2008-10-30 21:30:06 PDT
> It might also be worth adding a unit test to check if the referring visit ID
> contains the correct URI but I'm not quite sure how to look up a specific visit
> ID in the Places JS API yet.

I understand now that comment #7 implies that this is not yet possible.
Comment 22 David Adam 2008-11-04 02:01:07 PST
Created attachment 346233 [details] [diff] [review]
Implement support for RESULTS_AS_FULL_VISIT with tests

My first attempt at fixing this bug. Note that I am not particularly familiar with the Mozilla source tree, so this involves some fairly cargo-cult duplication of existing code.
Comment 23 Marco Bonardo [::mak] 2008-11-04 07:10:10 PST
some brainstorming: i guess if it would not be better waiting to have a referrer column in history table and return the referrer uri instead of an ever-changing visit id, especially now that we have temp tables and visit ids are spread across the two tables. if the scope for exposing the visit id is getting the referrer uri and the from visit uri why not directly returning uris?
Comment 24 Shawn Wilsher :sdwilsh 2008-12-09 13:58:25 PST
Because of comment 23, let's target this for post 3.1.
Comment 25 Dietrich Ayala (:dietrich) 2009-06-09 14:18:07 PDT
yes, comment #23 is spot on. we don't need to wait for a schema change though. could change it to use URI regardless.
Comment 26 Dietrich Ayala (:dietrich) 2009-06-09 14:49:25 PDT
Comment on attachment 346233 [details] [diff] [review]
Implement support for RESULTS_AS_FULL_VISIT with tests

canceling review. definitely most of the way there. should refactor to use URL instead of visit id.
Comment 27 James Andrewartha 2009-10-28 07:34:40 PDT
(In reply to comment #24)
> Because of comment 23, let's target this for post 3.1.

So 3.6 has branched, any chance of this happening for 3.7?
Comment 28 David Adam 2009-10-28 08:39:50 PDT
My understanding at this point is that visit IDs are no longer a reliable value, so making this work would require:
 - updating the schema to use a URI instead of a place ID (although comment #25 says we don't need this? I'm not sure how)
 - updating nsNavHistory::AddVisit and InternalAddVisit to support URIs instead of place IDs
 - rewriting patch to unbitrot and use URIs instead of visit IDs
 - updating the API documentation - RESULTS_AS_FULL_VISIT has never been implemented properly so changes shouldn't break things?

This is beyond my ken, unfortunately.
Comment 29 Marco Bonardo [::mak] 2009-10-28 08:46:51 PDT
(In reply to comment #28)
>  - updating the API documentation - RESULTS_AS_FULL_VISIT has never been
> implemented properly so changes shouldn't break things?

the plan is to merge FULL_VISIT in VISIT results, providing missing infos and getting rid of a useless result type
Comment 30 James Andrewartha 2010-01-12 08:57:57 PST
Created attachment 421273 [details]
sql to construct visit path

The author of https://addons.mozilla.org/en-US/firefox/addon/14650 has taken David's patch and used it to construct the visit path. I've extracted the relevant part from voyage-0.5-fx/modules/HistoryReader.jsm
Comment 31 Allison Naaktgeboren :ally 2012-09-14 15:28:09 PDT
Since this is the reason for ln 154 of services/sync/modules/engines/history.js, I'm curious what happened to this. 

:mak, did FULL_VISIT ever get merged into VISIT results?
Comment 32 Marco Bonardo [::mak] 2012-09-18 09:41:35 PDT
(In reply to :Ally Naaktgeboren from comment #31)
> :mak, did FULL_VISIT ever get merged into VISIT results?

not yet, still to do
Comment 33 Marco Bonardo [::mak] 2013-02-11 14:30:15 PST
we are not going to expose visit ids, if there's request to get a "visit path", would be better to ask for an API to get it.
Also, FULL_VISIT nodes have been removed.

Note You need to log in before you can comment on or make changes to this bug.