272166 - nsIWindowsShellService::getRegistryEntry does not support non-latin1 keys/values

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

79     string getRegistryEntry(in long aHKeyConstant, in string aSubKeyName, in
string aValueName);

This means that only latin1 is supported. Worse, on a non-latin1 (guess that's
really a non-windows1252) system, the characters will be interpreted as latin1,
likely leading to garbage for non-ascii characters.

this should use wstring or AString in the IDL. seamonkey has the same bug, which
I will file in a second.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Bug 272167 is the seamonkey version.

Comment 2

[Jesse Ruderman]

Is the new registry interface from bug 292981 better in this respect?

Comment 3

[Jungshik Shin]

Yes, it is. Shouldn't we get rid of broken methods in this interface now that we
have a better interface?

Comment 4

[Darin Fisher]

Yes.  I really want to go through the tree and change all consumers over to
using the new registry API.

Comment 5

[Jungshik Shin]

Attachment: patch to get rid of it

turned out that there's no consumer :-)

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 184218 [details] [diff] [review]
patch to get rid of it

ben's so far behind on reviews right now it isn't funny.  We added this from
winhooks because extensions used it, removing this will break Launchy and other
extensions that are relying on it.

r=me assuming that someone writes me a quick summary for the release notes, and
link to something that explains the new interface.

Email is probably best for those changes.

Comment 7

[Jungshik Shin]

Release note entry:

nsIWindowsShellService::getRegistryEntry was removed because it does not support
handling of non-ASCII characters. |open| and |readStringValue| methods of a new
and better interface |nsIWindowsRegKey| can be used. For more details on the
interface including the descriptions of other methods, see
xpcom/ds/nsIWindowsRegKey.idl

I'll send this to Mike

 

Comment 8

[Darin Fisher]

Thanks for writing up that release note jshin!  Maybe add the ContractID to that
note since the interface file doesn't document it.

Comment 9

[Jungshik Shin]

Comment on attachment 184218 [details] [diff] [review]
patch to get rid of it

Probably too late 1.1a1. (I don't mind this being shifted to 1.1a2) This is a
very low risk trivial patch except that a couple of extensions use the
inferface being removed. I've provided a draft for the release notes.

Comment 10

[Mike Connor [:mconnor]]

http://www.mozilla.org/projects/deerpark/new-extension-dev-features.html has
this, and gemal was the main consumer (and the reason it was added in the first
place) so I'm sure this should be a huge fallout.  I'd like to take this now
instead of later, its not a risk to our builds, it just might break older revs
of extensions, but sooner is better than later on that count.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 184218 [details] [diff] [review]
patch to get rid of it

a=asa

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

maybe the release note should also document whether to use createInstance or
getService?

Comment 13

[Darin Fisher]

biesi: the release notes are doctor'able... please feel free to edit the entry
for this.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, done.

Comment 15

[Henrik Gemal]

I've created a new bug https://bugzilla.mozilla.org/show_bug.cgi?id=295360 with
some feedback to the new registry system

Comment 16

[Jungshik Shin]

sorry forgot to mark as fixed. fix landed on the trunk