70966 - Need ability to name roots in an RDFXML datasource

Status: RESOLVED WONTFIX

(Core Graveyard :: RDF, defect)

Description

[David Hyatt]

I need the ability (in order to avoid the accumulation of a whole hell of
a lot of detritus in the skins/locales RDF files) to name roots in an RDF/XML
datasource.  The Flush API should only flush an assertion if the source of
the assertion is reachable through forward arcs from one of the named roots.

Once this is ready, I'll presumably need to patch the chrome registry to
make use of your new API.

Comment 1

[Chase Tingley]

I'm all over this bug -- I need something to take my mind off the sort service.

Comment 2

[Chase Tingley]

Attachment: first try at a patch

Comment 3

[Chase Tingley]

This patch does several things:

1. Adds a META_RDF_NAMESPACE_URI to be used here to track datasource roots. 
This will be reused in other bugs to clean up stuff ("rdf:nextVal") and possibly
in the future (e.g. a meta-aboutEach property).  I was debating between using
the words "monkeys", "goats", and "electric" in the namespace uri; I'm partial
to the first and third, but thematic consistency is important.

2. Updates the serializer to look for all resources reachable by following a
meta:HasRoot property arc from the resource meta:DataSource.  This could be done
the other way as well -- properties off the root resources leading to a common
target; I'm not sure if either way is better... 

If any root resources are found, they will be iteratively traversed to build up
a list of reachable resources.  If no roots are found, all resources are assumed
reachable.

3. Adds a simple MarkRoot() method to nsIRDFXMLSource to wrap the root
assertion.  If anyone can think of a better place to add this, please say so --
adding it to nsIRDFDataSource would be a pain, and in some cases useless...
since it's an ability associated with serializability, I added it to Source
(since you can QI the RDFXMLDataSource -> Source, but not -> Serializer. 
Incidentally, there's something about the relationship between Source and
Serializer that strikes me as not quite right).

4. Added very basic mark/purge capabilities to nsNameSpaceMap.  Unused
namespaces will no longer be serialized in the prologue.

5. Cleaned out some stale method declarations from RDFXMLDataSource.
---

I'm not thrilled with some of the serializer changes -- this thing with sticking
GetAllResources into an array seems bogus.  I was looking for an
nsISupportsArray implementation that could produce an nsISimpleEnumerator on
demand, so I could leave the GetAllResources() enumerator as-is, build an array
in the traversing case, and then enumerate for proper serialization, but no such
beast exists afaict.  If you see a better direction for this to be going, tell me.

Comment 4

[Chris Waterson]

This looks great to me; I'm not too worried about the array you use to collect
up the resources. Might be good to call out O(n**2) problems (e.g., use of
IndexOf to determine if we've visited the resource yet). These aren't a big deal
now, but we might have to fix them some day. Also, change ``goats#'' to
``schema.rdf#'', and I'll put an RDF/XML file there that describes the properties.

Comment 5

[Chris Waterson]

Comment on attachment 56729 [details] [diff] [review]
first try at a patch

sr=waterson

Comment 6

[Chase Tingley]

Attachment: au revoir les chèvres and a couple other cleanups

Comment 7

[Chase Tingley]

Comment on attachment 56844 [details] [diff] [review]
au revoir les chèvres and a couple other cleanups

might as well mark those other IndexOf() calls as well.  one more time.

Comment 8

[Chase Tingley]

Attachment: comment a couple other O(n**2)

Comment 9

[Chris Waterson]

Comment on attachment 56847 [details] [diff] [review]
comment a couple other O(n**2)

Looks great, sr=waterson

Comment 10

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 11

[Chase Tingley]

Attachment: same as previous but also patches nsChromeRegistry::UninstallProvider

New version; the chrome registry will now use this API in UninstallProvider. 
This got a lot uglier after hyatt's all-* -> chrome.rdf merge, since there are
three roots that must be marked every time (adding a "root of roots" resource
could solve this...).

Comment 12

[Chris Waterson]

reassign to tingley; he's done the work. Re-reviewing these changes, I noticed a
few things. First off, are they up-to-date? Might need to rev this patch...

Second, there are a few potential flow-of-control leaks that all start here:

+        nsIRDFResource* root;
+        CallQueryInterface(isupports, &root);  // addref
+        if (! root)
             continue;

Note that you _can_ use an nsSupportsArray on the stack (instead of an
nsAutoVoidArray): doing so might save you some headache if you have to leave the
routine early due to an error condition.

Did you want to turn this into a boolean expression?

+    PRBool hasSpecifiedRoot = (reachable.Count());

e.g.,

     PRBool hasSpecifiedRoot = (reachable.Count() > 0);

Or, simply move the condition into the |if| statement below since it's only used
once?


You don't ever make use of |rv| here, so why not just nuke it.

+            rv = mDataSource->GetTargets(reachableResource, property, PR_TRUE,
+                                         getter_AddRefs(arcTargets));


In your loops, it might help iterate backwards, since order isn't important; e.g.,

+    for (i = 0; i < reachable.Count(); i++) {

could become:

     for (i = reachable.Count() - 1; i >= 0; --i) { ... }

This way, you avoid calling Count() for each iteration. It's also a marginal win
in that most processors can compare to zero very cheaply; for example, it is
typical for a processor to set a `zero' flag bit as a side-effect of the
decrement instruction.

Comment 13

[Vidar Haarr (not reading bugmail)]

how about adding helpwanted, and perhaps nsbeta1 and mozilla1.0 ?

since this bug 'blocks' bug 114092 (implement nsChromeRegistry::UninstallPackage).

take a look at
http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeRegistry.cpp#2515
.. waterson: any progress on this ?

Comment 14

[Chase Tingley]

This bug doesn't really block UninstallPacket, it would just make it work
slightly better (without it, cruft will accumulate in chrome.rdf, as it
currently does for removal of things besides packages).  

I'm also removing bug 98392 dependency, which was optional.

I still need to clean this patch up to fix the issues Waterson pointed out.  I
will give it a go, and solicit new reviews.

Comment 15

[benc]

tever is not RDF QA anymore

Comment 16

[Axel Hecht]

WONTFIX.

Chrome registry is working different these days, or at least bsmedberg is
working it in that direction.

On top of that, if someone really needs this, we should trick the serializer
instead of doing something fuzzy with adding fake assertions into the DS.

And IMHO, this bug just timed out.