15006 - RDF container should use RDF:type to indicate its type

Status: RESOLVED INCOMPLETE

(Core Graveyard :: RDF, defect, P3)

Description

[Chris Waterson]

It incorrectly uses RDF:instanceOf right now. Fixing this will require trolling
through the code to pick up all incorrect uses.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Adding the arch keyword, since this is related to API cleanup.

Comment 2

[Chase Tingley]

Looking into this.  There's an issue with nsRDFXMLSerializer making a
distinction between containers and all other descriptions.  Since it only looks
at the properties out of the node (ArcLabelsOut(), as opposed to also looking at
the targets of those properties), it can't disinguish an rdf:type indicating a
container from any other rdf:type (and since a resource can have more than one
rdf:type, no cheating allowed...).  The easiest way to deal with this may be to
reduce the special casing for containers and just treat them like typed nodes,
at least for the purposes of serialization.  
(This should be fine, cf
http://www.w3.org/2000/03/rdf-tracking/#rdf-containers-syntax-ambiguity )

Comment 3

[Chase Tingley]

Attachment: patch

Comment 4

[Chase Tingley]

Comment on attachment 55404 [details] [diff] [review]
patch

Sorry, this diff is screwed up.

Comment 5

[Chase Tingley]

Attachment: try 2

Comment 6

[Chase Tingley]

Ok, I think this one's good.

I also pulled some redundant code from the serializer in favor of the container
utils methods.

Side note:
While testing this I noticed a (afaik) unreported bug in the RDF-XML
parsing/model building -- if I use |<rdf:type resource="http://w3.org/.../#Seq"
/>| to specify a container type, the parse state in the RDFContentSink does not
change from eRDFContentSinkState_InDescriptionElement to
eRDFContentSinkState_InContainerElement.  This causes a problem with subsequent
rdf:li data -- since the parser isn't aware that it's in a container, it treats
these like normal properties and doesn't update nextVal.  As a result,
attempting to enumerate the items in the container will return no items (since
it loops up to nextVal) -- this is evident when re-serializing the datasource.

I'll split this off, but this fix will come for free when the "a container is
just another typed node" cleanup happens, since that would presumably eliminate
the state distinction.

Comment 7

[benc]

tever is not RDF QA anymore

Comment 8

[Chase Tingley]

Attachment: unrotted/updated

This bug has been bringing shame upon my family for a hundred generations.

Comment 9

[Chase Tingley]

Comment on attachment 139534 [details] [diff] [review]
unrotted/updated

I'm ready to give this one a try.  The change in the serializer to catch
rdf:type instead of rdf:instanceOf in the IsContainerProperty() code means that
containers with other rdf:type arcs don't get serialized correctly.  However,
that's already true without this patch (bug 231546).

Comment 10

[Robert John Churchill]

Comment on attachment 139534 [details] [diff] [review]
unrotted/updated

Hmm... in the nsXULTemplateBuilder.cpp changes, when you remove the "is attr
equal to nsXULAtoms::instanceOf" check, do you need to instead replace that
with a "is it a kRDF_type" check?  (I'm unsure.)

Comment 11

[Chase Tingley]

We can't, or else we would break some template users.

This code is processing <rule> statements and converting them into nodes in the
rule network.  The instanceOf check is one of a number of cases that
short-circuit attribute parsing.  For whatever reason, waterson decided that you
shouldn't be allowed to construct a rule based on rdf:instanceOf data -- I
suspect because container structure is so intertwined with how templates work
that it would confuse things.

However, simple rules based on rdf:type are fair game, and are used by a number
of things in the tree (for instance, bookmarks code uses an rdf:type rule to
catch bookmark separators):
http://lxr.mozilla.org/seamonkey/search?string=%5C%3Crule+rdf%3Atype

That said, you're right that just removing the rdf:instanceOf case and not
replacing it with anything is changing behavior slightly -- it allows
construction of some simple rules that weren't allowed before, and I'm not
entirely sure that they would be well-behaved if anyone actually tried them.  I
can think of two ways to deal with it:
1) Add back in a line that catchs rdf:type if the value is a container type
2) Leave it as a documentation issue

I have a hunch you're going to recommend the first option :)

Comment 12

[Robert John Churchill]

Yeah, Chris and I are the individuals responsible for the XUL template stuff.  The various checks for 
special attributes are there because some attributes shouldn't be "copied" when DUP'ing nodes/
attributes from the "XUL <template> content nodes" to the "RDF->content mapping nodes".

For example, "ID" should never be DUP'ed, as you wouldn't want all RDF-generated content nodes 
to each have the ID attribute of the corresponding node from the associated XUL template node.

That said, I don't recall why "instanceOf" was a problem.  (It's been years. Doh.)

Anyway, I'm sure it was an issue at one time.  I don't know if it is an issue now.  However, unless 
you can prove that it isn't... as you guessed... yes, I'd recomment door number one.  :)

Other than that, the patch seems generally OK to me.

Comment 13

[Chase Tingley]

Comment on attachment 139534 [details] [diff] [review]
unrotted/updated

Ick, this breaks bookmarks.  I'm not sure what the specific problem is yet, but
I have a suspicion it has to do with assuming that certain resources only have
one rdf:type arc, when they now have two.

Comment 14

[Chase Tingley]

http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp#3695

This is at least part of the problem.  Current behavior is to return kNC_Folder
if the resource is a sequence, which wasn't happening with my patch (because the
call to GetTarget actually returns a value).

If I fix that, though, I can make the rule network code crash during a retract.

Comment 15

[Axel Hecht]

waterson left the building

Comment 16

[Gerard Goossen]

Attachment: fix

Besides the obvious, removes the special treatment for container types in the
serializer (only rdf:_i predicates are treated special).
Fixes comment 13 by first checking whatever it is a container.
Deals with comment 11 by leaving things as they are, i.e. ignoring
rdf:instanceOf so the rules work the same (but the result might be different
because the interpretation of datasources changes).

Comment 17

[Axel Hecht]

Comment on attachment 146817 [details] [diff] [review]
fix

This patch does not apply anymore. Sorry for letting it bitrot again. I suggest
that we really bitrot this and reanimate this once we're done with aviary. As
this changes more in bookmarks and friends than in rdf, and those files changed
quite a bit on aviary.