Last Comment Bug 250255 - use nsIWindowsRegKey for Windows registry handling
: use nsIWindowsRegKey for Windows registry handling
Status: RESOLVED FIXED
: intl
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: x86 Windows XP
-- normal (vote)
: ---
Assigned To: Jungshik Shin
: Yuying Long
: Makoto Kato [:m_kato]
Mentors:
Depends on: 316634
Blocks:
  Show dependency treegraph
 
Reported: 2004-07-07 13:31 PDT by Jungshik Shin
Modified: 2006-09-18 19:06 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
printer name patch (11.90 KB, patch)
2004-08-08 08:47 PDT, Jungshik Shin
no flags Details | Diff | Splinter Review
profile migrator patch (diff -u7 -w -p) (7.24 KB, patch)
2005-05-27 12:18 PDT, Jungshik Shin
no flags Details | Diff | Splinter Review
profile mgrator patch (same as above but with diff -u7 -p) (9.48 KB, patch)
2005-05-27 12:20 PDT, Jungshik Shin
no flags Details | Diff | Splinter Review
patch (now using nsILocalFileWin) (8.86 KB, patch)
2005-05-31 05:38 PDT, Jungshik Shin
cbiesinger: review+
darin.moz: superreview+
asa: approval1.8b3+
Details | Diff | Splinter Review
msie profile migrator patch (28.31 KB, patch)
2005-06-26 19:23 PDT, Jungshik Shin
no flags Details | Diff | Splinter Review
patch (msie migrator) with nsIURL change per cbie (28.39 KB, patch)
2005-06-28 11:18 PDT, Jungshik Shin
no flags Details | Diff | Splinter Review
patch (msie migrator) with nsIURL change per cbie v2 (28.57 KB, patch)
2005-07-09 05:04 PDT, Jungshik Shin
cbiesinger: review+
Details | Diff | Splinter Review
patch (profile migrator) v3 (28.94 KB, patch)
2005-07-10 11:20 PDT, Jungshik Shin
jshin1987: review+
darin.moz: superreview+
benjamin: approval1.8b4-
Details | Diff | Splinter Review
patch for 1.8.x branch (28.97 KB, patch)
2006-01-27 05:06 PST, Jungshik Shin
jshin1987: review+
jshin1987: superreview+
darin.moz: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
patch for 1.8 branch that was checked in (29.23 KB, patch)
2006-01-31 06:29 PST, Jungshik Shin
no flags Details | Diff | Splinter Review
homepage URI import patch (2.90 KB, patch)
2006-03-11 00:10 PST, Jungshik Shin
no flags Details | Diff | Splinter Review
homepage url import patch (4.63 KB, patch)
2006-09-12 09:20 PDT, Jungshik Shin
cbiesinger: review+
darin.moz: superreview+
Details | Diff | Splinter Review

