115926 - collation keys assumed to be null terminated, but are really binary data

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect, P3)

Description

[nhottanscp]

Separated from bug 114070.
A collation key is a binary data but the current code assuming it as null
terminated Unicode string (PRUnichar*). This need to change and also needs
length because it cannot rely on the null terminator.
MacOS X collator generates keys include 0x0000.

Comment 1

[nhottanscp]

cc to people in other bug.

I assume there are two separate implementations, nsXULSortService and
nsXULOutlinerBuilder. Would it be better to separate the bug?

Comment 2

[nhottanscp]

There is no binary data support in nsIRDFService. 
http://lxr.mozilla.org/seamonkey/source/rdf/base/idl/nsIRDFService.idl#62
Need binary data support so it can used to set a collation key.

Comment 3

[nhottanscp]

Also need to add the binary data type to nsIRDFLiteral.
http://lxr.mozilla.org/seamonkey/source/rdf/base/idl/nsIRDFLiteral.idl

Comment 4

[Chris Waterson]

Sounds reasonable. We should be able to cover that work under this bug. Nhotta,
are you gonna give it a whirl? If not, reassign to me or tingley.

Comment 5

[nhottanscp]

No, I am not familiar with the code, reassign to waterson.

Comment 6

[Chris Waterson]

Attachment: proposed fix

Introduce a new literal type (called a `blob' for lack of a better name) that
can hold arbitrary binary data.

Comment 7

[Chris Waterson]

rjc, tingley: could you take a look?

Comment 8

[Chris Waterson]

Attachment: after landing patch in 114882

Re-merged after landing patch for bug 114882.

Comment 9

[Chris Waterson]

Attachment: with changes in mozilla/rdf/base/idl, too.

The previous diff was too narrow.

Comment 10

[Chase Tingley]

XPCOM etiquette question -- since neither the "meat" of nsIRDFBlob (the binary
data) nor the interface's getter are scriptable, is there any point to making
the interface itself scriptable?

Comment 11

[Chris Waterson]

I think this will allow nsIRDFBlobs to be transported opaquely through script,
which might be useful.

Comment 12

[nhottanscp]

So with the patch, can the client set a binary collation key instead of wstring?

Comment 13

[Chris Waterson]

I think there's a bit of confusion about how collation works with the template
builder and RDF.

The template builder gets _all_ of its information from the datasource using the
methods of nsIRDFDataSource like GetTargets, GetTarget, ArcLabelsOut, etc. The
template builder (or the XUL sort service, acting on behalf of the template
builder), will look for certain properties in the RDF datasource to get a clue
as to how it should sort things.

Say one of the displayed values is retrieved using the RDF property
`http://home.netscape.com/NC-rdf#name'. The template builder (or the sort
service, acting on behalf of the template builder) will look for a property
called `http://home.netscape.com/NC-rdf#name?collation=true'. If it finds a
value for this property (using nsIRDFDataSource::GetTarget), then this value is
used for sorting purposes. If not, then the value of the display property
`http://home.netscape.com/NC-rdf#name' is used.

So, with this patch, what you're calling a ``client'' (really, an RDF
datasource) can return an nsIRDFBlob object as the collation key when the
template builder (or the XUL sort service, acting on its behalf) asks for the
`...?collation=true' property. We'll need to do a bit more work on the sort
service to make it pay attention to nsIRDFBlobs, but that work is trivial.

(rjc, please feel free to jump in and correct me if I've said anything wrong.)

Comment 14

[Chris Waterson]

Okay, bug 116328 deals with implementing collation-based sorting in the XUL
outliner builder.

Bug 116329 deals with adding support for nsIRDFBlob objects in the XUL sort
service (nhotta: I _think_ this may be what you meant by bug 114070).

Comment 15

[Chase Tingley]

Comment on attachment 62444 [details] [diff] [review]
with changes in mozilla/rdf/base/idl, too.

r=tingley

Comment 16

[Robert John Churchill]

>  rjc, please feel free to jump in and correct me if I've said anything wrong.

Chris:  what you described re: the XULSortService sounds like what I remember. :)


Hmm... in BlobImpl(PRUint8 *aBytes, PRInt32 aLength) {...}, should fail
gracefully if the following allocation fails:
       mData.mBytes = new PRUint8[aLength];

Comment 17

[Robert John Churchill]

BTW, I meant to mention that blobs are the right way to go;  year(s) ago, a
workaround for binary data issues was the following scenario:

o  XULSortService asks "?collation=true" across the graph
o  if a datasource wanted to return a collation key with binary data, it could
simply encode the binary data into a intermediate text format, then return that;
 the collation key comparison routines just had to be made to recognize encoded
data and decode it before doing the work.

Note: the encoding/decoding would indeed be overhead;  one of the reasons why
blobs are a nicer answer.

Comment 18

[nhottanscp]

Put nsbeta1, because bug 115071 needs this.

Comment 19

[David Hyatt]

sr=hyatt

Comment 20

[Chris Waterson]

Fix checked in.

Comment 21

[Tom Everingham]

verified