Closed Bug 235665 Opened 20 years ago Closed 18 years ago

Aggregation Broken in Bookmarks

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox1.0

People

(Reporter: bugs, Assigned: vlad)

References

Details

(Keywords: fixed-aviary1.0, Whiteboard: [in-aviary])

Attachments

(4 files, 12 obsolete files)

IRC log (shaver, pike, bsmedberg) about this and some other RDF issues
7.25 KB, text/plain
Details
comments on forward-proxy patch
36.06 KB, text/plain
Details
Final patch: Firefox portion
64.06 KB, patch
Details | Diff | Splinter Review
235665-trunk-gecko-inference.patch
18.86 KB, patch
shaver
: superreview+
Details | Diff | Splinter Review 
Bookmarks are now stored with unique resource IDs... this has broken aggregation. 

Aggregation is when a bookmark has an ID that Bookmarks doesn't recognize as a
container but other datasources do. An example is saved search queries. To the
Bookmarks datasource these are just bookmarks with funky URLs that can't be
opened, but when the Internet Search DataSource is aggregated onto the bookmarks
UI elements' composite DB, the template builder starts showing the item as a
container, since to the Internet Search DataSource it IS a container. 

Aggregation is essential to our extension strategy for 1.0 and future adventures
in web metadata that we want to pursue after 1.0.. we need this to be fixed. I
understand unique IDs were given to allow two bookmarks to exist with the same
URI. The fix is to introduce another property to UI elements that has a
potentially non-unique value (the URI) that the template builder knows to check
BEFORE checking the ID attribute with the composite.
targeting.
Priority: -- → P1
Target Milestone: --- → Firefox1.0beta
Flags: blocking1.0+
Priority: P1 → P2
Target Milestone: Firefox1.0beta → Firefox1.0
First cut at a patch for this.	I created a "AggregateDataSource", which has a
child DataSource and a RDF resource.  For any data-getting requests (e.g.,
GetTarget, HasAssertion, etc., but not Assert/Unassert), it first checks if the
operation's source has an arc with the given resource as the property; if it
does, it takes that arc's target, and tries the query against it first.

I added a XUL aggregationproperty="" tag, which would let you specify which
property would be used.  If such a tag is present, the template builder will
create an AggregationDataSource (is there a better name for this?), and set its
child data source to be the usual CompositeDS; otherwise it will just use the
CompositeDS as normal.

This works well for me in bookmarks; I'll attach a patch later on that shows
some of this in action, as well as a patch for the bookmarks service to create
an aggregation target with the URL/URI of each bookmark or similar.
Assignee: p_ch → vladimir
Status: NEW → ASSIGNED
Attached patch bookmarks bits for aggregation (obsolete) — — Splinter Review 
The relevant bits for bookmarks aggregation.  Create a new bookmark for a
file:// directory url, and you'll get the nice submenus and the like.  Note
that since there's no aggregationproperty set for, say, the bookmark manager's
template, it gets no aggregation, and you can deal with the file bookmarks like
a normal bookmark.

There's some issues; for example, the file DS has a "Name" arc.  This is
overriding whatever the user puts in as the bookmark name (because it's
checking the aggregation first).  I guess I could add a list of properties to
exclude from aggregation, but that might just be bloat.

Patch might not apply cleanly -- it's diffed against a bookmarksservice that
has my RSS bits in it.
Note you won't be able to do anything with those aggregated bookmarks without
this ugly hack of a patch:

--- foo	2004-06-19 19:57:03.378320984 +0200
+++ bookmarks.js	2004-06-19 19:57:05.198044344 +0200
@@ -1099,6 +1099,11 @@
         type = "PersonalToolbarFolder";
     }
 
+    if (type == "") {
+        // XXXvladimir NO NO NO UGLY HACK FIXME DIE ACK!
+        return "Bookmark";
+    }
+
     return type;
   },
 

Which, surprisingly, causes no ill effects that I can see; so this might work as
a temporary band-aid until a potential overhaul of the whole thing.  RSS
integration from bug 244078 should probably get implemented in terms of this,
with a RSSDataSource as well.
I need to do some thinking about attachment 151205 [details] [diff] [review] in the light of RDF reification.
http://www.w3.org/TR/rdf-primer/#reification
Comment on attachment 151205 [details] [diff] [review]
nsIRDFAggregateDataSource and associated template builder bits

Axel: can you review this when you figure out the reification story?
Attachment #151205 - Flags: review?(axel)
So _I_ think that reification is not what we want here.  This aggregation fix is
a way to provide a "shadow" datasource that lets us take an alterate view of,
f.e., the bookmarks datasource suitable for aggregation with other datasources
that name want to deal with a property as "identity", rather than attribute.

I think we probably don't want to call it AggregationDataSource.  It's almost a
"MappedDataSource" or some such -- it could well be useful for more than just
aggregation.

(If it's not, then just use the same aggregation property everywhere rather than
letting it be specified via a property, perhaps?)

Please excuse my sloppy RDF terminology.  I just read through a bunch of
reification stuff, and my brain is in agony.


I am considerably confused by the description of this in comments #1 and #2, and
shaver's representation. Is the problem merely determining which "resources" are
containers and which aren't? Or is this a generic way of mapping from one data
model to another?

Before this is even close to ready for review, it need to be specified about 10
times better.
It's a generic way of tying multiple data models together, where one data model
knows assertions about a resource based on an "alias" or "alternate name" for it.

This implementation is essentially what Ben suggested in comment
#1, but instead of implementing it in the template builder (which
would have been very messy), I implemented it as a
filter/overlay/whateveryouwanttocallit data source (yeah,
AggregateDataSource is a bad name).  Essentially, it lets you
aggregate multiple data sources together where they can give you
different information on a subject based on different names for that subject
(which are the targets of a particular specified resource(s?), the "aggregation
property").

So, if you have:

A -> prop1 -> B
A -> agg -> C

and some other data source with:

C -> prop2 -> D
C -> prop3 -> E

if you stick an AggregationDataSource in front of a
composite ds with the above the above two DS's, and set the
aggregationproperty to "agg", it will behave as if the graph looks
like this:

A -> prop1 -> B
A -> agg -> C 
A -> prop2 -> D
A -> prop3 -> E
(along with the two original arcs with C as the subject)

The algorithm for this is simple; for every operation that gets
information, it first checks if there is an aggregation property (agg)
arc, and if so, it checks if the target of that arc has the requested
resource data.  If it does, then it either returns it, or if an
enumerator is requested (i.e. GetTargets), it will return an
Enumerator that is the combination of both.

To use a concrete example, a bookmark is now a set of arcs off an
anonymous rdf resource.  The files data source knows about resources
that have the form of a file:// URI.  So, if you have:

bookmarks ds:
rdf:$anon -> NC:URL -> file:///home/vladimir/

files ds:
file:///home/vladimir -> rdf:instanceOf -> rdf:Seq
...

If you stick both those in a composite DS, and add an AggregationDS in
front of it with an aggregation property of "NC:URL", you'll have a
rdf:$anon that looks like a RDF container, because of the arcs that
the files ds will dynamically generate.

Being a RDF neophyte, I'm not sure how reification would help us here, though I
would welcome enlightenment. :)
oops, guessed wrong at the name I was looking for. I was looking for "inference"
and not "reification". Sorry for being sloppy.
http://www.w3.org/TR/2004/REC-owl-features-20040210/#sameAs sounds like it.
WARNING, I haven't read that spec at all and I crashed a car yesterday, so you may
not want to go where I go.

