97969 - nsFontMetrics{GTK,Xlib} use FMR-able nsCStringKey construction pattern

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I wrote in review comments on bug 95621 (this applies to both
nsFontMetricsGTK.cpp and nsFontMetricsXlib.cpp):

  * aFFREName should be |const nsACString&|, not |nsAWritableCString&|
  * the nsCStringKey constructed from aFFREName in TryNode should be constructed
without PromiseFlatCString (or .get()), since this code would crash if the
string isn't already flat (although it is).
  * in the code that's actually in the patch, you should use NS_LITERAL_CSTRING
to pass the string to TryNode, rather than constructing an nsCAutoString
(expensive) and copying the buffer (expensive)

I'm particularly worried about the second point, since the pattern you use is
one that would crash if used on a non-flat string.  (Yes, ns[C]StringKey needs
to be brought into the modern world once our main string implementation is
sharable, but we're better off keeping it the quirky way it is now until then. 
Either way, you're better off just passing the string directly without the
promise since ns[C]StringKey would take care of things just as well after any
such rewrite if you let it handle things rather than doing them yourself.)

The third point could also be a bit of a performance gain.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Although, actually, if you want the best performance, you'd be better making
aFFREName an |const nsAFlatString&| so that you can get the nsCStringKey
constructor that has better performance (based on static typing determination of
flatness).

Comment 2

[Roland Mainz]

Attachment: Prototype patch for Xlib toolkit

Comment 3

[Roland Mainz]

Filed prototype patch.
I am not sure whether all things are "right"... ;-/

Comments ?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No.  You added another copy rather than removing the one that was already there.
 You need to change the type of the string passed to TryNode as well, and use
NS_LITERAL_CSTRING at the caller of TryNode (rather than nsCAutoString).  (You
should also change |nsAWritableString &oPattern| to |nsAString &oPattern|, since
nsAWritableString is deprecated.)

Comment 5

[kill this account]

Scott, I do not see any mention of ns[C]StringKey in

  http://lxr.mozilla.org/seamonkey/source/string/doc/string-guide.html

Care to elaborate?

Comment 6

[kill this account]

Scott, are you there?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What is it that you need to know?

Comment 8

[kill this account]

I would like to see some documentation to go along with the changes 
scc made to the string API.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's odd here isn't the string API -- it's nsCStringKey, which, when
constructed on the stack (with OWN_CLONE, the default ownership model), expects
the string it's given to last longer than it does.

Comment 10

[kill this account]

--> ftang

Comment 11

[Frank Tang]

bulk move NEW FUTURE bug to ASSIGN

Comment 12

[Frank Tang]

what a hack. I have not touch mozilla code for 2 years. I didn't read these bugs
for 2 years. And they are still there. Just close them as won't fix to clean up.

Comment 13

[Travis Chase]

Mass Re-open of Frank Tangs Won't fix debacle. Spam is his responsibility not my own

Comment 14

[Travis Chase]

Mass Re-assinging Frank Tangs old bugs that he closed won't fix and had to be
re-open. Spam is his fault not my own

Comment 15

[Peter Weilbacher]

we don't have nsFontMetrics{GTK,Xlib} any more --> WONTFIX