Asserting an rdf int literal writes malformed xml

RESOLVED FIXED in mozilla0.9.8

Status

P3
normal
RESOLVED FIXED
17 years ago
16 days ago

People

(Reporter: bugzilla, Assigned: waterson)

Tracking

Trunk
mozilla0.9.8
x86
Windows 2000
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
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

Comment 2

17 years ago
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

17 years ago
>I don't see why we should.

s/should/should have to/

(Assignee)

Comment 4

17 years ago
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

17 years ago
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.
(Assignee)

Comment 6

17 years ago
Created attachment 64251 [details] [diff] [review]
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.
(Assignee)

Updated

17 years ago
Keywords: review
(Assignee)

Updated

17 years ago
Blocks: 33197
(Assignee)

Comment 7

17 years ago
Created attachment 64256 [details] [diff] [review]
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.
Attachment #64251 - Attachment is obsolete: true
(Assignee)

Comment 8

17 years ago
Created attachment 64257 [details] [diff] [review]
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'.
Attachment #64256 - Attachment is obsolete: true

Comment 9

17 years ago
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).
(Assignee)

Comment 10

17 years ago
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.
(Assignee)

Comment 11

17 years ago
Created attachment 64309 [details] [diff] [review]
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.
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.
(Assignee)

Comment 13

17 years ago
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+
(Assignee)

Comment 15

17 years ago
Created attachment 64400 [details] [diff] [review]
...and rjc's comments

Use PR_FormatTimeUSEnglish instead of hand-rolled cruft.
Attachment #64309 - Attachment is obsolete: true
(Assignee)

Comment 16

17 years ago
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

Comment 18

17 years ago
sr=hyatt
(Assignee)

Comment 19

17 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 20

17 years ago
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 :)
(Assignee)

Comment 21

17 years ago
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.
(Assignee)

Comment 22

17 years ago
Created attachment 64977 [details] [diff] [review]
diffs to nsRDFContentSink.cpp
(Assignee)

Updated

17 years ago
Blocks: 86419

Comment 23

15 years ago
tever is not RDF QA anymore
QA Contact: tever → nobody

Updated

16 days ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.