Closed Bug 118865 Opened 19 years ago Closed 19 years ago

Asserting an rdf int literal writes malformed xml

Categories

(Core Graveyard :: RDF, defect, P3)

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: bugzilla, Assigned: waterson)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Eww. N.B. that we'll need to add meta-information (like `moz:parseType') for
this to be parsed back as an integer.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.8
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.
>I don't see why we should.

s/should/should have to/

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.
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.
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.
Keywords: review
Blocks: 33197
Attached patch merge with harishd's changes (obsolete) — Splinter Review
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.
Attachment #64251 - Attachment is obsolete: true
Attached patch one more try. (obsolete) — Splinter Review
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'.
Attachment #64256 - Attachment is obsolete: true
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).
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.
Attached patch address tingley's comments (obsolete) — Splinter Review
Rather than overloading the naked `parseType' attribute, we'll use
`nc:parseType' instead. It costs a bit more code, but it's future-proof.
Attachment #64257 - Attachment is obsolete: true
>     // 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.
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 on attachment 64309 [details] [diff] [review]
address tingley's comments

Chris: WORKSFORME   r=rjc BTW
Attachment #64309 - Flags: review+
Use PR_FormatTimeUSEnglish instead of hand-rolled cruft.
Attachment #64309 - Attachment is obsolete: true
Comment on attachment 64400 [details] [diff] [review]
...and rjc's comments

I assume this is still r=rjc
Attachment #64400 - Flags: review+
Comment on attachment 64400 [details] [diff] [review]
...and rjc's comments

r=rjc
sr=hyatt
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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 :)
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.
Blocks: 86419
tever is not RDF QA anymore
QA Contact: tever → nobody
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.