But we have folks that would like to see OWL support in mozilla, if we start
inventing stuff that could be reused to do that, we should. Note that I don't 
promise any level of support for any sublanguage of OWL here.
owl:sameAs looks like a much better fit, yes.
Looking at OWL Lite, I assume we're not talking about a full implementation,
because that sounds like a performance disaster.  Even taking bits and pieces,
such as owl:sameAs seems to have problems.  sameAs seems to be underspecified,
in that it's not clear whether it is a transitive and/or commutative;
http://www.w3.org/TR/owl-guide/#owl_sameAs doesn't make that clear.

I would guess, however, that it's intended to be transitive, so A sameAs B, B
sameAs C implies A sameAs C.  This sounds potentially expensive to implement,
especially for the CompositeDS.  I would also assume commutative -- which isn't
true for the use case we're talking about here: a rdf:$anon bookmark with a URL
arc to file:///foo/ doesn't mean that file:///foo/ is also a rdf:$anon bookmark.

It would also mean that every rdf:$blah that is a file:///foo/ is equivalent,
including things like the Name arc, which goes against the reasoning for
changing bookmarks to use anonymous rdf resources in the first place.

We can always implement our own non-transitive, non-commutative owl:sameAs, but
at that point I don't think we can call it owl:sameAs.
Flags: blocking-aviary1.0RC1+
So it seems like we (Benjamin and I) need a better spec of what we (vlad) want to 
do.

Why do we need to dance with the observers?
Why do we need to dance with commands?

Is there a particular reason for not going with a vocabulary and an 
nsInferDataSource.cpp? Do we need to support multiple arcs for that?

Is there any reason why the current code only works for one "aggregation-arc"?

Comments on the code:
Benjamin was on a hunt to make kRDFService mRDFService, guess you want to 
follow that.
Please do callFoo(aArg) instead of callFoo (aArg).
Use nsCOMPtr smartness, like assigns and swap.
Don't do anything in NS_IF_ADDREF. It works sometimes, and the compiler might
opt the double work out, but it's generally brittle.
I just wrote up http://www.axel-hecht.de/mozwiki/ProxyDataSource ; should be
easier to discuss this in the wiki as well.

Note that my patch doesn't quite implement the spec I describe in the document;
specifically, the command handling in the patch is broken. :)
Get this in soon.
Flags: blocking-aviary1.0+ → blocking-aviary1.0-
If it comes down to it, we'll just have to take this on the branch, but I really
don't want to have any divergences like this. We need a solution to the
aggregation problem for 1.0 as it will likely be downloaded and used by a lot of
people and this is incredibly useful to extensions that work with Bookmarks. 
The idea was to get it into browser/ and use it just there, instead of putting
it into rdf/; we could do this, but right now the template builder is what
creates it, and a dependency from layout/xul/ -> browser/ is probably not a good
thing.  We could rip out the template builder code, but we'd have to make the
usage more complex -- instead of the template builder creating one whenever it's
given a set of proxy properties, we'd have to specify them in some other way
(and have to figure out how to add and remove data sources..)

