256077 - seamonkey could use bookmarks aggregation port from firefox

Status: RESOLVED WONTFIX

(SeaMonkey :: Bookmarks & History, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

seamonkey could probably use a port of bug 235665's firefox only bits, to take
advantage of the aggregation features.  Note that for now the implementation of
nsForwardProxyDataSource lives in the firefox browser/ tree; this can eventually
be either moved in to rdf/ for sharing, or similar.

Comment 1

[Jan Varga [:janv]]

Sounds good to me.

Comment 2

[Ray Booysen]

Reassigning as per Bug #32644

Comment 3

[[e-mail]]

Attachment: WIP1

I got this far, and some of it works - the builder now identifies find: URLs as containers, for instance - but it doesn't actually build their contents...

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 250106 [details] [diff] [review]
WIP1

I don't think I'm the right person to review this any more, sorry (no time to re-acquaint myself with the code either).

Comment 5

[Neil Deakin]

Comment on attachment 250106 [details] [diff] [review]
WIP1

>+nsBookmarksInferDataSource::GetTargets(nsIRDFResource *aSource, nsIRDFResource *aProperty, PRBool 
aTruthValue, nsISimpleEnumerator **aResult)
...
>+  nsCOMPtr<nsIRDFResource> proxy = GetProxyResource(aSource);
>+  if (proxy) {
>+    rv = mInner->GetTargets(proxy, aProperty, aTruthValue, getter_AddRefs(innerEnumerator));

This should be proxyEnumerator I think.


>+NS_IMETHODIMP
>+nsBookmarksInferDataSource::ArcLabelsOut(nsIRDFResource *aSource, nsISimpleEnumerator **aResult)
...
>+    rv = mInner->ArcLabelsOut(proxy, getter_AddRefs(innerEnumerator));

Same here

Is this change something that can't just be built into the bookmarks datasource?

Comment 6

[[e-mail]]

(In reply to comment #5)
>This should be proxyEnumerator I think.
Doh! Trying...

>Is this change something that can't just be built into the bookmarks datasource?
I thought long and hard about that, and managed to convince myself that it wouldn't composite properly. But if this works then I'll try that too.

Comment 7

[[e-mail]]

(In reply to comment #5)
>Is this change something that can't just be built into the bookmarks datasource?
I remember now.

The idea is that when the builder asks for the target(s), you need to check both the target(s) of the builder's resource and those of the bookmarked url. The bookmarks service can't do this check because it doesn't have access to the composite datasource that the builder is using.

Comment 8

[[e-mail]]

(In reply to comment #5)
>This should be proxyEnumerator I think.
OK, so this almost works, but unfortunately the trunk tree builder won't permit duplicate paths to the same resource any more, so saved search bookmarks only "finds" bookmarks that aren't already showing in the tree (and those bookmarks won't then be found in their real parent container...)

Does the template builder have the same "problem", or is there a workaround?

Comment 9

[Neil Deakin]

Assuming you meant the same resource appearing in different parts of the hierarchy, the content builder supports this, but the tree builder does not. I wasn't aware that the old one did very well at it either.

Comment 10

[[e-mail]]

Attachment: Branch patch

SmartFind queries work in the bookmarks manager on the branch :-)

If you don't have an old enough profile to have some, you can create some new SmartFind queries by searching bookmarks and clicking Save as Bookmark.

You can also create a bookmark to a filesystem folder and it will expand in the navigator window (but not in the bookmarks manager).

Changes from the trunk patch are:
1) proxyResource variable fixes
2) module registration moved around
3) ftp: excluded (done by httpindex, which we don't aggregate)
4) internetsearch: spelled correctly
5) browser changes included

Comment 11

[jag (Peter Annema)]

Comment on attachment 250756 [details] [diff] [review]
Branch patch

>Index: components/bookmarks/src/nsBookmarksService.h
>===================================================================

>+class nsBookmarksInferDataSource : public nsIRDFInferDataSource
>+{
...
>+private:
...
>+  nsCOMPtr<nsIRDFDataSource> mRDFService;

It doesn't look like you're using mRDFService anywhere.

r=jag

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 250756 [details] [diff] [review]
Branch patch

clearly I'm not going to get to this. also 
"branch patch" sounds like it may be outdated now...

Comment 13

[Robert Kaiser]

Neil, this is still assigned to you, but it looks outdated. Isn't it going to be fixed by bug 498596, which moves us to places bookmarks, anyhow?

Comment 14

[[e-mail]]

I don't know whether Places supports the feature, but given that this is a feature that got broken way back when in the Mozilla era, and attachment 250756 [details] [diff] [review] barely gets SeaMonkey 1.1 up to parity with Firefox 2, and Enn broke SeaMonkey 2.0 again anyway, there's not really much more that can be done here.