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

VERIFIED FIXED in mozilla0.9.8

Status

P3
normal
VERIFIED FIXED
17 years ago
14 days ago

People

(Reporter: nhottanscp, Assigned: waterson)

Tracking

({intl})

Trunk
mozilla0.9.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

17 years ago
cc to people in other bug.

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

Comment 2

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

Comment 3

17 years ago
Also need to add the binary data type to nsIRDFLiteral.
http://lxr.mozilla.org/seamonkey/source/rdf/base/idl/nsIRDFLiteral.idl
(Assignee)

Comment 4

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

Comment 5

17 years ago
No, I am not familiar with the code, reassign to waterson.
Assignee: nhotta → waterson
Keywords: intl
(Reporter)

Updated

17 years ago
Blocks: 115071
(Assignee)

Comment 6

17 years ago
Created attachment 62319 [details] [diff] [review]
proposed fix

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

Comment 7

17 years ago
rjc, tingley: could you take a look?
Status: NEW → ASSIGNED
Keywords: patch
Priority: -- → P3
Target Milestone: --- → mozilla0.9.8
(Assignee)

Comment 8

17 years ago
Created attachment 62443 [details] [diff] [review]
after landing patch in 114882

Re-merged after landing patch for bug 114882.
Attachment #62319 - Attachment is obsolete: true
(Assignee)

Comment 9

17 years ago
Created attachment 62444 [details] [diff] [review]
with changes in mozilla/rdf/base/idl, too.

The previous diff was too narrow.
Attachment #62443 - Attachment is obsolete: true

Comment 10

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

Comment 11

17 years ago
I think this will allow nsIRDFBlobs to be transported opaquely through script,
which might be useful.
(Reporter)

Comment 12

17 years ago
So with the patch, can the client set a binary collation key instead of wstring?
(Assignee)

Comment 13

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

Updated

17 years ago
Blocks: 116329
(Assignee)

Comment 14

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

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

Updated

17 years ago
Keywords: patch → review
(Reporter)

Comment 18

17 years ago
Put nsbeta1, because bug 115071 needs this.
Keywords: nsbeta1

Comment 19

17 years ago
sr=hyatt
(Assignee)

Comment 20

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

Updated

17 years ago
No longer blocks: 115071

Comment 21

17 years ago
verified
Status: RESOLVED → VERIFIED

Updated

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