I guess worst-case I could make the ProxyDataSource derive from
CompositeDataSource..
(In reply to comment #18)
> The idea was to get it into browser/ and use it just there, instead of putting
> it into rdf/; we could do this, but right now the template builder is what
> creates it, and a dependency from layout/xul/ -> browser/ is probably not a good
> thing.  

I still favour to have a single fixed vocabulary for this. That would be more 
like RDF plus we could add "infer" to the flags and be done with it. That 
wouldn't require a new interface and thus no dependency on browser. Template 
builder should just fail very gracefully, if it can't generate the proxy
datasource from browser/.
Or add a attribute infer, which has a whitespace separated list of inferation
engines (implementing nsIRDFDataSource and nsIRDFObserver). And make browser
implement NS_RDF_CONTRACTID "/infer;1?engine=proxy".
(I'd call it "forward-proxy", even, but let's discuss the name in the wiki for
a cycle or two.)
(In reply to comment #19)
> 
> I still favour to have a single fixed vocabulary for this. That would be more 
> like RDF plus we could add "infer" to the flags and be done with it. That 
> wouldn't require a new interface and thus no dependency on browser. Template 
> builder should just fail very gracefully, if it can't generate the proxy
> datasource from browser/.

Ok, this sounds like a good approach.  My current problem is that I still need a
way to specify a child data source -- I guess I can implement
nsIRDFCompositeDataSource and only accept one data source?  Ideally, I could
just derive from CompositeDataSource, but the impl isn't public.  Deriving,
though, would make the template builder changes much less drastic, since I
wouldn't have to change any of the code that deals with the DS other than
initialization.
Attached patch bookmarks-aggregation-1.patch (obsolete) — — Splinter Review 
Updated patch, with incorporated comments.

nsProxyDataSource.{cpp,h} live in browser/components/bookmarks/src, and are
exposed as CID "@mozilla.org/rdf/infer;1?engine=proxy".  It implements
nsIRDFCompositeDataSource, with the restriction that it only supports adding
one data source to it, and that you can't modify
coalesceDuplicate/allowNegative (they are ignored).  It's only there to avoid
adding a new interface.

The template builder understands a new "infer" attribute that takes a single
contract ID (can always extend it to multiple later, single for now).  If it
has this, it tries to instantiate it in front of its normal composite db.

The fixed vocabulary is NC:ForwardProxy; hardcoded in nsProxyDataSource.

On the bookmarks side of things, any content that's built from something like
this can only be opened -- can't modify, copy, insert, etc. it.  It'd be nice
to support copy, but turns out its not trivial.  Any content that's a container
has no commands available at all (no context menu even).  The proxying is /not/
used in the bookmarks manager, only elsewhere.

I've only tested this with file: URLs; need to find some info on how to
generate search URLs or whatnot.
Attachment #151205 - Attachment is obsolete: true
Attachment #151208 - Attachment is obsolete: true
Comment on attachment 154537 [details] [diff] [review]
bookmarks-aggregation-1.patch

I don't like NC:foo vocabs, we have a bug to get rid of those, so don't create
new ones.
Inquiry for a good namespace is on the way.

I think I would like this one to get called forward-proxy, and to drop the
considerations
if one should support backwards-proxying, too.

>+++ browser/components/bookmarks/src/nsProxyDataSource.cpp	2004-07-27 21:50:28.228114581 

<...>

>+#include "xpcom-config.h"
>+#include NEW_H

I bet at least these two includes are not needed, check the others, too?

>+#include "nsCOMPtr.h"
>+#include "nsIComponentManager.h"
>+#include "nsIEnumerator.h"
>+#include "nsIRDFNode.h"
>+#include "nsIRDFObserver.h"
>+#include "nsIRDFDataSource.h"
>+#include "nsIRDFCompositeDataSource.h"
>+#include "nsIRDFRemoteDataSource.h"
>+#include "nsIRDFService.h"
>+#include "nsIServiceManager.h"
>+#include "nsXPIDLString.h"
>+#include "nsCOMArray.h"
>+#include "rdf.h"
>+#include "nsRDFCID.h"
>+
>+#include "nsEnumeratorUtils.h"
>+
>+#include "nsProxyDataSource.h"
>+
>+static NS_DEFINE_IID(kISupportsIID,  NS_ISUPPORTS_IID);
>+static NS_DEFINE_CID(kRDFServiceCID, NS_RDFSERVICE_CID);
>+
>+#ifdef PR_LOGGING
>+static PRLogModuleInfo* gLog = nsnull;
>+#endif
>+
>+PRInt32 nsProxyDataSource::gGlobalRefCnt = 0;
>+nsIRDFService* nsProxyDataSource::gRDF = nsnull;
>+
>+nsresult
>+NS_NewBookmarksProxyDataSource(nsIRDFDataSource** result)
>+{
>+    nsProxyDataSource* db = new nsProxyDataSource();
>+    if (! db)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+
>+    *result = db;
>+    NS_ADDREF(*result);
>+    return NS_OK;
>+}
>+
>+nsresult
>+nsProxyDataSource::Init(void)
>+{
>+    nsresult rv;
>+
>+    if (gGlobalRefCnt++ == 0) {
>+        rv = nsServiceManager::GetService(kRDFServiceCID,
>+                                          NS_GET_IID(nsIRDFService),
>+                                          NS_REINTERPRET_CAST(nsISupports**, &gRDF));
>+        NS_ASSERTION(NS_SUCCEEDED(rv), "unable to get RDF service");
>+        if (NS_FAILED(rv)) return rv;
>+        NS_ADDREF(gRDF);
>+    }
>+
>+#ifdef PR_LOGGING
>+    if (!gLog)
>+        gLog = PR_NewLogModule("nsRDFProxyDataSource");
>+#endif
>+
>+    return NS_OK;
>+}
>+
>+nsresult
>+nsProxyDataSource::Uninit(void)

Uninit is a rather rarely used name, rather use shutdown or something like
that.
I'd prefer to see you calling Init from NS_NewBookmarksProxyDataSource (not
sure if this
should be called NS_NewForwardProxyDataSource. I think we should settle with
"forward proxy"
as name.

>+
>+nsProxyDataSource::nsProxyDataSource()
>+    : mUpdateBatchNest(0)
>+{
>+    Init();
>+
>+    gRDF->GetResource(NS_LITERAL_CSTRING(NC_NAMESPACE_URI "ForwardProxy"),
>+                      getter_AddRefs(mProxyProperty));
>+}

The last call can fail and should be part of Init, IMHO.

>+nsProxyDataSource::nsProxyDataSource(nsIRDFDataSource* aDS)

This constructor is not used, I don't think we really need it.

>+//
>+// our two methods for munging sources
>+// note that the only two possible return values are NS_OK and NS_RDF_NO_VALUE
>+//

These methods both only support a single forwarding arc, though the spec allows
multiple ones.

>+nsresult
>+nsProxyDataSource::GetProxyResource(nsIRDFResource *aSource, nsIRDFResource **aResult)

<...>
>+
>+nsresult
>+nsProxyDataSource::GetRealSource(nsIRDFResource *aSource, nsIRDFResource **aResult)

<...>


>+
>+NS_IMETHODIMP
>+nsProxyDataSource::QueryInterface(REFNSIID iid, void** result)

I'd favour the NS_INTERFACE_MAP_FOO macros for implementing this.

>+    rv = NS_NewUnionEnumerator(aResult, dsEnumerator, proxyEnumerator);

This one would need to hold more than two enumerators, too, if I read the spec
right.

<...>

>+NS_IMETHODIMP
>+nsProxyDataSource::GetAllCmds(nsIRDFResource* aSource,
>+                              nsISimpleEnumerator** aResult)
>
Didn't check commands yet at all.
>+
>+// Observer bits
>+

These don't match the spec, that says that additional notifications would be
issued, this
implementation just replaces them. I would expect that the spec is right, here.

>+
>+//
>+// nsIRDFCompositeDataSource bits
>+//
>+
<...>
I wonder if I'd prefer to have a new interface. This is a hack. If we want to
allow for 
inference in general, we should unhack this, IMHO.

<...>
Didn't check the union enumerator, I see changes coming up here, still.

>Index: browser/components/bookmarks/content/bookmarks.js

I leave the bookmarks stuff up to someone knowledgable over there.

>Index: content/shared/public/nsXULAtomList.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/shared/public/nsXULAtomList.h,v
>retrieving revision 1.113
>diff -u -8 -p -r1.113 nsXULAtomList.h
>--- content/shared/public/nsXULAtomList.h	9 Mar 2004 09:01:46 -0000	1.113
>+++ content/shared/public/nsXULAtomList.h	28 Jul 2004 05:28:31 -0000
>@@ -305,8 +305,9 @@ XUL_ATOM(staticHint, "staticHint")
> XUL_ATOM(_star, "*")
> XUL_ATOM(defaultz, "default")
> XUL_ATOM(screenX, "screenX")
> XUL_ATOM(screenY, "screenY")
> XUL_ATOM(type, "type")
> XUL_ATOM(hidechrome, "hidechrome")
> XUL_ATOM(popupset, "popupset")
> XUL_ATOM(parsetype, "parsetype")
>+XUL_ATOM(infer, "infer")

do we need some XUL head approval, like from hyatt, to change templates like
this?

>Index: browser/base/content/browser-menubar.inc


>+                  infer="@mozilla.org/rdf/infer;1?engine=proxy"

I'd rather see us using just infer="proxy", or infer="forward-proxy", even.
You can instantiate an arbitrary XPCOM module here, that sounds like a bad
idea,
it might even be a security issue.

<...>

I bet those comments aren't complete, but this is as much as I can do until
sunday or monday.
Whiteboard: [eta 04-08-04]
Attached patch forward-proxy-ds-0.patch (obsolete) — — Splinter Review 
(Splitting this patch into 4 chunks for reviewing.)

This patch just has the nsForwardProxyDataSource and related makefile/nsModule
bits.

Axel, I incorporated some of your feedback, though I'm not completely finished
yet.  Just posting this part of the patch as a work-in-progress, to get the
other split chunks posted.  However, it still only supports one arc (even
though the spec claims multiple); supporting multiple arcs would mean requiring
a new interface to specify which arcs are supported, instead of hardcoding one
resource URI (whatever that ends up being).  Same thing goes with the
UnionEnumerator -- though in this case, I could extend it to take multiple
child enums, but we could also just chain multiple UnionEnumerators :)
Attached patch xpcom-union-enumerator-0.patch (obsolete) — — Splinter Review 
This patch implements UnionEnumerator in xpcom/ds.   The idea is to join two
enumerators together into one.
Attached patch template-infer-0.patch (obsolete) — — Splinter Review 
Changes to the template builder to allow optionally creating an
infer-datasource with a given inference engine in front of the normal composite
DS.
Attached patch bookmarks-agg-0.patch (obsolete) — — Splinter Review 
Bookmarks bits to make this all useful.  Introduces "ImmutableBookmark" and
"ImmutableFolder", which are anything that can't be identified as normal
bookmarks.  The only requirement really is that the ImmutableBookmark has Name
and URL arcs, though this isn't checked.
sorry. lengthy comments attached.
ok, the vocabulary should be 
http://developer.mozilla.org/rdf/vocabulary/forward-proxy,
the predicate should be
http://developer.mozilla.org/rdf/vocabulary/forward-proxy#forward-proxy.
That looks a bit odd, but it is more in synch with what other vocabs would do.

I'd like to see the 
@mozilla.org/rdf/infer-datasource;1?engine=
contractID fragment as a define inside rdf.h.

Nit on attachement #154900, use AppendUTF16toUTF8 routine in nsReadableUtils.h
instead of 
inferContractID += NS_ConvertUTF16toUTF8(infer);

That should really be the last comment from me in this round. Bah, I suck.
Who is going to review the other three patches, btw?
(In reply to comment #28)
> ok, the vocabulary should be 
> http://developer.mozilla.org/rdf/vocabulary/forward-proxy,
> the predicate should be
> http://developer.mozilla.org/rdf/vocabulary/forward-proxy#forward-proxy.
> That looks a bit odd, but it is more in synch with what other vocabs would do.

Ok; you sure you don't want something like
/rdf/vocabulary/inference#forward-proxy , so that we'd have a vocabulary for
other potential inference engines?

> I'd like to see the 
> @mozilla.org/rdf/infer-datasource;1?engine=
> contractID fragment as a define inside rdf.h.

Ok.

> Nit on attachement #154900, use AppendUTF16toUTF8 routine in nsReadableUtils.h
> instead of 
> inferContractID += NS_ConvertUTF16toUTF8(infer);
> 
> That should really be the last comment from me in this round. Bah, I suck.
> Who is going to review the other three patches, btw?

Thanks for the comments!  I've got a new impl coming up tomorrow, with your
feedback, and with a fixed IsCommandEnabled.  As for the other patches, shaver's
going to take a look at the UnionEnumerator patch, and the others.. not sure,
but they're more minor than those two.  mconnor can probably look at the
bookmarks one.  I'd like to get the enumerator and the data source passing
muster first though before looking for reviews for the others (as they'll depend
on the ds).
(In reply to comment #29)
> (In reply to comment #28)
> > ok, the vocabulary should be 
> > http://developer.mozilla.org/rdf/vocabulary/forward-proxy,
> > the predicate should be
> > http://developer.mozilla.org/rdf/vocabulary/forward-proxy#forward-proxy.
> > That looks a bit odd, but it is more in synch with what other vocabs would do.
> 
> Ok; you sure you don't want something like
> /rdf/vocabulary/inference#forward-proxy , so that we'd have a vocabulary for
> other potential inference engines?

The vocabulary is specific to the engine. I could group the vocabularies for
inference in the index page, but other than that, the vocabularies don't really 
have anything to do with each other. This specific predicate looks a bit odd,
as it has only one predicate inside the vocabulary. But others may very well
have more, so they'd end up with /vocabulary-name#predicate, like other
vocabs on the web do.
So the vocabularies for the inference engines would live next to each other
beneath /rdf/vocabulary/, each in their own page.
Attached patch forward-proxy-ds-1.patch (obsolete) — — Splinter Review 
New ForwardProxy, with Axel's feedback incorporated.

For the NS_NewUnionEnumerator, if one of its args is null, it'll just return
the other arg without creating a nsUnionEnumerator at all.  Also, looking at
the other datasources, I'm not sure if GetURI should return a non-null URI
(though this version does) -- CompositeDataSource and InMemoryDataSource always
return null URIs, and those are the most likely child DSs of this.
Attachment #154898 - Attachment is obsolete: true
Comment on attachment 154899 [details] [diff] [review]
xpcom-union-enumerator-0.patch

In this house, we use nsCOMPtr for owning refs.

Your brace style (esp. for if/else that have one single-line clause and one
multi-line) doesn't fit with what we do here in Mozilla land, at least in the
parts of XPCOM that I care about.  Please brace all parts, if any are braced.

I'm not entirely sold on the name -- I might prefer nsConcatenatingEnumerator
since I would expect a union to eliminate duplicates.  And I think we might
want to support more than 2 enumerators as operands.

But this'll certainly do for now, and we can revisit those issues based on
experience using them.	r=shaver, with COMPtr and bracing fixed.

(Please attach a diff-as-committed for ease of migration to other branches,
also.)
Attachment #154899 - Flags: review?(shaver) → review+
Comment on attachment 155307 [details] [diff] [review]
forward-proxy-ds-1.patch

>--- /dev/null	2004-02-23 13:02:56.000000000 -0800
>+++ rdf/base/idl/nsIRDFInferDataSource.idl	2004-08-05 13:00:22.696318573 -0700
>@@ -0,0 +1,60 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1
>

bad license

>--- /dev/null	2004-02-23 13:02:56.000000000 -0800
>+++ browser/components/bookmarks/src/nsForwardProxyDataSource.h	2004-08-05 13:00:22.490382408 -0700
>@@ -0,0 +1,86 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

bad license

>+
>+#ifndef FORWARDPROXYDATASOURCE___H___
>+#define FORWARDPROXYDATASOURCE___H___
>+
>+#include "nsCOMArray.h"
>+#include "nsIRDFService.h"
>+#include "nsIRDFInferDataSource.h"
>+#include "nsIRDFDataSource.h"
>+
>+class nsForwardProxyDataSource : public nsIRDFInferDataSource,
>+                                 public nsIRDFObserver
>+{

<...>
>+
>+    static PRInt32 gGlobalRefCnt;
>+    static nsIRDFService* gRDF;
>+
>+    nsresult Shutdown(void);

get rid of these three, see below

>+};
>+
>+#endif /* FORWARDPROXYDATASOURCE___H___ */
>--- /dev/null	2004-02-23 13:02:56.000000000 -0800
>+++ browser/components/bookmarks/src/nsForwardProxyDataSource.cpp	2004-08-05 13:10:20.912884955 -0700
>@@ -0,0 +1,746 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

bad license

<...>
>+
>+PRInt32 nsForwardProxyDataSource::gGlobalRefCnt = 0;
>+nsIRDFService* nsForwardProxyDataSource::gRDF = nsnull;
>+

remove these and ...

>+nsresult
>+nsForwardProxyDataSource::Init(void)
>+{
>+    nsresult rv;
>+
>+    // do we need to initialize our globals?
>+    if (gGlobalRefCnt++ == 0) {
>+        rv = nsServiceManager::GetService(kRDFServiceCID,
>+                                          NS_GET_IID(nsIRDFService),
>+                                          NS_REINTERPRET_CAST(nsISupports**, &gRDF));

just use a nsCOMPtr<nsIRDFService> rdf = do_GetService() here.
You never use the rdf service anymore, no reason to keep it around.

>+        NS_ASSERTION(NS_SUCCEEDED(rv), "unable to get RDF service");
>+        if (NS_FAILED(rv)) return rv;
>+        NS_ADDREF(gRDF);
>+    }
>+
>+    // per-instance members: this might eventually be different per-DS,
>+    // so it's not static
>+    rv = gRDF->GetResource(NS_LITERAL_CSTRING(DEVMO_NAMESPACE_URI_PREFIX "forward-proxy#forward-proxy"),
>+                           getter_AddRefs(mProxyProperty));
>+    if (NS_FAILED(rv)) return rv;
>+
>+    return NS_OK;
>+}
>+
>+nsresult
>+nsForwardProxyDataSource::Shutdown(void)
>+{
>+    if (--gGlobalRefCnt == 0) {
>+        NS_RELEASE(gRDF);
>+        gRDF = nsnull;
>+    }
>+
>+    return NS_OK;
>+}

As you don't need to release the rdf service anymore, this thing goes away
completely.

>+nsForwardProxyDataSource::nsForwardProxyDataSource()
>+    : mUpdateBatchNest(0)
>+{
>+}
>+
>+nsForwardProxyDataSource::~nsForwardProxyDataSource()
>+{
>+    Shutdown();
>+}

this one is just empty. You should implement it though, that fixes a warning.

>+
>+//
>+// our two methods for munging sources
>+// note that the only two possible return values are NS_OK and NS_RDF_NO_VALUE
>+//

add a comment about supporting multiple foward proxy arcs

// XXX see bug xxxxx to add support for multiple forward proxy arcs

<...>

>+nsresult
>+nsForwardProxyDataSource::GetRealSource(nsIRDFResource *aSource, nsIRDFResource **aResult)
>+{
>+    nsresult rv;
>+    nsCOMPtr<nsIRDFResource> realSourceResource;
>+
>+    rv = mDS->GetSource(mProxyProperty, aSource, PR_TRUE,
>+                        getter_AddRefs(realSourceResource));
>+    if (NS_FAILED(rv) || rv == NS_RDF_NO_VALUE) return NS_RDF_NO_VALUE;
>+
>+    *aResult = nsnull;

move this one up? Like you did in GetProxySource()
So that *aResult is nsnull on failure.

>+    realSourceResource.swap(*aResult);
>+    return NS_OK;
>+}
>+
>+//----------------------------------------------------------------------
>+//
>+// nsISupports interface
>+//
>+
>+NS_IMPL_ADDREF(nsForwardProxyDataSource)
>+
>+//
>+// Use a custom Release() for the same reasons one is used in the
>+// Composite DS; we have circular relationships with our child DS.
>+
>+NS_IMETHODIMP_(nsrefcnt)
>+nsForwardProxyDataSource::Release()
>+{
>+    NS_PRECONDITION(PRInt32(mRefCnt) > 0, "duplicate release");
>+    nsrefcnt count =
>+      PR_AtomicDecrement(NS_REINTERPRET_CAST(PRInt32 *, &mRefCnt));

this is not threadsafe anymore, just use --mRefCnt.

In case you wonder, prefix incs don't need to generate a temporary object
and are thus liked better within mozilla code.

>+
>+    if (count == 0) {
>+        NS_LOG_RELEASE(this, count, "nsForwardProxyDataSource");
>+        mRefCnt = 1;
>+        NS_DELETEXPCOM(this);
>+        return 0;
>+    }
>+    else if (mDS && (PRInt32(count) == 1)) {
>+        // if the count is 1, the only ref is from our nested data
>+        // source, which holds on to us as an observer.
>+
>+        // We must add 1 here because otherwise the nested releases
>+        // on this object will enter this same code path.
>+        PR_AtomicIncrement(NS_REINTERPRET_CAST(PRInt32 *, &mRefCnt));

++mRefCnt.

>+
>+        mDS->RemoveObserver(this);
>+        mDS = nsnull;
>+
>+        // In CompositeDataSource, there's a comment here that we call
>+        // ourselves again instead of just doing a delete in case
>+        // something might have added a ref count in the meantime.
>+        // However, if that happens, this object will be in an
>+        // inconsistent state, because we'll have removed the Observer
>+        // from mDS.  Hence the assertion.
>+        NS_ASSERTION(mRefCnt >= 1, "bad mRefCnt");
>+        return Release();
>+    }
>+    else {
>+        NS_LOG_RELEASE(this, count, "nsForwardProxyDataSource");
>+        return count;
>+    }
>+}
>+

<...>

>+//
>+// methods which need proxying
>+//
>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::GetTarget(nsIRDFResource* aSource,
>+                                    nsIRDFResource* aProperty,
>+                                    PRBool aTruthValue,
>+                                    nsIRDFNode** aResult)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    nsresult rv = NS_RDF_NO_VALUE;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+
>+    rv = mDS->GetTarget(aSource, aProperty, aTruthValue, aResult);
>+    if (NS_FAILED(rv) || rv == NS_OK)
>+        return rv;
>+
>+    if (GetProxyResource(aSource, getter_AddRefs(proxyResource)) == NS_OK)
>+        rv = mDS->GetTarget(proxyResource, aProperty, aTruthValue, aResult);
>+
>+    return rv;
>+}
>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::GetTargets(nsIRDFResource* aSource,
>+                                     nsIRDFResource* aProperty,
>+                                     PRBool aTruthValue,
>+                                     nsISimpleEnumerator** aResult)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+    nsCOMPtr<nsISimpleEnumerator> proxyEnumerator, dsEnumerator;
>+
>+    rv = mDS->GetTargets(aSource, aProperty, aTruthValue, getter_AddRefs(dsEnumerator));
>+    if (NS_FAILED(rv)) return rv;
>+
>+    if (GetProxyResource(aSource, getter_AddRefs(proxyResource)) == NS_OK) {
>+        rv = mDS->GetTargets(proxyResource, aProperty,
>+                             aTruthValue, getter_AddRefs(proxyEnumerator));
>+        if (NS_FAILED(rv)) return rv;
>+    }
>+
>+    rv = NS_NewUnionEnumerator(aResult, dsEnumerator, proxyEnumerator);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::HasAssertion(nsIRDFResource* aSource,
>+                                       nsIRDFResource* aProperty,
>+                                       nsIRDFNode* aTarget,
>+                                       PRBool aTruthValue,
>+                                       PRBool* aResult)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+
>+    *aResult = PR_FALSE;
>+
>+    rv = mDS->HasAssertion(aSource, aProperty, aTarget, aTruthValue, aResult);
>+    if (NS_SUCCEEDED(rv) && *aResult)
>+        return rv;
>+
>+    if (GetProxyResource(aSource, getter_AddRefs(proxyResource)) == NS_OK)
>+        rv = mDS->HasAssertion(proxyResource, aProperty, aTarget, aTruthValue, aResult);
>+
>+    return rv;
>+}
>+
>+NS_IMETHODIMP 
>+nsForwardProxyDataSource::HasArcOut(nsIRDFResource *aSource, nsIRDFResource *aArc, PRBool *aResult)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+
>+    *aResult = PR_FALSE;
>+
>+    rv = mDS->HasArcOut(aSource, aArc, aResult);
>+    if (NS_SUCCEEDED(rv) && *aResult)
>+        return rv;
>+
>+    if (GetProxyResource(aSource, getter_AddRefs(proxyResource)) == NS_OK)
>+        rv = mDS->HasArcOut(proxyResource, aArc, aResult);
>+
>+    return rv;
>+}
>+
>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::ArcLabelsOut(nsIRDFResource* aSource,
>+                                       nsISimpleEnumerator** aResult)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+    nsCOMPtr<nsISimpleEnumerator> proxyEnumerator, dsEnumerator;
>+
>+    rv = mDS->ArcLabelsOut(aSource, getter_AddRefs(dsEnumerator));
>+    if (NS_FAILED(rv)) return rv;
>+
>+    if (GetProxyResource(aSource, getter_AddRefs(proxyResource)) == NS_OK) {
>+        rv = mDS->ArcLabelsOut(proxyResource, getter_AddRefs(proxyEnumerator));
>+        if (NS_FAILED(rv)) return rv;
>+    }
>+
>+    rv = NS_NewUnionEnumerator(aResult, dsEnumerator, proxyEnumerator);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::GetAllCmds(nsIRDFResource* aSource,
>+                                     nsISimpleEnumerator** aResult)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+    nsCOMPtr<nsISimpleEnumerator> proxyEnumerator, dsEnumerator;
>+
>+    if (GetProxyResource(aSource, getter_AddRefs(proxyResource)) == NS_OK) {
>+        rv = mDS->GetAllCmds(proxyResource, getter_AddRefs(proxyEnumerator));
>+        if (NS_FAILED(rv)) return rv;
>+    }
>+
>+    rv = mDS->GetAllCmds(aSource, getter_AddRefs(dsEnumerator));
>+    if (NS_FAILED(rv)) return rv;
>+
>+    rv = NS_NewUnionEnumerator(aResult, dsEnumerator, proxyEnumerator);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::IsCommandEnabled(nsISupportsArray* aSources,
>+                                           nsIRDFResource* aCommand,
>+                                           nsISupportsArray* aArguments,
>+                                           PRBool* aResult)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIRDFResource> source;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+
>+    *aResult = PR_FALSE;
>+
>+    PRUint32 sourcesCount;
>+    rv = aSources->Count(&sourcesCount);
>+    if (sourcesCount == 0) {
>+        *aResult = PR_FALSE;
>+        return NS_OK;
>+    }
>+    // if the original DS thinks the commands are ok, they're ok.
>+    rv = mDS->IsCommandEnabled(aSources, aCommand, aArguments, aResult);
>+    if (NS_SUCCEEDED(rv) && *aResult)
>+        return NS_OK;
>+
>+    nsCOMPtr<nsISupportsArray> proxiedSources;
>+    rv = NS_NewISupportsArray(getter_AddRefs(proxiedSources));
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    for (PRUint32 i = 0; i < sourcesCount; i++) {
>+        source = do_QueryElementAt(aSources, i, &rv);
>+        if (NS_FAILED(rv)) return rv;
>+
>+        if (GetProxyResource(source, getter_AddRefs(proxyResource)) == NS_OK) {
>+            rv = proxiedSources->AppendElement(proxyResource);
>+            if (NS_FAILED(rv))
>+                return rv;
>+        } else {
>+            rv = proxiedSources->AppendElement(proxyResource);
>+        }
>+    }
>+
>+    return mDS->IsCommandEnabled(proxiedSources, aCommand, aArguments, aResult);
>+}
>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::DoCommand(nsISupportsArray* aSources,
>+                                    nsIRDFResource* aCommand,
>+                                    nsISupportsArray* aArguments)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    nsresult rv;
>+    nsCOMPtr<nsISupportsArray> fixedSources;
>+    nsCOMPtr<nsIRDFResource> source;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+
>+    PRUint32 sourcesCount;
>+    rv = aSources->Count(&sourcesCount);
>+    if (sourcesCount == 0) {
>+        return NS_OK;
>+    }
>+
>+    // go through the aSources array, looking for things which
>+    // have aggregate resources -- if they're found, append them
>+    // to a new array, and then pass the whole thing to mDS
>+    for (PRUint32 i = 0; i < sourcesCount; i++) {
>+        source = do_QueryElementAt(aSources, i, &rv);
>+        if (NS_FAILED(rv)) return rv;
>+
>+        if (GetProxyResource(source, getter_AddRefs(proxyResource)) == NS_OK) {
>+            if (!fixedSources) {
>+                rv = aSources->Clone(getter_AddRefs(fixedSources));
>+                if (NS_FAILED(rv)) return rv;
>+            }
>+
>+            rv = aSources->AppendElement(proxyResource);
>+            if (NS_FAILED(rv)) return rv;
>+        }
>+    }
>+
>+    if (fixedSources) {
>+        return mDS->DoCommand(fixedSources, aCommand, aArguments);
>+    } else {
>+        return mDS->DoCommand(aSources, aCommand, aArguments);
>+    }
>+}
>+
>+

