Brad Lassey [:blassey] (use needinfo?) Reporter
	Description • 17 years ago

      No description provided.

	Brian Crowder Assignee
	Comment 1 • 16 years ago

jst:  Review-ping?

	Bernard Alleysson
	Comment 2 • 16 years ago

Please see:
http://msdn.microsoft.com/en-us/library/ms647486(VS.85).aspx

LoadString "nBufferSize" is in TCHAR unit.

For instance, in the patch:

+  wchar_t szString[512];
+  LoadStringW(hInst, IDS_TITLE, szString, sizeof(szString));

sizeof(szString)/sizeof(szString[0]) should be used, or please use NS_ARRAY_LENGTH.

All use of LoadStringW should be carefully reviewed, as there are several of them in the patch.

	Johnny Stenback (:jst)
	Comment 3 • 16 years ago

Comment on attachment 309231 [details] [diff] [review]
directy from "other" patch on bug 418703

Sorry for not getting to this sooner :(

- In TryToUseNPRuntimeJavaPlugIn():

+  if (ERROR_SUCCESS != ::RegOpenKeyExW(HKEY_LOCAL_MACHINE,
                                       keyName, 0, KEY_READ, &javaKey)) {

Fix next line indentation.

- Further down, same file:

-      char current_version[80];
+      wchar_t current_version[80];
       DWORD length = sizeof(current_version);

That needs to be sizeof(current_version)/sizeof(current_version[0]).

         DWORD pathlen = sizeof(path);

Same thing.

-        result = ::RegQueryValueEx(keyloc, "Plugins Directory", NULL, &type,
+        result = ::RegQueryValueExW(keyloc, L"Plugins Directory", NULL, &type,
                                    (LPBYTE)&path, &pathlen);

Fix next line indentation.

-    LONG result = ::RegOpenKeyEx(HKEY_LOCAL_MACHINE, curKey, 0, KEY_READ,
+    LONG result = ::RegOpenKeyExW(HKEY_LOCAL_MACHINE, curKey, 0, KEY_READ,
                                  &baseloc);

fix indentation.

     // Look for "BrowserJavaVersion"
-    if (ERROR_SUCCESS != ::RegQueryValueEx(baseloc, "BrowserJavaVersion", NULL,
+    if (ERROR_SUCCESS != ::RegQueryValueExW(baseloc, L"BrowserJavaVersion", NULL,
                                            NULL, (LPBYTE)&browserJavaVersion,
                                            &numChars))
here too.

       pathlen = sizeof(path);

sizeof(path)/sizeof(path[0]).

-      result = ::RegEnumKeyEx(baseloc, index, curKey, &numChars, NULL, NULL,
+      result = ::RegEnumKeyExW(baseloc, index, curKey, &numChars, NULL, NULL,
                               NULL, &modTime);

Fix indentation...

This patch is full of sizeof() issues, please go through all the places where you change the type of string variables and make sure the size isn't assumed to be the length. And while you're at it, fix as many indentation problems as you run into as well.

Once done, I can r+sr, but until then, r-

	Brian Crowder Assignee
	Comment 5 • 16 years ago

(In reply to comment #3)
> (From update of attachment 309231 [details] [diff] [review])
> Fix next line indentation.

I believe I have repaired all the indentation in this patch, please double-check my work.

> - Further down, same file:
> 
> -      char current_version[80];
> +      wchar_t current_version[80];
>        DWORD length = sizeof(current_version);
> 
> That needs to be sizeof(current_version)/sizeof(current_version[0]).

Actually, RegQueryValueEx's 6th parameter, according to MSDN, is "A pointer to a variable that specifies the size of the buffer pointed to by the lpData parameter, in bytes.", so this use of sizeof is correct.  I have fixed others, however.  Again, please double-check my work.

	Brian Crowder Assignee
	Comment 6 • 16 years ago

Comment on attachment 337898 [details] [diff] [review]
updated revision

I also removed a lot of meaningless twiddling from the previous patch (changes from lstrcmp to strcmp and back, and so on), so this patch should be markedly smaller and easier to review.

	Brian Crowder Assignee
	Comment 7 • 16 years ago

Bugzilla interdiff will probably fail you; the old patch was badly bitrotted.  I don't have a good way of giving you a better interdiff myself, either, I'm afraid.  I had to hand-rewrite a lot of this, because patch was barfing on the old patch, even with fuzz.

	Bernard Alleysson
	Comment 8 • 16 years ago

about the patch "update revision":

+    if(RegSetValueExW(hkey, wmime, 0,  REG_SZ, (LPBYTE) L"(none)", 7) != ERROR_SUCCESS)

Replace "7" by "7 * sizeof(WCHAR)".

Replace "MessageBox(0, "Error adding MIME type value", "Default Plugin", MB_OK);"
with "MessageBoxW(0, L"Error adding MIME type value", L"Default Plugin", MB_OK);" ?

	Bernard Alleysson
	Comment 9 • 16 years ago

>   DWORD dwType, keysize = 512;

Should be:

   DWORD dwType, keysize = 512 * sizeof(WCHAR);

	Brian Crowder Assignee
	Comment 10 • 16 years ago

Comment on attachment 337898 [details] [diff] [review]
updated revision

I'll post another patch with Bernard's finds fixed shortly.

	Brian Crowder Assignee
	Comment 11 • 16 years ago

Thanks, Bernard.

	Brian Crowder Assignee
	Comment 12 • 16 years ago

Woops, with comment #9 in, this time.

	Bernard Alleysson
	Comment 13 • 16 years ago

1)
>     DWORD numChars = _MAX_PATH;

Should be DWORD numChars = _MAX_PATH * sizeof(WCHAR);
It is used here
+    if (ERROR_SUCCESS != ::RegQueryValueExW(baseloc, L"BrowserJavaVersion", NULL,
+                                            NULL, (LPBYTE)&browserJavaVersion,
+                                            &numChars))

There are other declarations "DWORD numchars = _MAX_PATH" but some doesn't seem to be used ?

2) You changed only one occurrence of "MessageBox" to "MessageBoxW", there are other occurrences.

That's all I could find, I went through the patch twice.

	Brian Crowder Assignee
	Comment 14 • 16 years ago

Comment on attachment 337918 [details] [diff] [review]
v4

Thanks, one more spin coming up.

	Brian Crowder Assignee
	Comment 15 • 16 years ago

Thanks again, Bernard.

	Brian Crowder Assignee
	Comment 16 • 16 years ago

The v4 -> v5 interdiff works; I moved the numChars decls closer to where they're used, introduced a numBytes variable for this query, and then fixed the other MessageBox invocation.

	Brian Crowder Assignee
	Comment 17 • 16 years ago

jst:  Review-ping?

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 18 • 16 years ago

jst: review ping?

	Brian Crowder Assignee
	Comment 19 • 16 years ago

Actually, vlad posted a patch in another bug which jst is going to review, instead I think.  We should probably resolve this one INCOMPLETE?