320831 - Expose visit ID in result nodes.

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect, P3)

Description

[Brett Wilson]

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Looks like this is fixed. Correct me if I'm wrong. 

Comment 2

[Brett Wilson]

Not fixed. The interface is there, but RESULTS_AS_FULL_VISIT is not implemented yet. Should be easy, though.

Comment 4

[AndrewM]

Is this still targeted for Firefox 3?

Comment 5

[Dietrich Ayala (:dietrich)]

Drivers: We should consider blocking on this, so that extensions can utilize it for walking visit trails.

Comment 6

[Jesse Ruderman]

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]

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]

> 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]

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]]

This will not block the final release of Firefox 3.

Comment 11

[Ash]

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]

> 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]

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]

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]

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

javascript:void(location=document.referrer);

Comment 16

[Casper Gripenberg]

Great, but it doesn't work with bookmarks.

Comment 17

[bomfog]

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]

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)]

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]

Attachment: 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]

> 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]

Attachment: 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]]

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]

Because of comment 23, let's target this for post 3.1.

Comment 25

[Dietrich Ayala (:dietrich)]

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)]

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]

(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]

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]]

(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]

Attachment: 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]

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]]

(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]]

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.