IMHO, IsCommandEnabled should check for the very same array that DoCommand is
using. Put that into a utility function and be done with it?
IsCommandEnabled may return true for the initial array but return false for the
proxied one.

<...>

>+//
>+// nsIRDFObserver impl
>+//
>+
>+// we coalesce observed actions on an aggregate resource
>+// as if occuring on the parent

fix comment to note that you're actually generating the notifications for
both the actual source and the proxied source.

>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::OnAssert(nsIRDFDataSource* aDataSource,
>+                                   nsIRDFResource* aSource,
>+                                   nsIRDFResource* aProperty,
>+                                   nsIRDFNode* aTarget)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    for (PRInt32 i = mObservers.Count() - 1; i >= 0; --i) {
>+        mObservers[i]->OnAssert(this, aSource, aProperty, aTarget);
>+    }
>+
>+    nsCOMPtr<nsIRDFResource> realSourceResource;
>+    if (!GetRealSource(aSource, getter_AddRefs(realSourceResource)) != NS_OK) {

too many knots.

>+        for (PRInt32 i = mObservers.Count() - 1; i >= 0; --i) {
>+            mObservers[i]->OnAssert(this, realSourceResource, aProperty, aTarget);
>+        }
>+    }
>+        return NS_OK;
>+
>+    return NS_OK;

