Closed Bug 320831 Opened 19 years ago Closed 11 years ago

Expose visit ID in result nodes.

Categories

(Toolkit :: Places, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: brettw, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

 
Priority: -- → P2
Blocks: 318653
Depends on: 323492
Looks like this is fixed. Correct me if I'm wrong. 
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Not fixed. The interface is there, but RESULTS_AS_FULL_VISIT is not implemented yet. Should be easy, though.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: --- → Firefox 2 beta1
Priority: P3 → P4
Target Milestone: Firefox 2 beta1 → Firefox 3
Assignee: brettw → nobody
Status: REOPENED → NEW
Is this still targeted for Firefox 3?
Drivers: We should consider blocking on this, so that extensions can utilize it for walking visit trails.
Flags: blocking-firefox3?
Priority: P4 → P3
Blocks: 392844
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.
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.
> 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.
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.

This will not block the final release of Firefox 3.
Flags: blocking-firefox3? → blocking-firefox3-
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.
> 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.
Blocks: 437245
Target Milestone: Firefox 3 → Firefox 3.1
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
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.

You don't need an extension for that use case.  A bookmarklet suffices:

javascript:void(location=document.referrer);
Great, but it doesn't work with bookmarks.
Another addon work-around that might partially satisfy Casper's case is Tab History https://addons.mozilla.org/en-US/firefox/addon/1859
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?
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.
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.
> 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.
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.
Attachment #345468 - Attachment is obsolete: true
Attachment #346233 - Flags: review?
Attachment #346233 - Flags: review? → review?(dietrich)
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?
Because of comment 23, let's target this for post 3.1.
Assignee: nobody → zanchey
Status: NEW → ASSIGNED
Flags: blocking-firefox3-
Product: Firefox → Toolkit
QA Contact: places → places
Target Milestone: Firefox 3.1 → mozilla1.9.2a1
Flags: blocking1.9.2?
Blocks: 382187
yes, comment #23 is spot on. we don't need to wait for a schema change though. could change it to use URI regardless.
No longer blocks: 437245
Flags: blocking1.9.2? → wanted1.9.2+
Target Milestone: mozilla1.9.2a1 → ---
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.
Attachment #346233 - Flags: review?(dietrich)
Blocks: 260405
(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?
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.
Assignee: zanchey → nobody
Status: ASSIGNED → NEW
(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
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
Blocks: 554414
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?
(In reply to :Ally Naaktgeboren from comment #31)
> :mak, did FULL_VISIT ever get merged into VISIT results?

not yet, still to do
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.
Status: NEW → RESOLVED
Closed: 18 years ago11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.