250255 - use nsIWindowsRegKey for Windows registry handling

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Jungshik Shin]

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

[Jungshik Shin]

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

[Hidehiro Kozawa]

http://bugzilla.mozilla.org/show_bug.cgi?id=232969#c34
didn't be posted as new bug yet?

Comment 3

[Jungshik Shin]

(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

[Brodie]

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

[Jungshik Shin]

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

[Jungshik Shin]

Attachment: 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

[Hidehiro Kozawa]

jshin:

If you post rest of patch for building (MZLU customizing and linking), I can
test it.

Comment 8

[Jungshik Shin]

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

[Jungshik Shin]

Attachment: profile migrator patch (diff -u7 -w -p)

Comment 10

[Jungshik Shin]

Attachment: 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

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

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

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

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

Comment 13

[Jungshik Shin]

(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

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

(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

[Jungshik Shin]

(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

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

ok, I don't know that function well enough to know. does user-defined version
information exist?

Comment 17

[Jungshik Shin]

Attachment: 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

[Darin Fisher]

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

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

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

[Jungshik Shin]

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

[Caleb]

(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

[Jungshik Shin]

See comment #0. There are still some places to switch over to nsIWindowsRegKey.

Comment 23

[Jungshik Shin]

Attachment: msie profile migrator patch

this is for MSIE profile migration.

Comment 24

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

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

[Jungshik Shin]

Attachment: 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

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

you still have one of those in CopySmartKeywords...

Comment 27

[Jungshik Shin]

Attachment: 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

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

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

[Jungshik Shin]

Attachment: 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

[Darin Fisher]

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

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

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

[Jungshik Shin]

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

[Benjamin Smedberg]

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

[Darin Fisher]

bsmedberg: I think this is about fixing non-ASCII issues with profile migration,
but I defer to jshin.

Comment 35

[Jungshik Shin]

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

[Benjamin Smedberg]

Comment on attachment 188853 [details] [diff] [review]
patch (profile migrator) v3

I'm inclined to say this should wait until we branch.

Comment 37

[Darin Fisher]

> 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

[Benjamin Smedberg]

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

[Jungshik Shin]

landed on the trunk

Comment 40

[Jungshik Shin]

Attachment: 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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Jshin:

The patch cannot apply to 1.8 branch...

Comment 42

[Jungshik Shin]

Attachment: 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

[Jungshik Shin]

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

[:Gavin Sharp [email: [e-mail]]]

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

[Jungshik Shin]

(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

[Jungshik Shin]

Attachment: homepage URI import patch

Comment 47

[Jungshik Shin]

Attachment: 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

[Jungshik Shin]

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

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

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

[Jungshik Shin]

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.