one of these should do ;-)

>+}
>+

<...>

>+
>+NS_IMETHODIMP
>+nsForwardProxyDataSource::OnMove(nsIRDFDataSource* aDataSource,
>+                                 nsIRDFResource* aOldSource,
>+                                 nsIRDFResource* aNewSource,
>+                                 nsIRDFResource* aProperty,
>+                                 nsIRDFNode* aTarget)
>+{
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+
>+    for (PRInt32 i = mObservers.Count() - 1; i >= 0; --i) {
>+        mObservers[i]->OnChange(this, aOldSource, aNewSource, aProperty, aTarget);
>+    }
>+
>+    nsCOMPtr<nsIRDFResource> realOldResource;
>+    nsCOMPtr<nsIRDFResource> realNewResource;
>+
>+    GetRealSource(aOldSource, getter_AddRefs(realOldResource));
>+    GetRealSource(aNewSource, getter_AddRefs(realNewResource));
>+
>+    if ((!realOldResource && !realNewResource) ||
>+        (realOldResource == realNewResource))
>+        return NS_OK;
>+
>+    for (PRInt32 i = mObservers.Count() - 1; i >= 0; --i) {
>+        mObservers[i]->OnChange(this,
>+                                realOldResource ? realOldResource.get() : aOldSource,
>+                                realNewResource ? realNewResource.get() : aNewSource,
>+                                aProperty, aTarget);

no .get()s

>+    }
>+
>+    return NS_OK;
>+}