Description User image Jungshik Shin 2004-07-07 13:31:24 PDT
In bug 243618, bug 232969 and bug 247377 (it's for aviary), I made interim
patches to make non-ASCII registry values work as long as they're within the
repertoire of the default system encoding. When bug 239279 is fixed, we'll be
able to  make any Unicode values work on Win 2k/XP/NT4 using Windows 'W' APIs.
Comment 1 User image Jungshik Shin 2004-07-07 13:38:56 PDT
bug 167128 is another fixed in a similar way although it doesn't involve
registry-related APIs but the patten is exactly the same as other interim
patches mentioned in comment #0.
Comment 2 User image Hidehiro Kozawa 2004-07-07 19:58:52 PDT
http://bugzilla.mozilla.org/show_bug.cgi?id=232969#c34
didn't be posted as new bug yet?
Comment 3 User image Jungshik Shin 2004-07-08 19:38:58 PDT
(In reply to comment #2)
> http://bugzilla.mozilla.org/show_bug.cgi?id=232969#c34
> didn't be posted as new bug yet?

That's bug 243618 and has already been fixed :-) Btw, please don't use the full
URL when refering to a bug or comment (just use 'bug xyz comment uv' and
bugzilla will take care of the rest).

Comment 4 User image Brodie 2004-07-08 19:52:37 PDT
There is too much ad-hoc hacking of W wrapper functions in Mozilla. The real fix
for all of these is bug 239279. We really need to add MSLU, define UNICODE for
the build, fix the millions of breakages it will cause, and use W API
everywhere. And then wouldn't the world be a better place...
Comment 5 User image Jungshik Shin 2004-07-08 20:03:14 PDT
re: comment #4
That's exactly my plan. I filed this bug basically as a reminder to myself.  I
wouldn't do anything here (there's nothing I can do here) until bug 239279 is
resolved in one way or another.
Comment 6 User image Jungshik Shin 2004-08-08 08:47:07 PDT
Created attachment 155485 [details] [diff] [review]
printer name patch

This is to enable Mozilla to recognize printers with names in any Unicode
characters on Win2k/XP while still working with printers named in the system
code page on Win 9x/ME. 

I made this patch to test MZLU (bug 239279), but haven't done any testing yet.
Comment 7 User image Hidehiro Kozawa 2004-08-16 18:13:48 PDT
jshin:

If you post rest of patch for building (MZLU customizing and linking), I can
test it.
Comment 8 User image Jungshik Shin 2005-05-26 10:34:39 PDT
A new interface nsIWindowsRegKey takes care of the OS version check and W/A API
switching so that we don't have wait for bug 239279 to be fixed.
Comment 9 User image Jungshik Shin 2005-05-27 12:18:29 PDT
Created attachment 184713 [details] [diff] [review]
profile migrator patch (diff -u7 -w -p)
Comment 10 User image Jungshik Shin 2005-05-27 12:20:24 PDT
Created attachment 184714 [details] [diff] [review]
profile mgrator patch (same as above but with diff -u7 -p)

I got rid of a memory leak while making nsProfileMigrator use nsIWindowsRegKey
Comment 11 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-05-27 12:27:50 PDT
Comment on attachment 184714 [details] [diff] [review]
profile mgrator patch (same as above but with diff -u7 -p)

could you also use
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsILocalFileWin.idl#40 instead
of this manual version stuff querying?

(hm... why does getVersionInfoField not use a wide string as argument?)
Comment 12 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-05-27 12:30:48 PDT
bah, the impl of that screws up data outside the native charset :-/ that should
be fixed...
Comment 13 User image Jungshik Shin 2005-05-27 17:15:15 PDT
(In reply to comment #12)
> (hm... why does getVersionInfoField not use a wide string as argument?)

Is there any non-ASCII field *name* in VersionInfo? If there's, we have to
change the interface to accept AString. Otherwise, it's unnecessary, isn't it? 

> bah, the impl of that screws up data outside the native charset :-/ that should
> be fixed...

The implementation of nsILocalFile(Win) doesn't support characters outside the
native code page in many methods at the moment, which hopefully will be fixed
soon in bug 162361 I've been working on.

(In reply to comment #11)
> could you also use
> http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsILocalFileWin.idl#40 instead
> of this manual version stuff querying?

I'll revise the patch to use that although that means characters outside the
native charset wouldn't be supported until bug 162361 is fixed. 

Comment 14 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-05-28 15:32:08 PDT
(In reply to comment #13)
> Is there any non-ASCII field *name* in VersionInfo? If there's, we have to
> change the interface to accept AString. Otherwise, it's unnecessary, isn't it? 

well, the lpSubBlock argument of VerQueryValue is an LPTSTR. doesn't that mean
that it can contain non-ASCII values?
(http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/resources/versioninformation/versioninformationreference/versioninformationfunctions/verqueryvalue.asp)
Comment 15 User image Jungshik Shin 2005-05-28 18:23:09 PDT
(In reply to comment #14)
> 
> well, the lpSubBlock argument of VerQueryValue is an LPTSTR. doesn't that mean
> that it can contain non-ASCII values?

Not necessarily. MS appears to use LPTSTR in all of their APIs whenever a
pointer to 'chartype' has to be passed.   

(http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/resources/versioninformation/versioninformationreference/versioninformationfunctions/verqueryvalue.asp)

The above page lists all the possible values that can be used as lpSubBlock and
I don't see any non-ASCII 'block name' in the list. (well, they may do in the
future...)

Nonetheless, I've already made a patch to change the interface, but I'm not sure
if it's a good thing(TM) :-)



Comment 16 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-05-29 07:48:20 PDT
ok, I don't know that function well enough to know. does user-defined version
information exist?
Comment 17 User image Jungshik Shin 2005-05-31 05:38:57 PDT
Created attachment 184920 [details] [diff] [review]
patch (now using nsILocalFileWin)

As suggested by cbie, I now use nsILocalFileWin.
As far as I can tell from MSDN,  'subBlockName' is limited to the set of
predefined values so that I just left alone nsILocalFileWin.
Comment 18 User image Darin Fisher 2005-06-02 09:09:43 PDT
Comment on attachment 184920 [details] [diff] [review]
patch (now using nsILocalFileWin)

>Index: browser/components/migration/src/nsProfileMigrator.cpp

>+  nsAutoString filePath;
>+  if (value.CharAt(1) == ':') 
>+    filePath = Substring(value, 0, lastIndex + 4);
>+  else
>+    filePath = Substring(value, 1, lastIndex + 3);        

nit: how about this,

  int index = (value.CharAt(1) == ':') ? 0 : 1;
  nsDependentSubstring filePath(value, index, lastIndex + 4 - index);


Otherwise this looks fine.  I trust that you have tested this carefully.
There're plenty of edge cases to test :-(
Comment 19 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-06-03 09:25:38 PDT
Comment on attachment 184920 [details] [diff] [review]
patch (now using nsILocalFileWin)

per http://www.mozilla.org/hacking/portable-cpp.html#dont_ifdef_include
the includes in nsProfileMigrator.cpp should not be in an ifdef (at least those
that are available xp :-) )

I don't really understand this code:
+  if (value.CharAt(1) == ':') 
+    filePath = Substring(value, 0, lastIndex + 4);
+  else
+    filePath = Substring(value, 1, lastIndex + 3);	   

but I guess it worked before. can you kill the trailing whitespace at the last
of these lines though?
Comment 20 User image Jungshik Shin 2005-06-15 06:14:23 PDT
Comment on attachment 184920 [details] [diff] [review]
patch (now using nsILocalFileWin)

asking for approval of landing to the trunk.

thanks for r/sr. I took care of review comments in my tree. My test may not be
as thorough as it can, but at least the profile migration works as well as the
current code, I believe. Little substantial change of the code in terms of
functionality except for migrating to a new interface and a little refactoring
using nsILocalFileWin.
Comment 21 User image Caleb 2005-06-16 06:39:04 PDT
(In reply to comment #17)
> Created an attachment (id=184920) [edit]
> patch (now using nsILocalFileWin)

This patch was checked in, can you solve this bug or is there any more work
needed to be done?

Comment 22 User image Jungshik Shin 2005-06-16 07:47:06 PDT
See comment #0. There are still some places to switch over to nsIWindowsRegKey.
Comment 23 User image Jungshik Shin 2005-06-26 19:23:39 PDT
Created attachment 187369 [details] [diff] [review]
msie profile migrator patch

this is for MSIE profile migration.
Comment 24 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-06-27 00:28:59 PDT
Comment on attachment 187369 [details] [diff] [review]
msie profile migrator patch

browser/components/migration/src/nsIEProfileMigrator.cpp
   nsCOMPtr<nsIURI>
uri(do_CreateInstance("@mozilla.org/network/standard-url;1"));

this is probably wrong, why does it not use nsIIOService::NewURI? (or
NS_NewURI)

+	   uri->SetSpec(NS_ConvertUTF16toUTF8(url)); 

same
Comment 25 User image Jungshik Shin 2005-06-28 11:18:29 PDT
Created attachment 187521 [details] [diff] [review]
patch (msie migrator)  with nsIURL change per cbie

Thanks for spotting it. Using NS_NewURI gets rid of a bunch of warnings about
'invalid specs' when importing IE bookmarks.
Comment 26 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-06-28 13:48:33 PDT
you still have one of those in CopySmartKeywords...
Comment 27 User image Jungshik Shin 2005-07-09 05:04:13 PDT
Created attachment 188760 [details] [diff] [review]
patch (msie migrator) with nsIURL change per cbie v2

I took care of the second nsIURI as well. Thanks for pointing that out !
Comment 28 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-07-10 10:02:37 PDT
Comment on attachment 188760 [details] [diff] [review]
patch (msie migrator) with nsIURL change per cbie v2

+      nsAutoString port(Substring(hostPort, portDelimOffset + 1, 
+				   hostPort.Length() - (portDelimOffset + 1)));

maybe it'd be more readable to use StringTail here?

+      if (NS_FAILED(regKey->GetValueName(offset, valueName)))

hm, nsIWindowsRegKey docs do not describe what happens when there is no value
at that offset :-/

+      if (NS_SUCCEEDED(regKey->Open(nsIWindowsRegKey::ROOT_KEY_CURRENT_USER,
+			searchUrlKey + keyName,

hm... why not regKey->OpenChild?

in fact, I think the code here won't work due to a missing backslash


+	 { L"ftp=",	4, PR_FALSE, "network.proxy.ftp",

hmm, I hope that L"foo" will work on all windows compilers.. I guess gcc and
msvc are ok, and moz doesn't support any others yet


Did this patch remove all users of LARGE_BUFFER? if so, the #define should be
removed...

r=biesi
Comment 29 User image Jungshik Shin 2005-07-10 11:20:18 PDT
Created attachment 188853 [details] [diff] [review]
patch (profile migrator) v3

addressed biesi's concerns. Got rid of LARGE_BUFFER, used OpenChild. Because I
don't wanna be blamed for non-portable code (in the future) ;-), I went back to
8bit strings and used NS_ConvertASCIItoUTF16 explicitly. This part being
non-perf-critical, this should be all right, I guess.

Btw, I used |Substring| with two parameters instead of StringTail because it's
simpler thatway.
Comment 30 User image Darin Fisher 2005-07-11 14:51:19 PDT
Comment on attachment 188853 [details] [diff] [review]
patch (profile migrator) v3

There is a version of NS_NewURI that takes a nsAString parameter for the URI
string.  You might want to use that to reduce the amount of code you have to
type.  It might make things slightly cleaner.

sr=darin
Comment 31 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-07-11 15:16:46 PDT
ah, yeah, I meant to mention that - one place in that patch already uses the
nsAString& method. maybe it'd be worth being consistent...
Comment 32 User image Jungshik Shin 2005-07-31 00:44:17 PDT
Comment on attachment 188853 [details] [diff] [review]
patch (profile migrator) v3

asking for a. 
this is rewritting msie/opera migrators using a new windows registry api and
risk is low (while doing that, it fixes a couple of 'bugs',too)
Comment 33 User image Benjamin Smedberg [:bsmedberg] 2005-08-02 04:56:07 PDT
I'm concerned about the risk of this: our migrators don't get the most thorough
testing during the release cycle. What bugs does this actually fix which are
worth taking on the risk?
Comment 34 User image Darin Fisher 2005-08-02 08:56:25 PDT
bsmedberg: I think this is about fixing non-ASCII issues with profile migration,
but I defer to jshin.
Comment 35 User image Jungshik Shin 2005-08-02 09:20:52 PDT
It fixes handling of the IDN hostnames  and non-ASCII URL query parts (if
they're stored unescaped in the registry)  in IE history/proxy settings, but IDN
part is not critical  because IE doesn't support IDN without a 3rd party plugin. 

In case of Opera, handling of IDN hostnames in proxy setting will be fixed.
Because Opera does support IDN, this has some real impact (although not big). 

Another thing is to fix import of non-ASCII IE smart keywords.  

While doing these, I  killed some warnings I got when migrating from IE.
(comment #25)

Given all these, if you don't feel comfortable, it's fine with me to put it off
until a branch is cut. 
Comment 36 User image Benjamin Smedberg [:bsmedberg] 2005-08-02 09:29:42 PDT
Comment on attachment 188853 [details] [diff] [review]
patch (profile migrator) v3

I'm inclined to say this should wait until we branch.
Comment 37 User image Darin Fisher 2005-08-02 10:27:34 PDT
> I'm inclined to say this should wait until we branch.

Does that mean that this patch will not be included in Firefox 1.5?  What does
"wait until we branch" mean?  If you intend to approve this for the 1.5 branch,
then I think it makes sense to land this now rather than twice later, no?
Comment 38 User image Benjamin Smedberg [:bsmedberg] 2005-08-02 10:52:28 PDT
Sorry, I meant "I don't think we should take this for firefox 1.5, and should
commit it on the trunk after firefox 1.5 branches".
Comment 39 User image Jungshik Shin 2005-10-01 19:01:46 PDT
landed on the trunk
Comment 40 User image Jungshik Shin 2006-01-27 05:06:12 PST
Created attachment 209830 [details] [diff] [review]
patch for 1.8.x branch

this is a profile migrator patch for 1.8.x branch, which is virtually identical to attachment 188853 [details] [diff] [review] except that it fixed a trivial problem (s/;/=/ in one place) identified recently in the trunk. 

We for sure need this for FF 2.0. This has been in the trunk for a few months and hasn't led to any regression. 

I'm not asking for 1.8.0.2 because it's not likely to be approved for the same reason as it's not approved for 1.8b4. (of course, I'll be glad to land this in 1.8.0.x) In that case, we need to land a branch patch in 1.8.0.x for bug 316634.
Comment 41 User image Masayuki Nakano [:masayuki] 2006-01-31 03:04:51 PST
Jshin:

The patch cannot apply to 1.8 branch...
Comment 42 User image Jungshik Shin 2006-01-31 06:29:30 PST
Created attachment 210233 [details] [diff] [review]
patch for 1.8 branch that was checked in

(In reply to comment #41)

> The patch cannot apply to 1.8 branch...

That's because MOZ_PLACE patch got landed in the branch after I uploaded my patch. It's a trivial fix I took care of when landing this patch.
Comment 43 User image Jungshik Shin 2006-02-05 22:51:24 PST
Let's not add 'fixed 1.8.1' until I take care of mconnor's check-in for bug 313529   that included direct calls to Windows registry APIs (that patch went in for 1.8.0 branch as well and has a bug with non-ascii filename, but perhaps I'll just fix 1.8.1 and trunk).
Comment 44 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-03-05 22:05:32 PST
The 1.8 branch patch that was landed moved the following block of code in nsIEProfileMigrator.cpp:
#ifdef MOZ_PLACES
          bms->InsertItem(keywordsFolder, uri, -1);
          bms->SetItemTitle(uri, NS_ConvertUTF8toUTF16(keyNameStr));
#else

Was that intentional? This made the trunk and branch versions of this file different.
Comment 45 User image Jungshik Shin 2006-03-11 00:04:30 PST
(In reply to comment #44)
> The 1.8 branch patch that was landed moved the following block of code in
> nsIEProfileMigrator.cpp:
> #ifdef MOZ_PLACES
>           bms->InsertItem(keywordsFolder, uri, -1);
>           bms->SetItemTitle(uri, NS_ConvertUTF8toUTF16(keyNameStr));
> #else
> 
> Was that intentional? This made the trunk and branch versions of this file
> different.

Yes, it's intentional (that way, MOZ_PLACES can avoid building the unncessary code), but I'd better move that block in the trunk the way it's done in 1.8branch so that they're in sync. 

Comment 46 User image Jungshik Shin 2006-03-11 00:10:27 PST
Created attachment 214763 [details] [diff] [review]
homepage URI import patch
Comment 47 User image Jungshik Shin 2006-09-12 09:20:36 PDT
Created attachment 237996 [details] [diff] [review]
homepage url import patch

attachment 214763 [details] [diff] [review] was bit-rotten due to the patch for bug 346602.

The second hunk is for what's mentioned in comment #42. 

I haven't yet been able to test this patch. To import the homepage url, I have to delete 'Firefox folders' (to emulate a totally new install), but my build always crashes in toolkit/components/places/src/nsNavBookmarks.cpp (line 1162) before reaching |GetSourceHomePageURL|.
Comment 48 User image Jungshik Shin 2006-09-12 18:10:04 PDT
Comment on attachment 237996 [details] [diff] [review]
homepage url import patch

Asking for r/sr.

I tested the patch on 1.8branch, but on trunk I can't test it because of the crash (unrelated to this patch) mentioned in the previous comment.
Comment 49 User image Christian :Biesinger (don't email me, ping me on IRC) 2006-09-13 13:40:10 PDT
Comment on attachment 237996 [details] [diff] [review]
homepage url import patch

this code is kind of odd...
does it really need the type checking?

anyway, r=biesi (shouldn't the bug be reopened if it's not fixed on trunk yet?)
Comment 50 User image Jungshik Shin 2006-09-18 19:06:31 PDT
fix checked in. thansk for r/sr.
(In reply to comment #49)
> (From update of attachment 237996 [details] [diff] [review] [edit])

> does it really need the type checking?

you're right that it doesn't. I got rid of the check when landing. (actually, the landing was done in two stages, one with the check and the other that removed the check.)


> anyway, r=biesi (shouldn't the bug be reopened if it's not fixed on trunk yet?)

I should have reopened it (because the homepage url import was added after this bug had been closed). There are other registry issues (in mailnews), but I'd rather deal with them in another bug. 


Note You need to log in before you can comment on or make changes to this bug.