Closed Bug 115926 Opened 18 years ago Closed 18 years ago

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

Categories

(Core Graveyard :: RDF, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.8

People

(Reporter: nhottanscp, Assigned: waterson)

Details

(Keywords: intl)

Attachments

(1 file, 2 obsolete files)

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.
cc to people in other bug.

I assume there are two separate implementations, nsXULSortService and
nsXULOutlinerBuilder. Would it be better to separate the bug?
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.
Also need to add the binary data type to nsIRDFLiteral.
http://lxr.mozilla.org/seamonkey/source/rdf/base/idl/nsIRDFLiteral.idl
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.
No, I am not familiar with the code, reassign to waterson.
Assignee: nhotta → waterson
Keywords: intl
Blocks: 115071
Attached patch proposed fix (obsolete) — Splinter Review
Introduce a new literal type (called a `blob' for lack of a better name) that
can hold arbitrary binary data.
rjc, tingley: could you take a look?
Status: NEW → ASSIGNED
Keywords: patch
Priority: -- → P3
Target Milestone: --- → mozilla0.9.8
Attached patch after landing patch in 114882 (obsolete) — Splinter Review
Re-merged after landing patch for bug 114882.
Attachment #62319 - Attachment is obsolete: true
The previous diff was too narrow.
Attachment #62443 - Attachment is obsolete: true
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?
I think this will allow nsIRDFBlobs to be transported opaquely through script,
which might be useful.
So with the patch, can the client set a binary collation key instead of wstring?
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.)
Blocks: 116329
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).
No longer blocks: 116329
Comment on attachment 62444 [details] [diff] [review]
with changes in mozilla/rdf/base/idl, too.

r=tingley
Attachment #62444 - Flags: review+
>  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];

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.
Keywords: patchreview
Put nsbeta1, because bug 115071 needs this.
Keywords: nsbeta1
sr=hyatt
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
No longer blocks: 115071
verified
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.