<...>

>Index: rdf/base/public/rdf.h
>===================================================================
>RCS file: /cvsroot/mozilla/rdf/base/public/rdf.h,v
>retrieving revision 1.27
>diff -u -8 -p -r1.27 rdf.h
>--- rdf/base/public/rdf.h	27 Feb 2003 03:05:41 -0000	1.27
>+++ rdf/base/public/rdf.h	5 Aug 2004 20:22:57 -0000
>@@ -65,17 +65,17 @@ static const char kURI##prefix##_##name[
> 
> /**
>  * Core RDF vocabularies that we use to define semantics
>  */
> 
> #define RDF_NAMESPACE_URI  "http://www.w3.org/1999/02/22-rdf-syntax-ns#"
> #define WEB_NAMESPACE_URI  "http://home.netscape.com/WEB-rdf#"
> #define NC_NAMESPACE_URI   "http://home.netscape.com/NC-rdf#"
>-
>+#define DEVMO_NAMESPACE_URI_PREFIX "http://developer.mozilla.org/rdf/vocabulary/"
> 
> /**
>  * @name Standard RDF error codes
>  */
> 
> /*@{*/
> 
> /* Returned from nsIRDFCursor::Advance() if the cursor has no more
>@@ -98,16 +98,18 @@ static const char kURI##prefix##_##name[
> 
> 
> /* ContractID prefixes for RDF DLL registration. */
> #define NS_RDF_CONTRACTID                           "@mozilla.org/rdf"
> #define NS_RDF_DATASOURCE_CONTRACTID                NS_RDF_CONTRACTID "/datasource;1"
> #define NS_RDF_DATASOURCE_CONTRACTID_PREFIX         NS_RDF_DATASOURCE_CONTRACTID "?name="
> #define NS_RDF_RESOURCE_FACTORY_CONTRACTID          "@mozilla.org/rdf/resource-factory;1"
> #define NS_RDF_RESOURCE_FACTORY_CONTRACTID_PREFIX   NS_RDF_RESOURCE_FACTORY_CONTRACTID "?name="
>+#define NS_RDF_INFER_DATASOURCE_CONTRACTID          NS_RDF_CONTRACTID "/infer-datasource;1"
>+#define NS_RDF_INFER_DATASOURCE_CONTRACTID_PREFIX   NS_RDF_INFER_DATASOURCE_CONTRACTID "?engine="

