118865 - Asserting an rdf int literal writes malformed xml

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect, P3)

Description

[Blake Ross]

I've got code that looks like this:

    nsCOMPtr<nsIRDFNode> percent;
    gRDFService->GetResource(aTargetPath, getter_AddRefs(res));
    nsresult rv = gRDFService->GetIntLiteral(someInteger, getter_AddRefs
(percent));
    if (NS_FAILED(rv)) return rv;
    rv = Assert(res, gNC_ProgressPercent, percent, PR_TRUE);

The output in the datasource file is of the form

<NC:ProgressPercent  </RDF:Description>

That's it.  The correct output, of course, is 
<NC:ProgressPercent>someInteger</NC:ProgressPercent>.

Let's see if I can get an attachable testcase together.

Comment 1

[Chris Waterson]

Eww. N.B. that we'll need to add meta-information (like `moz:parseType') for
this to be parsed back as an integer.

Comment 2

[Chase Tingley]

I don't see why we should.  Why not just have SerializeChildAssertion() QI to
different node types and print accordingly?
i.e. expand on
http://lxr.mozilla.org/seamonkey/source/rdf/base/src/nsRDFXMLSerializer.cpp#388

(in fact there's an NS_ERROR just below that to this effect.)

Unless something trickier is at work here.

Comment 3

[Chase Tingley]

>I don't see why we should.

s/should/should have to/

Comment 4

[Chris Waterson]

The reason we need a `parseType' attribute is so that we will reconstitute an
nsIRDFInt object when we read this back in. (Otherwise, we'll just parse it back
as a string.)

We might as well do dates while we're at it, too.

Comment 5

[Chase Tingley]

D'oh, you're right.  I should learn to read comments :\

eww.

Time to go hunt around and see if there's any precedent for simple typing in
RDF.  I have a feeling I may have deleted a lot of www-rdf-interest messages on
the subject at one point.

Comment 6

[Chris Waterson]

Attachment: add support for parsing date and int literals

This patch adds support for parsing date and integer literals, as well as
`RDF:parseType="resource"'. Yeah, I'm evil: I'm using the `RDF:parseType'
attribute to get this done. But I figure that's what it's there for.

I need to update to pick up harishd's changes: may need to post another patch
if there are merge conflicts.

Comment 7

[Chris Waterson]

Attachment: merge with harishd's changes

Sho'nuff, had some conflicts after harishd's landing. This patch merges those,
removes some unused code, and capitalizes the parseType attribute values
(`Resource', `Integer', `Date'), paying lip-service to the spec.

Comment 8

[Chris Waterson]

Attachment: one more try.

One more try. I figure we might as well not emit invalid RDF if someone later
adds a new node type (like nsIRDFBlob). Fixed up <RDF:li> emits to include
`parseType' when appropriate. Emit `parseType' instead of `RDF:parseType'.

Comment 9

[Chase Tingley]

We probably shouldn't be using a naked "parseType" or a "parseType" in the RDF 
namespace, since it would be spec violation to use values for it other 
than "Literal" or "Resource" (and might horribly confuse other parsers if they 
ever tried to read our emitted RDF).

Comment 10

[Chris Waterson]

damn you, tingley! ;-)

You are, of course, right. I was being lazy. I'll go the extra mile and move
Integer and Date to the `http://home.netscape.com/NC-rdf#' namespace.

Comment 11

[Chris Waterson]

Attachment: address tingley's comments

Rather than overloading the naked `parseType' attribute, we'll use
`nc:parseType' instead. It costs a bit more code, but it's future-proof.

Comment 12

[Robert John Churchill]

>     // XXX it doesn't support nsIRDFResource _or_ nsIRDFLiteral???
>     // We should serialize nsIRDFInt, nsIRDFDate, etc...

Perhaps delete the above comments?



> static const char *gMonths[] = {
> static const char *gDays[] = {
> rdf_FormatDate

Isn't there something in either NSPR (prtime.c and family) or I18N that could
help with this?  Its kind of a bummer to have hardcoded names for months and
days, since they can't be internationalized/localized.

Comment 13

[Chris Waterson]

I'll fix the comments. Thanks for pointing that out.

I could use PR_FormatTime[USEnglish]; I didn't because of the NO_NSPR_10_SUPPORT
define -- I guess it really doesn't matter. I'll change it. I couldn't find any
routines to _parse_ dates in I18n, otherwise I'd have used those. But, FWIW, I'm
not worried about localizing these dates: this is the format that we use to
_save_ data after it's been read once -- note that dates are always saved as
GMT, which isn't exactly user-friendly. (In fact, I was tempted to output them
as seconds from the Unix epoch, but didn't just so debugging would be easier
down the road.)

Comment 14

[Robert John Churchill]

Comment on attachment 64309 [details] [diff] [review]
address tingley's comments

Chris: WORKSFORME   r=rjc BTW

Comment 15

[Chris Waterson]

Attachment: ...and rjc's comments

Use PR_FormatTimeUSEnglish instead of hand-rolled cruft.

Comment 16

[Chris Waterson]

Comment on attachment 64400 [details] [diff] [review]
...and rjc's comments

I assume this is still r=rjc

Comment 17

[Robert John Churchill]

Comment on attachment 64400 [details] [diff] [review]
...and rjc's comments

r=rjc

Comment 18

[David Hyatt]

sr=hyatt

Comment 19

[Chris Waterson]

Fix checked in.

Comment 20

[Chase Tingley]

I hate to be a bastard about process, but the portions of this checkin that 
fixed bug 33197 were never posted here as a patch and AFAIK not reviewed.  Did 
rjc get a chance to look at them before they went in?

I'm probably totally off-base, but I'm slightly confused.  Sorry for the bother 
in advance :)

Comment 21

[Chris Waterson]

Yeah, you're right. :-( I was maintaining two sets of diffs (to different sets
of files) in the same tree, and didn't include the changes to
nsRDFContentSink.cpp in either set. (And that was the whole point of posting the
``merge with harishd's changes'' patch in attachment 64256 [details] [diff] [review]!)

Here they are, for the record. If you have any post hoc suggestions, we can
discuss them.

Comment 22

[Chris Waterson]

Attachment: diffs to nsRDFContentSink.cpp

Comment 23

[benc]

tever is not RDF QA anymore