Closed Bug 292981 Opened 19 years ago Closed 19 years ago

Provide a scriptable interface to the Windows registry (nsIWindowsRegKey)

Categories

(Core :: XPCOM, enhancement, P1)

Product:
Core
Component:
XPCOM
Platform:
x86
Windows XP
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8beta3

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

Attachments

(1 file, 4 obsolete files)

v1.1 patch
44.40 KB, patch
neil
: review+
dougt
: superreview+
chofmann
: approval1.8b2+
chofmann
: approval1.8b3+
Details | Diff | Splinter Review 
Provide a scriptable interface to the Windows registry

Like it or not there are extensions out there that have need to interface the
Windows registry.  They usually have to write native code to do this, when they
really ought to be able to just do it from JS.

Moreover, we also have need for Windows registry manipulations in various places
through Firefox and Mozilla in general.  For example, it'd be nice to create an
extension manager install location based on the Windows registry, spec'd here:
http://wiki.mozilla.org/Extension_Manager:Win32_Registry_Location

We also have need to access the Windows registry in other contexts as well, and
for a variety of reasons the Win32 registry API is not easy to use (e.g., buffer
sizing and i18n issues).

Right now, there's the ability to read from the registry via
nsIWindowsShellService, but that is Firefox only and it is very limited in what
it provides.  I'd prefer an interface that represents a Win32 registry key.
Attached file prototype: nsIWindowsRegKey.idl (obsolete) — 
This is a prototype of the interface I'm considering.

An implementation of it would be available via ContractID and via
NS_NewWindowsRegKey so that it may be used prior to XPCOM initialization.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta3
Comment on attachment 182659 [details]
prototype: nsIWindowsRegKey.idl


>   * This method reads the binary contents of the named value under this key.
>   */
>  ACString readValue(in string name);

If this is for binary content, shouldn't we use an octet array? 

>   * This method reads the ANSI string contents of the named value.  The string
>   * is encoded using the ANSI codepage, and may be converted to Unicode using
>   * NS_CopyNativeToUnicode.
>   *
>   * NOTE: JS code should generally use readValueUTF16 instead of this method.
>   */
>  ACString readValueANSI(in string name);

I think either we have to use an octet array as [out] param or	just make it
not scriptable. Script users can always  use readValueUTF16 and
|nsIScriptableUnicodeConverter|, I guess.


Btw, are all value names always in ASCII? It seems like they're ...
jshin: thanks for the quick feedback!


> >  ACString readValue(in string name);
> 
> If this is for binary content, shouldn't we use an octet array? 

I had it coded using an octet array and resulting length parameter, but then I
realized (based on the discussions over in the nsICryptoHash bug) that ACString
is fine for binary data.  Sure, XPConnect will null pad expand it, but JS
consumers can still iterate over the "octets" even though they are really
Unicode values.  Plus, ACString supports internal null bytes, and it caries a
length parameter.  It is the ideal container.  The DOM already has an API that
works like this: nsIDOMWindowInternal::atob.  That one uses AString, but it's
basically the same idea.


> >  ACString readValueANSI(in string name);
> 
> I think either we have to use an octet array as [out] param or	just make it
> not scriptable. Script users can always  use readValueUTF16 and
> |nsIScriptableUnicodeConverter|, I guess.

Yeah, I also nearly marked this [noscript] (as with the nsIFile "native"
methods).  I have no problem doing that here.  The only reason I didn't do it is
because of the reasons I gave above.  The string could be used by JS provided JS
consumers know what they are doing.  Maybe it could lead to too much harm.


> Btw, are all value names always in ASCII? It seems like they're ...

Yeah... I thought about that too.  So, value names and key names can be Unicode,
eh?  Guess so... perhaps I should use AString for all of these value and key
names instead, OK.