just the latter. the resource-factory one is just defined as that is used for
the 
default resource factory. We don't have that here.

> // contract ID is in the form
> // @mozilla.org/rdf/delegate-factory;1?key=<key>&scheme=<scheme>
> #define NS_RDF_DELEGATEFACTORY_CONTRACTID    "@mozilla.org/rdf/delegate-factory;1"
> #define NS_RDF_DELEGATEFACTORY_CONTRACTID_PREFIX    NS_RDF_DELEGATEFACTORY_CONTRACTID "?key="
> 
> /*@}*/
> 

<...>

we're getting close, but not close enough for a conditional r+, IMHO.
Nice catch to add the devmo macro to rdf.h, thanx.
Do change the NPL to MPL licenses.
Attachment #155307 - Flags: review?(axel) → review-
Attachment #151205 - Flags: review?(axel)
Attached patch forward-proxy-ds-2.patch (obsolete) — — Splinter Review 
Take 3!

I don't think I missed anything in the comments... I hope not, anyway.	(Those
.get()'s in OnChange are necessary, otherwise gcc complains about operands to
?: having differing types.)
Attachment #155307 - Attachment is obsolete: true
Attaching final union enumerator patch.
Attachment #154899 - Attachment is obsolete: true
Comment on attachment 155637 [details] [diff] [review]
forward-proxy-ds-2.patch

browser/components/bookmarks/src/nsForwardProxyDataSource.cpp


<...>

>+NS_IMETHODIMP
>+nsForwardProxyDataSource::GetURI(char* *uri)
>+{
>+    NS_PRECONDITION(uri != nsnull, "null pointer");
>+    NS_PRECONDITION(mDS != nsnull, "Null datasource");
>+    nsresult rv;
>+
>+    nsCAutoString theURI(NS_LITERAL_CSTRING("x-rdf-infer:forward-proxy"));
>+
>+    char *dsURI = nsnull;
>+    rv = mDS->GetURI(&dsURI);
>+    if (NS_FAILED(rv)) return rv;

dsURI is newly allocated, wrap it in an nsXPIDLCString,
getterCopies so that it get's deallocated nicely.

>+
>+    if (dsURI) {
>+        theURI += NS_LITERAL_CSTRING("?ds=");

theURI.AppendLiteral("?ds=");

>+        theURI += nsDependentCString(dsURI);

Just the nsXPIDLCString here
>+    }
>+
>+    if ((*uri = nsCRT::strdup(theURI.get())) == nsnull)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+
>+    return NS_OK;
>+}

<...>

Make GetProxyResourcesArray crash safe for null input and don't
return nsnull and NS_OK, or at least add a comment that you do.
As that is not XPCOM style. In this particular case, it'd be
ok, but you should return early or something of you end up
empty. As the bookmarks DS crashes, too, for example.

>+// helper method for munging an array of sources into an array of the
>+// sources plus their proxy resources.
>+
>+nsresult
>+nsForwardProxyDataSource::GetProxyResourcesArray (nsISupportsArray* aSources,
>+                                                  nsISupportsArray** aSourcesAndProxies)
>+{
>+    nsresult rv;
>+    nsCOMPtr<nsISupportsArray> fixedSources;
>+    nsCOMPtr<nsIRDFResource> source;
>+    nsCOMPtr<nsIRDFResource> proxyResource;
>+
>+    PRUint32 sourcesCount;
>+    rv = aSources->Count(&sourcesCount);
>+    if (sourcesCount == 0) {
>+        return NS_OK;
>+    }
>+
>+    // go through the aSources array, looking for things which
>+    // have aggregate resources -- if they're found, append them
>+    // to a new array
>+    for (PRUint32 i = 0; i < sourcesCount; i++) {
>+        source = do_QueryElementAt(aSources, i, &rv);
>+        if (NS_FAILED(rv)) return rv;
>+
>+        if (GetProxyResource(source, getter_AddRefs(proxyResource)) == NS_OK) {
>+            if (!fixedSources) {
>+                rv = aSources->Clone(getter_AddRefs(fixedSources));
>+                if (NS_FAILED(rv)) return rv;
>+            }
>+
>+            rv = aSources->AppendElement(proxyResource);
>+            if (NS_FAILED(rv)) return rv;
>+        }

You never append to fixedSources, right? I should have catched
that the first time around, I guess.

>+    }
>+
>+    if (!fixedSources) {
>+        *aSourcesAndProxies = aSources;
>+    } else {
>+        *aSourcesAndProxies = fixedSources;
>+    }
>+    NS_ADDREF(*aSourcesAndProxies);
>+
>+    return NS_OK;
>+}
>+

<...>

You have a few if() single line, see comment #32, braces go
everywhere.
Still getting closer, sorry.
Attachment #155637 - Flags: review?(axel) → review-
Attached file nsForwardProxyDataSource.cpp (#3) (obsolete) — 
I suck.  Thanks for taking the time to go over this in detail, nice to have
things caught at review time than at crash time. :) Here's #4!	Fixed the
string leak, added {}'s around single-line ifs (other than if (NS_FAILED(rv))
return rv -- didn't make these ENSURE_SUCCESS because the rest of similar code
doesn't do much ENSURE_SUCCESS'ing, though that's a weak excuse), and fixed the
clone'd Array business.

Attaching only the cpp file; I'll make a final patch once this passes Axel's
review.
cleaning up old eta, 
be good if we could get quick review from axel, shaver, or other
Whiteboard: [eta 04-08-04] → [have patch]
how likely is this to break stuff?  if its pretty isolated then maybe it could
still this late in the PR cycle, othewise its time to minus this bug.
Overall pretty isolated; I believe axel already gave me r+, but he didn't flag
the attachment -- I haven't been able to catch him on irc to confirm.  I'm
fairly comfortable with this working how it should, but unless ben really wants
it in, I have no objections to pushing it out past 1.0 (1.1 maybe?)..
Comment on attachment 156057 [details]
nsForwardProxyDataSource.cpp (#3)

Axel, I can't remember what you said on irc a few days ago -- is this good to
go?
Attachment #156057 - Flags: review?(axel)
Comment on attachment 156057 [details]
nsForwardProxyDataSource.cpp (#3)

r=me on this file and the rest of the patch with
if (NS_FAILED(rv)) return rv;

return NS_OK;

turned into

return rv;

in Init, GetTargets, ArcLabelsOut, GetAllCmds.

Thanx for your patience.
Attachment #156057 - Flags: review?(axel) → review+
Attached patch Final patch: Gecko portion (obsolete) — — Splinter Review 
This is the final patch for this that makes rdf, content, and layout changes. 
There is no functional change with this patch, unless ineference is used (via
the infer attribute) and an inference engine is implemented (which isn't in the
rdf tree for now).
Attachment #154900 - Attachment is obsolete: true
Attachment #155637 - Attachment is obsolete: true
This is the final patch of the Firefox portion of bookmarks aggregation.  This
is also where the ForwardProxyDataSource implementation lives
(bookmarks-specific, for now).
Attachment #154902 - Attachment is obsolete: true
Attachment #156057 - Attachment is obsolete: true
I supsect this patch is causing the crashes people are seeing with the 20040818
windows nightly.
do we have a bug or talkback links for the crashes?
(In reply to comment #46)
> do we have a bug or talkback links for the crashes?

It was bug 255991, fixed as of earlier this morning.
Did this patch already land on the branch (as mentioned on the burning edge)? If
yes, someone should add a note on the status whiteboard.
(In reply to comment #48)
> Did this patch already land on the branch (as mentioned on the burning edge)? If
> yes, someone should add a note on the status whiteboard.

Yes, this is in on the branch; not in on trunk yet as trunk was closed until,
er, a few hours ago :)  Will be there soon.
Whiteboard: [have patch] → [in-aviary]
Blocks: 256077
Still not on the trunk?
Not yet; a few problems have come up that I'm fixing, and I'd rather not have to
fix them in both places.  Should be on trunk by monday, along with new "final
patches" here.
adding fixed-aviary1.0 keyword
Keywords: fixed-aviary1.0
Is this on the trunk yet? How many problems did it cause?
Not yet; I've been chasing down some firefox-specific bugs when inference is
actually used within a template builder.  However, I will land the non-browser
portion of this on trunk today, been dragging my heels too long on that.  I'll
do the browser/* merge soonish after that.  It caused no problems on aviary
other than in cases where inference wasn't explicitly enabled (other than the
help viewer issue, which was fixed), and the problems it did cause were mainly
due to js and other code making assumptions about which composite DS to get data
from, instead of using the inference ds.

I'll place a new gecko-only patch here to correspond to what I landed on trunk.
New clean patch, against Trunk, for gecko-only bits for inference.
Attachment #155638 - Attachment is obsolete: true
Attachment #156398 - Attachment is obsolete: true
Comment on attachment 157443 [details] [diff] [review]
235665-trunk-gecko-inference.patch

Yeah, this looks like a faithful (and mercifully small) port.
Attachment #157443 - Flags: superreview+
Checked in on trunk.   I don't think this is needed for 1.7, but mkaply can
decide that -- the only bits that actually use this currently live in firefox.
yeah, I don't see a reason to put this on 1.7.
So, since the Gecko patch landed on the trunk years ago, and the Firefox patch landed on the trunk with the Aviary landing, is this still open for any particular reason?
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
Assignee: vladimir → nobody
Status: ASSIGNED → NEW
vladd, Michael?

(In reply to comment #59)
> ... Is this still open for any particular reason?

I'd say it was only left open for resolution of comment 57 and then not closed out after Michael Kaply's response.


Luckily, since the only activity for years has been people washing their hands of it, nobody will notice when I just close it.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee: nobody → vladimir
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: