Note: There are a few cases of duplicates in user autocompletion which are being worked on.

use nsIWindowsRegKey for Windows registry handling

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Jungshik Shin, Assigned: Jungshik Shin)

Tracking

({intl})

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

8.86 KB, patch
Biesinger
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
28.94 KB, patch
Jungshik Shin
: review+
Darin Fisher
: superreview+
bsmedberg
: approval1.8b4-
Details | Diff | Splinter Review
29.23 KB, patch
Details | Diff | Splinter Review
4.63 KB, patch
Biesinger
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
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.
Status: NEW → ASSIGNED
Summary: use 'W' APIs for Windows registry handling → use 'W' APIs for Windows registry handling

Comment 2

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

Comment 3

13 years ago
(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

13 years ago
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...
(Assignee)

Comment 5

13 years ago
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.
Keywords: intl
(Assignee)

Comment 6

13 years ago
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

13 years ago
jshin:

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

Comment 8

12 years ago
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.
No longer depends on: 239279
Summary: use 'W' APIs for Windows registry handling → use nsIWindowsRegKey for Windows registry handling
(Assignee)

Comment 9

12 years ago
Created attachment 184713 [details] [diff] [review]
profile migrator patch (diff -u7 -w -p)
Attachment #155485 - Attachment is obsolete: true
(Assignee)

Comment 10

12 years ago
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 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?)
bah, the impl of that screws up data outside the native charset :-/ that should
be fixed...
(Assignee)

Comment 13

12 years ago
(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. 

(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)
(Assignee)

Comment 15

12 years ago
(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) :-)



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

Comment 17

12 years ago
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.
Attachment #184713 - Attachment is obsolete: true
Attachment #184714 - Attachment is obsolete: true
Attachment #184920 - Flags: superreview?(darin)
Attachment #184920 - Flags: review?(cbiesinger)

Comment 18

12 years ago
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 :-(
Attachment #184920 - Flags: superreview?(darin) → superreview+
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?
Attachment #184920 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 20

12 years ago
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.
Attachment #184920 - Flags: approval1.8b3?

Updated

12 years ago
Attachment #184920 - Flags: approval1.8b3? → approval1.8b3+

Comment 21

12 years ago
(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?

(Assignee)

Comment 22

12 years ago
See comment #0. There are still some places to switch over to nsIWindowsRegKey.
(Assignee)

Comment 23

12 years ago
Created attachment 187369 [details] [diff] [review]
msie profile migrator patch

this is for MSIE profile migration.
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
(Assignee)

Comment 25

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #187369 - Attachment is obsolete: true
you still have one of those in CopySmartKeywords...
(Assignee)

Comment 27

12 years ago
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 !
(Assignee)

Updated

12 years ago
Attachment #187521 - Attachment is obsolete: true
Attachment #188760 - Flags: superreview?(darin)
Attachment #188760 - Flags: review?(cbiesinger)
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
Attachment #188760 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 29

12 years ago
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.
Attachment #188760 - Attachment is obsolete: true
Attachment #188853 - Flags: superreview?(darin)
Attachment #188853 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #188760 - Flags: superreview?(darin)

Comment 30

12 years ago
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
Attachment #188853 - Flags: superreview?(darin) → superreview+
ah, yeah, I meant to mention that - one place in that patch already uses the
nsAString& method. maybe it'd be worth being consistent...
(Assignee)

Comment 32

12 years ago
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)
Attachment #188853 - Flags: approval1.8b4?
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

12 years ago
bsmedberg: I think this is about fixing non-ASCII issues with profile migration,
but I defer to jshin.
(Assignee)

Comment 35

12 years ago
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 on attachment 188853 [details] [diff] [review]
patch (profile migrator) v3

I'm inclined to say this should wait until we branch.
Attachment #188853 - Flags: approval1.8b4? → approval1.8b4-

Comment 37

12 years ago
> 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?
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".
(Assignee)

Comment 39

12 years ago
landed on the trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 316634
(Assignee)

Comment 40

12 years ago
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.
Attachment #209830 - Flags: superreview+
Attachment #209830 - Flags: review+
Attachment #209830 - Flags: approval1.8.1?
Attachment #209830 - Flags: approval1.8.1? → branch-1.8.1?(darin)

Updated

12 years ago
Attachment #209830 - Flags: branch-1.8.1?(darin) → branch-1.8.1+
Jshin:

The patch cannot apply to 1.8 branch...
(Assignee)

Comment 42

12 years ago
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.
Attachment #209830 - Attachment is obsolete: true
Keywords: fixed1.8.1
(Assignee)

Comment 43

12 years ago
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).
Keywords: fixed1.8.1
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.
(Assignee)

Comment 45

12 years ago
(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. 

(Assignee)

Comment 46

12 years ago
Created attachment 214763 [details] [diff] [review]
homepage URI import patch
(Assignee)

Comment 47

11 years ago
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|.
Attachment #214763 - Attachment is obsolete: true
(Assignee)

Comment 48

11 years ago
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.
Attachment #237996 - Flags: superreview?(darin)
Attachment #237996 - Flags: review?(cbiesinger)

Updated

11 years ago
Attachment #237996 - Flags: superreview?(darin) → superreview+
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?)
Attachment #237996 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 50

11 years ago
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. 

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