So, I was thinking that I would internally try to use the 'W' unicode methods
and failover to the 'A' versions if necessary.  I know that we have been talking
about using MSLU (or MZLU), but I don't want to be blocked by that stalled effort.
(In reply to comment #3)

> > >  ACString readValue(in string name);
> > 
> > If this is for binary content, shouldn't we use an octet array? 
> 
> I had it coded using an octet array and resulting length parameter, but then I
> realized (based on the discussions over in the nsICryptoHash bug) that ACString
> is fine for binary data.  snip ... It is the ideal container.  

Thanks for the explanation. What do you think of renaming it 'readValueBinary'?
Btw, one minor drawback is that in a debug build, we'll get a warning from
XPConnect. 


> > >  ACString readValueANSI(in string name);
> > 
> > I think either we have to use an octet array as [out] param or	just make it
> > not scriptable. Script users can always  use readValueUTF16 and
> > |nsIScriptableUnicodeConverter|, I guess.
> 
> The string could be used by JS provided JS consumers know what they are doing. 

 That's true, but there are some clueless 'users' so that either we have to
force them to do the right thing by making them convert explicitly (with
nsIScriptableU..Converter) or make very clear what they have to do if they want
to use this API.

> > Btw, are all value names always in ASCII? It seems like they're ...
> 
> Yeah... I thought about that too.  So, value names and key names can be Unicode,
> eh?  Guess so... perhaps I should use AString for all of these value and key
> names instead, OK.
> 
> 
> So, I was thinking that I would internally try to use the 'W' unicode methods
> and failover to the 'A' versions if necessary.  I know that we have been talking
> about using MSLU (or MZLU), but I don't want to be blocked by that stalled effort.

I guess there are two options here: 1. just go ahead with what you have for now
and convert key/value params to AString later when we're ready for MSLU/MZLU 2.
convert key/value params to AString now and rely on the 'manual' A/W API
switching (this would result in more code)


I'd go with ACString readValueBinary(in wstring name);

What about querying the type of a value?
fwiw code like win/nsOSHelperAppService.cpp already has code to use W APIs,
falling back to A ones.

> ROOT_KEY_CLASSES_ROOT

should those use the windows naming (HKEY_CLASSES_ROOT)?

>   * The native HKEY value as an unsigned 32-bit value.
>   */
>  attribute unsigned long nativeKey;

Is that true on 64-bit platforms too?

>   * This method closes the key.  If the key is already called, then this

called -> closed?


Should there be an enumerator for the subkeys maybe?
Thanks for all the great feedback on this interface...


Yeah, I'll go with readValueBinary.  The ASSERTION is troubling.  We should
probably disable that.

I'll make the ANSI API noscript.

I'll convert key/value params to AString now.  I don't want to have to rev the
interface later.  I'll try to do manual A/W API switching.

> What about querying the type of a value?

Ah, yes... that would be good to add, but I'm not sure if I want to expose the
raw type values used by the registry or define my own set: Binary, String, and
Integer.  This API only supports those three types, so maybe we should have a
type getter that maps to one of those three type constants.(In reply to comment 


> > ROOT_KEY_CLASSES_ROOT
> 
> should those use the windows naming (HKEY_CLASSES_ROOT)?

HKEY_CLASSES_ROOT is a #define, so I cannot use it here.


> >   * The native HKEY value as an unsigned 32-bit value.
> >   */
> >  attribute unsigned long nativeKey;
> 
> Is that true on 64-bit platforms too?

No, probably not.  We should probably care about Win64.  OK, I'll make the
nativeKey getter be a noscript HKEY getter.  I had wanted to give people
(script) a way to open a new key relative to another key, so perhaps I'll just
expose an API for that here.


> 
> >   * This method closes the key.  If the key is already called, then this
> 
> called -> closed?

yes, thx


> Should there be an enumerator for the subkeys maybe?

That's what the childCount and getChildName stuff is all about.  It allows
people to enumerate subkeys.  But, yeah... perhaps a nsISimpleEnumerator that
returns nsIWindowsRegKey objects would be even more convenient.  Then, I'd
probably want to have an attribute on nsIWindowsRegKey to expose the key's name.
(In reply to comment #7)
> The ASSERTION is troubling.  We should probably disable that.

This is probably a bit crazy idea. Nonetheless, let me try. How about adding a
new xpidl type for 'binary' which is virtually identical to ACString (mapped to
nsACString in cpp, etc) except that it doesn't assert crossing the XPConnect
boundary. In a sense, it's similar to AUTF8String except that its difference
with ACString is a lot more trivial than AUTF8String. 
iirc we're running low on xpidl flags, so adding new ones always scares people.
(In reply to comment #7)
>But, yeah... perhaps a nsISimpleEnumerator that
>returns nsIWindowsRegKey objects would be even more convenient.
No it wouldn't, because it would require opening the keys.

(In reply to comment #9)
>iirc we're running low on xpidl flags, so adding new ones always scares people.
jshin said type, not flag?
these magical types are implemented as flags
I'd probably add a method to return a relative registry key. So if you have a 
key object, you ask it for key "X" and and it returns you another key object 
that represents registry key "X" under the original key.

Would variants make things cleaner here from a JS standpoint?
Yeah, I like the opening a relative key idea.  Let's not try to add a new XPIDL
type if we can help it.  I think using ACString is fine.

Reading xpcconvert.cpp indicates that 1) STRICT_CHECK_OF_UNICODE is not defined,
and 2) it only applies to T_CHAR and T_CHAR_STR.
sounds like getPrefBranch :-)

The problem with ACString is, imho, that people may well try to interpret the
data as a string, and that is probably not such a good idea, since it'll be
treated like a latin1 string, which is probably wrong often...
> sounds like getPrefBranch :-)

No, this interface is meant to tighly bound to the Win32 API so that people can
leverage this in place of the Win32 API.


> The problem with ACString is, imho, that people may well try to interpret the
> data as a string, and that is probably not such a good idea, since it'll be
> treated like a latin1 string, which is probably wrong often...

So, that's where documentation comes in and changing the name to readValueBinary.
OK, this interface reflects many of the suggestions made in this bug.  I've
also modified the TYPE and ACCESS flags to directly correspond to the values
defined in the Platform SDK.  The reason for doing that is that it enables this
interface to be used with types and access modes that are not explicitly
defined on this interface.  I'm not sure what it will mean if someone tried to
compile Mozilla under 64-bit Windows.  From the registry documentation it seems
like there is a different 64-bit registry altogether, so maybe we could define
a different interface for 64-bit Windows... I'm really not sure.  At any rate,
I don't think it is that important to worry about 64-bit Windows.

This new interface also does away with the read/write ANSI string functions.  I
just don't think those are all that useful given that the registry stores
strings as Unicode anyways.  C++ callers that really need ANSI strings can call
NS_CopyUnicodeToNative, which would be equivalent to calling the 'A' versions
of the registry functions.

I've also renamed read/writeBinaryValue to read/writeRawValue, and I've added a
type parameter to those functions.  The readRawValue returning a type is just a
convenience so callers do not also have to call getValueType.

Also, there are no explicit helper functions for TYPE_EXPAND_SZ and
TYPE_MULTI_SZ.	We could add those I suppose.  Thoughts?

Finally, I did not bother adding an enumeration API since it could easily be
built on top of this API, and moreover most consumers would probably prefer
using the more direct index-based API (that happens to map directly to
RegEnumKeyEx anyways).
Attachment #182659 - Attachment is obsolete: true
Any other comments?  Are people happy with this interface?  Shall I proceed to
implement this puppy? ;-)
Comment on attachment 182746 [details]
revised prototype: nsIWindowsRegKey.idl

global questions:

@throw what? (don't forget ACCESS_DENIED, i tend to lock keys against everyone)
do you want to use DOMString?
why not use AUTF8String?

>  [noscript] attribute HKEY key;
>  /**
>   * This method closes the key.  If the key is already been closed, then this
>   * method does nothing.
What will the value of attribute key be after you call close()?
What happens if I use w32api to call the registry close api w/ attribute key?
>   */
>  void close();

>   * This method starts watching the key to see if any of its values have
>   * changed.  The key must have been opened with mode including ACCESS_NOTIFY.
what happens if things are changed? i didn't see an observer. i like the
nsIPrefBranch/nsIObserver system.
>   */
>  void startWatching();

>   * This method returns true if the key has changed and false otherwise.
>   * This method will always return false if startWatching was not called.
this seems like a fairly silly approach.
>  boolean hasChanged();
> @throw what? (don't forget ACCESS_DENIED, i tend to lock keys against everyone)

Yeah, I need to go over the interface and document exceptions.  I wait to do
that once I start impleemnting.


> do you want to use DOMString?
> why not use AUTF8String?

Because the registry stores its strings using double-byte encoding, so exposing
that encoding seems most optimal.


> >  [noscript] attribute HKEY key;
> >  /**
> >   * This method closes the key.  If the key is already been closed, then this
> >   * method does nothing.
> What will the value of attribute key be after you call close()?

After calling close, the key will be null'd out.  I'll document that.


> What happens if I use w32api to call the registry close api w/ attribute key?

Then you had better call SetKey(NULL), which will not close the key.  I'll 
document that since it could be abused.



> >   * This method starts watching the key to see if any of its values have
> >   * changed.  The key must have been opened with mode including ACCESS_NOTIFY.
> what happens if things are changed? i didn't see an observer. i like the
> nsIPrefBranch/nsIObserver system.

the idea is that you would check HasChanged on your reg key to determine quickly
(and efficiently) if you should read the value from the registry.  i like the
pref branch API as well, but that would require polling the registry.  we could
implement that as an add-on (separate interface) to this API if needed.


> >   * This method returns true if the key has changed and false otherwise.
> >   * This method will always return false if startWatching was not called.
> this seems like a fairly silly approach.
> >  boolean hasChanged();

would you prefer it throw an exception?
oh, that was a reference back to not using nsIObsever ala prefchanged.

perhaps the thing to do is to off AddWatch(arrayof items) and a magic "*" for
everything, so that on change we check the items that the user watched, and only
tell the user if the stuff they care about changed. but yeah, perhaps a wrapper
for that would be fine.
Yeah, a wrapper would be fine.  This API is meant to be a thin layer around the
Win32 API.
Attached patch v0 prototype patch (obsolete) — — Splinter Review 
This is a partially complete patch.
Attachment #182746 - Attachment is obsolete: true
Comment on attachment 182976 [details] [diff] [review]
v0 prototype patch

>+  // Unicode supported on NT 3.5, 4.0, 5.0 and above.
>+  BOOL isUnicode = (osvi.dwMajorVersion >= 5 ||
>+                    (osvi.dwMajorVersion == 4 &&
>+                     osvi.dwMinorVersion == 0 &&
>+                     osvi.dwPlatformId == VER_PLATFORM_WIN32_NT) ||
>+                    osvi.dwMajorVersion == 3);
The test for VER_PLATFORM_WIN32_NT suffices.

>+  nsresult rv = child->Open((PRUint32) mKey, path, mode);
I thought the whole point of this was to avoid 64-bit issues...

I also think that it would be easier to do the string conversions inline in
nsWindowsRegKey rather than writing conversion functions.
> The test for VER_PLATFORM_WIN32_NT suffices.

does it?  will all future versions of microsoft windows set this platform id? 
perhaps it would make sense to test for this platform id as well as the major
version >= 5.


> >+  nsresult rv = child->Open((PRUint32) mKey, path, mode);
> I thought the whole point of this was to avoid 64-bit issues...

What 64-bit issues?  For 32-bit Windows, this implementation detail is fine.  My
concern was with creating an interface that would not be usable under some
64-bit version of Windows.  So long as the interface passes that test, we're happy.


> I also think that it would be easier to do the string conversions inline in
> nsWindowsRegKey rather than writing conversion functions.

Perhaps.
Attached patch v1 patch (obsolete) — — Splinter Review 
Neil, would you kindly do the review honors here.  I decided that I really
liked your last comment, and so I did away with WinReg and the 'wrapper'
methods.  Trying to implement RegQueryValueExW was already getting way out of
control.  My thinking was that that code could eventually be factored out into
MZLU, but that's perhaps just a pipe dream.  This new patch is quite a bit
simpler, and moreover it is complete :-)

I decided not to offer any support for REG_MULTI_SZ and REG_EXPAND_SZ in this
patch.	Does that seem reasonable to everyone?

I also removed support for recursively deleting subkeys.  To implement that I
would prefer to use SHDeleteKey, but that may not be available on some very old
Windows systems.  If we have need for that API, then we can add it back. 
Without it, folks will just have to enumerate and delete subkeys manually.

I still need to flesh out TestWinReg.js a bit more, but that need not hold up
review.
Attachment #182976 - Attachment is obsolete: true
Attachment #182993 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 182993 [details] [diff] [review]
v1 patch

>+   * The native HKEY value as an unsigned 32-bit value.
Since you refer to 64-bit Windows below, maybe you should revise this?

>+// This class simplifies conversion from unicode to native charset somewhat.
Neat :-)

>+// Prototype for the Win32 RegNotifyChangeKeyValue function, which is not 
>+// available on Win95.
It turns out that the export exists even in the original 1995 version of
ADVAPI32 but it always returns ERROR_CALL_NOT_IMPLEMENTED.

>+#ifdef DEBUG
>+  if (getenv("WINREG_USE_ANSI"))
>+    sUseUnicode = PR_FALSE;
>+#endif
Might as well put this before the other tests?

>+nsWindowsRegKey::SetKey(HKEY key)
>+{
>+  mKey = key;
>+  return NS_OK;
>+}
Don't you need to StopWatching(); here too?

>+  LONG rv;
Could easily get confused with nsresult rv; which is a slightly different type.

>+  return (rv == ERROR_SUCCESS) ? NS_OK : NS_ERROR_FAILURE;
Does anyone know of any existing API error mapping code?

>+  // According to MSDN a key name is limited to 255 characters.
Should we have some #defines for these constants?

>+    PRUnichar nameBuf[16384];
Isn't that a bit large for a stack-based allocation? Not that I have any better
ideas.

>+    // This must be a string type in order to fetch the value as a string.
>+    NS_ENSURE_STATE(type == TYPE_STRING);
This should work for REG_EXPAND_SZ type too, of course it will retrieve the raw
%env% strings. For REG_MULTI_SZ I don't see why it won't work although in that
case you'll get a number of null-terminated strings which the caller will have
to deal with.

>+    rv = RegSetValueExA(mKey, PromiseNativeString(name).get(), 0, REG_SZ,
>+                        (const BYTE *) nativeValue.get(),
>+                        nativeValue.Length() + sizeof(char));
Hmm... I wonder what happens if you try to write "Reg\0Multi\0Sz\0"

>+  if (rv != ERROR_SUCCESS) {
>+    StopWatching();
>+    return NS_ERROR_FAILURE;
Again, a better mapping would be handy (or default to NS_ERROR_NOT_SUPPORTED,
which is what will happen on Windows 95).

>+NS_IMETHODIMP
>+nsWindowsRegKey::HasChanged(PRBool *result)
>+{
>+  if (!mWatchEvent) {
>+    *result = PR_FALSE;
>+  } else if (WaitForSingleObject(mWatchEvent, 0) == WAIT_OBJECT_0) {
>+    // An event only gets signaled once, then it's done, so we have to set up
>+    // another event to watch.
>+    StopWatching();
>+    StartWatching(mWatchRecursive);
>+  }
Where do you assign to *result when you have an event?

>+		TestCreateUnique.cpp \
>+		TestMoveTo.cpp \
From another patch I think ;-)
Good feedback, thanks.  I'll post an updated patch shortly.

The stack allocation of 16384 comes straight out of a MSDN code sample.  Since
the function isn't defined to issue any explicit error code when the name buffer
is too small, I'm forced (I think) to preallocate the maximum size.  I could
probably use RegQueryInfoKey to figure out what the largest value name is and
preallocate that size instead.  Do you think that approach is better?
Am I right to think that you didn't bother to use function pointers (a la
widget/src/windows/nsWindowsAPI.h or patch for bug 162361) because this code is
not critical enough (perf-wise) to warrant extra effort?  
(In reply to comment #27)
>The stack allocation of 16384 comes straight out of a MSDN code sample.
OK, if it's good enough for Microsoft, it's good enough for me ;-)
(In reply to comment #28)
> Am I right to think that you didn't bother to use function pointers (a la
> widget/src/windows/nsWindowsAPI.h or patch for bug 162361) because this code is
> not critical enough (perf-wise) to warrant extra effort?  

jshin: the v0 prototype patch actually does the pointer indirection.  branching
on sUseUnicode is probably not much of a cost.
Attached patch v1.1 patch — — Splinter Review 
I am not keen on explicitly supporting REG_MULTI_SZ and REG_EXPAND_SZ.	Do we
need to support them?  I've made readStringValue work with those types, but
this interface exposes no way to write those types.  Is that a big deal?
Attachment #182993 - Attachment is obsolete: true
Attachment #183098 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #182993 - Flags: review?(neil.parkwaycc.co.uk)
Blocks: 293548
(In reply to comment #31)
>I am not keen on explicitly supporting REG_MULTI_SZ and REG_EXPAND_SZ. Do we
>need to support them?  I've made readStringValue work with those types, but
>this interface exposes no way to write those types.  Is that a big deal?
I thought writeStringValue appeared to suffice for those types.
> I thought writeStringValue appeared to suffice for those types.

Yes, but it always passes REG_SZ as the type parameter to RegSetValueEx.  I'm
assuming that the registry remembers the type.
Comment on attachment 183098 [details] [diff] [review]
v1.1 patch

You can take out all the REG_*_SZ stuff if you like, seeing as I'd overlooked
that we'd need extra code to be able to write them anyway.

>+nsWindowsRegKey::HasChanged(PRBool *result)
>+{
>+  if (!mWatchEvent) {
>+    *result = PR_FALSE;
>+  } else if (WaitForSingleObject(mWatchEvent, 0) == WAIT_OBJECT_0) {
>+    // An event only gets signaled once, then it's done, so we have to set up
>+    // another event to watch.
>+    StopWatching();
>+    StartWatching(mWatchRecursive);
>+    *result = PR_TRUE;
>+  }
Still goes wrong if you have an unsignalled event :-P

r=me by visual comparison with the previous patch with this fixed.
Attachment #183098 - Flags: review?(neil.parkwaycc.co.uk) → review+
Thanks Neil.  I left the REG_*_SZ stuff in place for reading since at least that
allows this interface to be used to read those type of values.  It isn't
perfect, but it would still allow someone to use the interface to read those
values without resorting to calling readBinaryValue (which has the problem of
not inflating ANSI valued strings to Unicode).

I fixed HasChanged in my local tree to be like this:

{
  if (mWatchEvent && WaitForSingleObject(mWatchEvent, 0) == WAIT_OBJECT_0) {
    // An event only gets signaled once, then it's done, so we have to set up
    // another event to watch.
    StopWatching();
    StartWatching(mWatchRecursive);
    *result = PR_TRUE;
  } else {
    *result = PR_FALSE;
  }
  return NS_OK;
}

Thanks for catching my error.
Comment on attachment 183098 [details] [diff] [review]
v1.1 patch

Doug, can you please bless this addition to xpcom / provide additional review. 
Thanks!
Attachment #183098 - Flags: superreview?(dougt)
Comment on attachment 183098 [details] [diff] [review]
v1.1 patch

please change:

#ifdef XP_WIN

to:

#if defined(XP_WIN) && !defined(WINCE)

Could you always use the UNICODE version instead of using both the A and W
versions?

Not sure it matters, but on Windows Me/98/95 this change may make mozilla
require the Microsoft Layer for Unicode.  I do not know if we already have this
dependency.
(In reply to comment #37)
> (From update of attachment 183098 [details] [diff] [review] [edit])
> please change:
> 
> #ifdef XP_WIN
> 
> to:
> 
> #if defined(XP_WIN) && !defined(WINCE)

How about if I make that change in the Makefile?  I can just make it so that
this class is not built on WINCE.


> Could you always use the UNICODE version instead of using both the A and W
> versions?

The W routines return ERROR_CALL_NOT_IMPLEMENTED on Win9x, so that is why this
code uses the W routines only on newer NT platforms and the A routines for the
others.


> Not sure it matters, but on Windows Me/98/95 this change may make mozilla
> require the Microsoft Layer for Unicode.  I do not know if we already have 
> this dependency.

We do not have a dependency on MSLU yet.  This code avoids the need for such a
dependency by calling the A routines on Win9x.  If we wanted to call the W
routines and have them function on Win9x, then yet... we'd need MSLU.  There is
a bug about adding MSLU (or a custom version of it) to Mozilla in general, but
that is not required for this bug. 
If we omit the "A" suffix then the compiler will choose "A" or "W" depending on
the build environment. I hope that normal builds will default to "A" while WINCE
builds default to "W" so that the lack of "A" exports won't be an issue; you'll
also want to make sure that on WINCE sUseUnicode is always set to true.
that is not how we build WinCE -- UNICODE is not always defined nor can it be.


The makefile change should be fine.
Minimo probably has no need for these registry APIs.  If it ever does, then I
suggest that we fix this implementation to support WINCE.

Dougt: My goal is to replace raw uses of the Win32 registry API with calls to
this new interface throughout the tree.  So, please let me know if you think
you'll need this to work on WINCE.  Or, we can just cross that bridge when we
slam into it.
darin, sorry my comment was not clear -- I do not need this on WinCE.  However
if you land this as is, it will break the WinCE build.  Since you do not have a
wince build and there isn't a tbox setup yet, land your changes as is and I will
check in a patch after you to fix the issues.

I do not see any reason to hold you up on this.
Attachment #183098 - Flags: superreview?(dougt) → superreview+
Comment on attachment 183098 [details] [diff] [review]
v1.1 patch

This is only needed for Firefox 1.1a if we want bug 293548 as well.  Otherwise,
this can wait until 1.1b.  (I'd like to see this land early if possible so that
people can try it out in the alpha, but it isn't a requirement.)
Attachment #183098 - Flags: approval1.8b3?
Attachment #183098 - Flags: approval1.8b2?
Darin: there will be a second alpha of 1.1, corresponding to 1.8b3.  Do you want
to target that, or just get this bug and bug 293548 fixed now?

/be
Targeting the second alpha is fine by me.  There may be some bug fixes in my
patch for bug 293548 that may be alpha1 fodder though.
Comment on attachment 183098 [details] [diff] [review]
v1.1 patch

a=chofmann
Attachment #183098 - Flags: approval1.8b3?
Attachment #183098 - Flags: approval1.8b3+
Attachment #183098 - Flags: approval1.8b2?
Attachment #183098 - Flags: approval1.8b2+
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Do we need a similar interface for gconf access?
Yeah, sorta like that...  Just wondering whether a platform-agnostic "platform
registry" api is at all worth it...
> Yeah, sorta like that...  Just wondering whether a platform-agnostic "platform
> registry" api is at all worth it...

The goal of this patch was to expose Windows specific stuff.  HKEY_LOCAL_MACHINE
does not map well to GConf, for example.  There are other major differences as
well.  The idea is to make it so that one would never need to use the raw Win32
registry APIs from C++ (because they are easy to use incorrectly), and to make a
really useful API for JS consumers.
(In reply to comment #50)
> Yeah, sorta like that...  Just wondering whether a platform-agnostic "platform
> registry" api is at all worth it...

brendan/bryner didn't like that suggestion. using it would require platform
specific code in any case.
my windows machine at work keeps breaking here after this checkin:

A_CONFIG_H_ -DMOZILLA_CLIENT /cygdrive/c/build/trees/tbirddbg/mozilla/xpcom/ds/n
sWindowsRegKey.cpp
nsWindowsRegKey.cpp
c:/build/trees/tbirddbg/mozilla/xpcom/ds/nsWindowsRegKey.cpp(110) : error C2065:
 'getenv' : undeclared identifier
make[4]: *** [nsWindowsRegKey.obj] Error 2
make[4]: Leaving directory `/cygdrive/c/build/trees/tbirddbg/mozilla/xpcom/ds'

I've tried a clobber build and still run into the same problem. Anyone else
seeing this or having problems? 
Mscott, try adding #include <stdlib.h> to the top of nsWindowsRegKey.cpp; if
that works, r+a=me to check that in.
that fixed the problem. Thanks Benjamin. I just checked in the fix.
Summary: Provide a scriptable interface to the Windows registry → Provide a scriptable interface to the Windows registry (nsIWindowsRegKey)
Comment on attachment 183098 [details] [diff] [review]
v1.1 patch

>+  const unsigned long ROOT_KEY_CLASSES_ROOT     = 0x80000000;
>+  const unsigned long ROOT_KEY_CURRENT_USER     = 0x80000001;
>+  const unsigned long ROOT_KEY_LOCAL_MACHINE    = 0x80000002;
I built xpidl from scratch, and the system I was using objects to these declarations because 0x80000000 is negative...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: