380347 - port shellservice/winhooks to suiterunner

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

We should probably implement a suite shell service for system hooks in suiterunner. This should probably go into suite/common/shell or suite/shell and base upon the Firefox shell service in http://mxr.mozilla.org/mozilla/source/browser/components/shell/ as esp. the Windows integration part of this has seen some good work for Vista integration, IIRC.
Our old implementation in http://mxr.mozilla.org/mozilla/source/xpfe/components/winhooks/ probably has some additional functionality when it comes to mail stuff or so, we need to port that over as well, of course.

This will also get rid of nsWindowsHooks.properties and replace it with a locale file in suite/locales/.../common/ which solves one part of bug 377801

Comment 1

[Mark Banner (:standard8)]

Implementing this bug will mean we don't need to fix bug 364168 (or at least not in the same way).

Comment 2

[Frank Wein [:mcsmurf]]

Attachment: Patch, first attempt

Ok, here's the work I've done so far. It's not ready for review yet, but if you want you can look at it and point out obviously wrong code ;-).
The patch adds a shell service for Windows like FF and TB have one. It also disables the default browser/default mail dialogs and replaces them with a new, generic default client dialog. It's the same dialog in browser and mail, so it is only displayed once per session. But the old dialogs can also be kept if wanted.

Comment 3

[Frank Wein [:mcsmurf]]

Forgot the locale changes in the patch, will be included in the next patch.

Comment 4

[Robert Kaiser]

Comment on attachment 279523 [details] [diff] [review]
Patch, first attempt

>Index: mailnews/mapi/mapihook/src/Makefile.in
>===================================================================
>-ifndef MOZ_THUNDERBIRD
>+ifeq (,$(MOZ_SUITE)$(MOZ_THUNDERBIRD))
> CPPSRCS        += nsMapiRegistry.cpp nsMapiRegistryUtils.cpp
> endif

As already discussed on IRC before, I think it would be better to just kill off nsMapiRegistry* completely, there is no mailnews besides Thunderbird and suite.

Same for the places referring to that in msgMapiSupport.cpp

Comment 5

[Robert Kaiser]

For references, see bug 353906 where Thunderbird adopted shell service - and all those nsMapiRegistry* files have a comment that says "This File is no longer used by Thunderbird. Seamonkey is the only consumer." so we probably should really kick them out with this work.

Additionally, bug 353906 comment #44 reads to me as accountUtils.js being shared between SeaMonkey and Thunderbird, so 1) it should contain stuff already to make this work with TB's shell service and 2) make sure you don't break TB with your changes :)

Comment 6

[Frank Wein [:mcsmurf]]

Attachment: Shell service only

Ok, new patch which only includes the shell service itself. The patch got too big, UI patch and patch for mailnews will follow later. This patch can be reviewed and applied independently of those two other patches.

Comment 7

[Frank Wein [:mcsmurf]]

Comment on attachment 282610 [details] [diff] [review]
Shell service only

Forgot the browser-prefs.js change in that patch (only needed when the UI uses the shell service), it's just
+ pref("shell.checkDefaultClient", true);

Comment 8

[Robert Kaiser]

Did you want to request reviews or such as well?

Comment 9

[[e-mail]]

Comment on attachment 282610 [details] [diff] [review]
Shell service only

>+  /**
>+   * app types we can be registered to handle
>+   */
>+  const unsigned short MAIL    = 0x0001;
>+  const unsigned short NEWS    = 0x0002;
>+  const unsigned short BROWSER = 0x0004;
Odd numbering. Can we have browser first please? Makes it easier to add RSS.

>+   * @param aStartupCheck true if this is the check being performed
>+   *                      by the first mail window at startup,
>+   *                      false otherwise.
Delete "mail"

>+   * @param aApps the application types being tested (Mail, News, Browser)
Browser, Mail, News

>+  const long BACKGROUND_TILE      = 1;
>+  const long BACKGROUND_STRETCH   = 2;
>+  const long BACKGROUND_CENTER    = 3;
>+  const long BACKGROUND_FILL      = 4;
What does this last one do? You don't seem to use it anyway.

>+   * entire screen. A rgb value, where (r << 16 | g << 8 | b)
An RGB value (r << 16 | g << 8 | b)

>+static nsresult
>+OpenUserKeyForReading(HKEY aStartKey, const nsAString& aKeyName, HKEY* aKey)
const nsString& aKeyName

>+    if (aStartKey == HKEY_LOCAL_MACHINE) {
>+      // prevent infinite recursion on the second pass through here if
>+      // ::RegOpenKeyEx fails in the all-users case.
>+      return NS_ERROR_NOT_AVAILABLE;
>+    }
>+    return OpenUserKeyForReading(HKEY_LOCAL_MACHINE, aKeyName, aKey);
Eww. I'd just do RegOpenKeyExW(aStartKey) and if that returns not found and the start key isn't HKLM (which it never is, except to support the recursion) then I'd retry with HKLM.

>+  case ERROR_ACCESS_DENIED:
>+    if (aHKLMOnly || aStartKey == HKEY_CURRENT_USER)
>+      return NS_ERROR_FILE_ACCESS_DENIED;
>+    // fallback to HKCU immediately on access denied since we won't be able
>+    // to create the key.
>+    return OpenKeyForWriting(HKEY_CURRENT_USER, aKeyName, aKey, aHKLMOnly);
>+  case ERROR_FILE_NOT_FOUND:
>+    res = ::RegCreateKeyExW(aStartKey, flatName.get(), 0, NULL,
>+                            0, KEY_READ | KEY_WRITE, NULL, aKey,
>+                            NULL);
>+    if (res != ERROR_SUCCESS) {
>+      if (aHKLMOnly || aStartKey == HKEY_CURRENT_USER) {
>+        // prevent infinite recursion on the second pass through here if
>+        // ::RegCreateKey fails in the current user case.
>+        return NS_ERROR_FILE_ACCESS_DENIED;
>+      }
>+      return OpenKeyForWriting(HKEY_CURRENT_USER, aKeyName, aKey, aHKLMOnly);
>+    }
I've really no idea why this is so badly written... why not just RegCreateKeyExW on HKLM and if it fails and it's not HKLM only then on HKCU?

>+//     shell\open\command               (default)         REG_SZ     <apppath> -url "%1" -requestPending
What does -requestPending do, why isn't it always in the same place, and where's -osint?

[Is it possible to register as an HTML document editor?]

>+//   HKCU\SOFTWARE\Classes\SeaMonkey.Url.news (default)   REG_SZ    SeaMonkey (News) URL
>+//                                       DefaultIcon      REG_SZ    <apppath>,0
>+//                                       EditFlags        REG_DWORD 2
>+//     shell\open\command                (default)        REG_SZ    <apppath> -mail "%1"
Not -news?

>+//   The following protocols:
>+//    news,nntp,snews
>+//   are mapped like this:
>+//
>+//   HKCU\SOFTWARE\Classes\<protocol>\   (default)       REG_SZ     SeaMonkey (News) URL
>+//                                       EditFlags       REG_DWORD  2
>+//                                       URL Protocol    REG_SZ
>+//    DefaultIcon                        (default)       REG_SZ     <appath>,0
>+//    shell\open\command                 (default)       REG_SZ     <appath> -mail "%1"
-news again?

>+  // File Extension Class - as of 1.8.1.2 the value for VAL_URL_OPEN is also
1.8.1.2?

>+        DWORD res = ::RegQueryValueExW(theKey, PromiseFlatString(value).get(),
value should already be flat...

>+  const nsString &flatName = PromiseFlatString(keyName);
keyName is already flat etc. etc....

>+#ifndef MOZ_CAIRO_GFX
This is never false.

>+        do {
>+          i -= bpr;
>+
>+          stream->Write(((const char*)bits) + i, bpr, &written);
>+          if (written == bpr) {
>+            rv = NS_OK;
Should set this outside of the loop.

>+  int aParameters[2] = { COLOR_BACKGROUND, COLOR_DESKTOP };
Why are we setting the window background colour?

>+  classDescription: "Default Suite Cmdline Handler",
"Set Default ..."

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #9)
> (From update of attachment 282610 [details] [diff] [review])
> >+  /**
> >+   * app types we can be registered to handle
> >+   */
> >+  const unsigned short MAIL    = 0x0001;
> >+  const unsigned short NEWS    = 0x0002;
> >+  const unsigned short BROWSER = 0x0004;
> Odd numbering. Can we have browser first please? Makes it easier to add RSS.

That'll be because its for use with a bitwise operator...

Now I'm going to dive in with some other comments from the build aspects (I realise you may have got some of these from other apps/my original patch, but I've learnt since then ;-) )

Index: suite/Makefile.in
+ifeq ($(OS_ARCH),WINNT)
+DIRS += shell
+endif

To make it easier to dev other platforms, could all OS enter the shell directory please. To enter the public directory would also be fine, you'd just have to do a global ifdef on the src directory.

I'm very surprised there are no changes to suite/build/Makefile.in in this patch - I'd have expected to see the header includes and a library option change as well.

Index: suite/build/nsSuiteModule.cpp
+#ifdef XP_WIN
+#include "nsWindowsShellService.h"
+NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsWindowsShellService, Init)
+#endif

It would be nice to include this stuff in the #if defined(XP_WIN) sections that are already in that file...

Index: suite/installer/windows/packages

You'll need to add nsSetDefault.js to this file as well.

Index: suite/shell/src/Makefile.in

+REQUIRES = \

do we really need that lot of dependencies? Its fine if we do, it just seems rather large.

+MODULE = shellservice
...
+ifdef CPPSRCS
+LIBRARY_NAME = shellservice_s
+endif
...
+FORCE_STATIC_LIB=1

I'd prefer it if these were all at the start of the file (makes it easier to work out what's happening). Though not sure if that's sensible with LIBRARY_NAME at the moment - but it may be if you put the ifdef for WINNT around the whole lot (but between the .mk files) as I mentioned above.

+ifeq ($(OS_ARCH),WINNT)

+ifeq (,$(filter-out windows,$(MOZ_WIDGET_TOOLKIT)))

These seem a little inconsistent.

Comment 11

[[e-mail]]

(In reply to comment #10)
>(In reply to comment #9)
>>(From update of attachment 282610 [details] [diff] [review] [details])
>>>+  /**
>>>+   * app types we can be registered to handle
>>>+   */
>>>+  const unsigned short MAIL    = 0x0001;
>>>+  const unsigned short NEWS    = 0x0002;
>>>+  const unsigned short BROWSER = 0x0004;
>>Odd numbering. Can we have browser first please? Makes it easier to add RSS.
>That'll be because its for use with a bitwise operator...
In case it wasn't clear, it was the ordering, not the values, that I disliked.

>>+ifeq ($(OS_ARCH),WINNT)
> 
>>+ifeq (,$(filter-out windows,$(MOZ_WIDGET_TOOLKIT)))
> 
>These seem a little inconsistent.
Yes, I'd go with OS_ARCH tests for Windows.

Comment 12

[Frank Wein [:mcsmurf]]

Attachment: Shell service only #2

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: Shell service only #2

Comment 14

[Frank Wein [:mcsmurf]]

Comment on attachment 303932 [details] [diff] [review]
Shell service only #2

All review comments should be fixed with this patch. requestPending is for DDE stuff, see http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsNativeAppSupportWin.cpp#260

Comment 15

[[e-mail]]

Comment on attachment 303932 [details] [diff] [review]
Shell service only #2

>+  /**
>+   * Used to determine whether or not to show a "Set Default Client"
>+   * query dialog. This attribute is true if the application is starting
>+   * up and "shell.checkDefaultClient" is true, otherwise it
>+   * is false.
>+   */
>+  attribute boolean shouldCheckDefaultClient;
We don't actually use this yet do we?

>+OS_LIBS         += $(call EXPAND_LIBNAME,ole32 version uuid)
>+OS_LIBS         += shell32.lib version.lib
Inconsistent. Find out from ted/bsmedberg which one is right?

>+  virtual ~nsWindowsShellService() {};
Shouldn't need to be virtual.

>+  const nsString &flatName = PromiseFlatString(aKeyName);
aKeyName is already flat.

>+  DWORD res = ::RegOpenKeyExW(aStartKey, flatName.get(), 0, KEY_READ, aKey);
>+
>+  switch (res) {
>+  case ERROR_SUCCESS:
>+    break;
>+  case ERROR_ACCESS_DENIED:
>+    return NS_ERROR_FILE_ACCESS_DENIED;
>+  case ERROR_FILE_NOT_FOUND:
>+    if (aStartKey != HKEY_LOCAL_MACHINE) {
>+      // retry with HKEY_LOCAL_MACHINE
>+      return OpenUserKeyForReading(HKEY_LOCAL_MACHINE, aKeyName, aKey);
I still don't like this "recursion".
if (res == ERROR_FILE_NOT_FOUND && aStartKey != HKEY_LOCAL_MACHINE)
  res = ::RegOpenKeyExW(HKEY_LOCAL_MACHINE, aKeyName.get(), 0, KEY_READ, aKey);

>+  }
No default action in the switch. What happens for other errors?

>+  const nsString &flatName = PromiseFlatString(aKeyName);
aKeyName is already flat.

>+  DWORD res = ::RegCreateKeyExW(aStartKey, flatName.get(), 0, NULL,
>+                                0, KEY_READ | KEY_WRITE, NULL, aKey,
>+                                &dwDisp);
>+  
>+  if (REG_FAILED(res)) {
>+    if (!aHKLMOnly && aStartKey != HKEY_CURRENT_USER) {
>+      // fallback to HKCU immediately on error since we won't be able
>+      // to create the key.
>+      return OpenKeyForWriting(HKEY_CURRENT_USER, aKeyName, aKey, aHKLMOnly);
>+    }
>+    return NS_ERROR_FILE_ACCESS_DENIED;
This is almost unreadable!
if (REG_FAILED(res) && aTryHKCU)
  res = ::RegCreateKeyExW(HKEY_CURRENT_USER, aKeyName.get(), ...);

return REG_FAILED(res) ? NS_ERROR_FILE_ACCESS_DENIED : NS_OK;

>+// The DefaultIcon registry key value should never be used (e.g. NON_ESSENTIAL)
>+// when checking if Firefox is the default browser since other applications
>+// (e.g. MS Office) may modify the DefaultIcon registry key value to add Icon
>+// Handlers.
s/Firefox/SeaMonkey/

>+  //   seamonkey.exe\shell\properties   (default)   REG_SZ  SeaMonkey  &Options
Preferences

>+PRBool
>+nsWindowsShellService::TestForDefault(SETTING aSettings[], PRInt32 aSize)
>+{
>+  nsCOMPtr<nsILocalFile> lf;
>+  nsresult rv = NS_NewLocalFile(mAppShortPath, PR_TRUE,
>+                                getter_AddRefs(lf));
>+
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  nsAutoString exeName;
>+  rv = lf->GetLeafName(exeName);
>+  if (NS_FAILED(rv))
>+    return rv;
rv isn't a PRBool

>+          isDefault = PR_FALSE;
>+          break;
Could just return PR_FALSE; (final return would then become return PR_TRUE;)

>+  if (!::GetModuleFileNameW(0, appPath, MAX_BUF))
>+    return NS_ERROR_FAILURE;
That's not quite true ;-)

>+nsWindowsShellService::nsWindowsShellService()
>+:mCheckedThisSessionClient(PR_FALSE)
>+{
>+}
Inline this?

>+    PRBool isDefaultBrowser = PR_TRUE;
>+    PRBool isDefaultMail    = PR_TRUE;
>+    PRBool isDefaultNews    = PR_TRUE;
Should be BOOL, no?

>+    if (aApps & nsIShellService::BROWSER)
>+      hr = pAAR->SetAppAsDefaultAll(APP_REG_NAME);
>+    if (aApps & nsIShellService::MAIL)
>+      hr = pAAR->SetAppAsDefaultAll(APP_REG_NAME_MAIL);
>+    if (aApps & nsIShellService::NEWS)
>+      hr = pAAR->SetAppAsDefaultAll(APP_REG_NAME_NEWS);
Don't set hr, you don't use it again.

>+    res = ::RegDeleteValueW(key, EmptyString().get());
L"" (or I think 0 also works?)

>+      res = DeleteRegKey(key, nsDependentString(subkeyName));
Bad function signature, you don't actually use any nsString methods.

>+  // Set the Options menu item
Preferences

>+  // Set the Safe Mode menu item
Hmm, I couldn't find this - would having a branch SeaMonkey installed confuse things?

>+        do {
>+          i -= bpr;
>+
>+          stream->Write(((const char*)bits) + i, bpr, &written);
>+          if (written == bpr) {
>+            rv = NS_OK;
No point setting rv each time through the loop. Just set it once.

>+    // XXX write background loading stuff!
I hope this is copied code ;-)

>+  // eventually, the path is "%APPDATA%\Mozilla\Firefox\Desktop Background.bmp"
s/Firefox/SeaMonkey/

>+       // The size is always 3 unicode characters.
No it's not, it's two!

>+  int aParameters[2] = { COLOR_BACKGROUND, COLOR_DESKTOP };
Why are we setting two colours?

>+  BYTE r = (aColor >> 16);
>+  BYTE g = (aColor << 16) >> 24;
>+  BYTE b = (aColor << 24) >> 24;
>+  COLORREF colors[2] = { RGB(r,g,b), RGB(r,g,b) };
Ask jag for some better code for this ;-)

>+  // Try to create/open a subkey under HKLM.
>+  DWORD rv = ::RegCreateKeyExW(HKEY_CURRENT_USER,
>+                               L"Control Panel\\Colors", 0, NULL,
>+                               REG_OPTION_NON_VOLATILE, KEY_WRITE, NULL,
>+                               &key, &dwDisp);
Comment wrong?

>+      var shell = Cc["@mozilla.org/suite/shell-service;1"].
>+                  getService(Ci.nsIShellService);
var shell = Components.classes[...]
                      .getService(...);
(use constants for the two ...s if you prefer.)

>+optionsLabel=%S &Options
Preferences (and fix optionsLabel in the code, I overlooked it earlier)

Comment 16

[[e-mail]]

Comment on attachment 303932 [details] [diff] [review]
Shell service only #2

And jar.mn changes are missing.

Comment 17

[[e-mail]]

Comment on attachment 303932 [details] [diff] [review]
Shell service only #2

> 		suitebrowser \
> 		suitemigration \
> 		txmgr \
> 		unicharutil \
> 		windowwatcher \
>+		suitebrowser \
>+		suitemigration \
Duplicates.

Comment 18

[Frank Wein [:mcsmurf]]

Attachment: Shell service only #3

>> +  attribute boolean shouldCheckDefaultClient;
> We don't actually use this yet do we?

Correct, it's used by my UI patch though (will be attached, when this patch is in).

>> >+OS_LIBS         += $(call EXPAND_LIBNAME,ole32 version uuid)
>> >+OS_LIBS         += shell32.lib version.lib
> Inconsistent. Find out from ted/bsmedberg which one is right?

I used $(call ...).

>> +  virtual ~nsWindowsShellService() {};
> Shouldn't need to be virtual.

Fixed.

I also fixed the OpenUserKeyForReading and OpenUserKeyForWriting functions, I hope it's ok now :).

>> +    res = ::RegDeleteValueW(key, EmptyString().get());
> L"" (or I think 0 also works?)

MSDN says NULL works, too.

>> +  res = DeleteRegKey(key, nsDependentString(subkeyName));
> Bad function signature, you don't actually use any nsString methods.

Changed the second param to a LPCWSTR and changed the callees to use L"subkeyName". I also renamed the function name to RegDeleteTree to reflect what it emulates (there is a Windows API function with that name, but it only exists in Vista and above).

>> +  // Set the Safe Mode menu item
> Hmm, I couldn't find this - would having a branch SeaMonkey installed confuse
> things?

Branch uses 8.3 style in the registry, so should not.

Background color code stayed as is because it turned out that another way is not really easier because of the registry write.

>> +  int aParameters[2] = { COLOR_BACKGROUND, COLOR_DESKTOP };
> Why are we setting two colours?

According to MSDN, they set those two set the same. I removed COLOR_DESKTOP.

Everything not mentioned here was fixed.

To test this patch, one can execute this in the JS Console:
Components.classes["@mozilla.org/suite/shell-service;1"].getService(nsIShellService).setDefaultClient(false, true, nsIShellService.BROWSER);
To set mail as default app, just replace BROWSER with MAIL.

Comment 19

[Frank Wein [:mcsmurf]]

Oh and make the "+ Layout" change "+ layout" :).

Comment 20

[Frank Wein [:mcsmurf]]

Attachment: Shell service only #4

Fixed the "Firefox" code comments, fixed the OpenKeyForWriting recursive call, fixed isDefault (removed it and directly return PR_TRUE), changed the wcslen call to !*keyName, renamed optionsLabel to preferencesLabel, fixed the code comment for HKCU.

Comment 21

[[e-mail]]

Comment on attachment 310202 [details] [diff] [review]
Shell service only #4

We're down to the last few nits now, nearly there!
Marking r- because this patch has non-shell-service bits in it.

>+#define NS_SUITEWININTEGRATION_CONTRACTID "@mozilla.org/suite/shell-service;1"
Hmm, is this name right? I guess it's OK. Presumably NS_SUITEMACINTEGRATION_CONTRACTID will have the same value, but NS_SUITEMACINTEGRATION_CID will have a different value.

>+  DWORD  RegDeleteTree(HKEY baseKey, LPCWSTR keyName);
We can't really use an internal name that matches a Windows API, because the Vista SDK's WinUser.h will #define RegDeleteTree RegDeleteTree[A|W]. Call it DeleteRegTree perhaps?

>+  DWORD  DeleteRegKeyDefaultValue(HKEY baseKey, LPCWSTR keyName);
Also, why are these not static methods in the .cpp file?

>+#include "prbit.h"
Not used!

>+OpenUserKeyForReading(HKEY aStartKey, const nsString& aKeyName, HKEY* aKey)
>+OpenKeyForWriting(HKEY aStartKey, const nsString& aKeyName, HKEY* aKey,
>+                  PRBool aHKLMOnly)
I've just noticed that aKeyName doesn't need to be an nsString&, a LPWCSTR will suffice (as per RegDeleteTree and DeleteRegKeyDefaultValue).

>+    res = ::RegDeleteValueW(key, 0);
You mentioned NULL in your Bugzilla comment which might make more sense here.

>+  nsAutoString current(buf);
>+  if (REG_FAILED(res) || !current.Equals(aValue)) {
Doesn't aValue.Equals(buf) work?

>+  nsAutoString optionsKey(NS_LITERAL_STRING(SMI));
>+  optionsKey.Append(exeName);
>+  optionsKey.AppendLiteral("\\shell\\properties");
Please rename all the other occurrences of options to preferences too!

>+  int aParameters[1] = { COLOR_BACKGROUND };
>+  BYTE r = (aColor >> 16);
>+  BYTE g = (aColor << 16) >> 24;
>+  BYTE b = (aColor << 24) >> 24;
>+  COLORREF colors[1] = { RGB(r,g,b) };
>+
>+  ::SetSysColors(sizeof(aParameters) / sizeof(int), aParameters, colors);
I wonder whether it's worth writing these as simple variables instead of arrays and using ::SetSysColors(1, &parameter, &color);

>+const Cc = Components.classes;
Not used.

>+const Ci = Components.interfaces;
Not inlined like I asked ;-)
const nsIShellService = Components.interfaces.nsIShellService;
is OK if you need to avoid wrapping below.

>+      var shell = Components.classes["@mozilla.org/suite/shell-service;1"].
>+                  getService(Ci.nsIShellService);
>+      shell.setDefaultClient(true, true, nsIShellService.BROWSER);
Components.classes["@mozilla.org/suite/shell-service;1"]
          .getService(Components.interfaces.nsIShellService);
(         .setDefaultClient if you want... saves on the var)
Same for mail; do we need -setDefaultNews?

> 		rdf \
> 		string \
> 		suitebrowser \
> 		suitemigration \
> 		txmgr \
>+		shellservice \
Alphabetical order please.

Comment 22

[Frank Wein [:mcsmurf]]

Attachment: Shell service only #5

Ok, all review comments are fixed now. I used aValue.Equals(nsDependentString(buf, len)) for comparing the PRUnichar array to the nsString.

Comment 23

[[e-mail]]

Comment on attachment 316116 [details] [diff] [review]
Shell service only #5

Almost there!

>+#ifndef nswindowsshellservice_h____
>+#define nswindowsshellservice_h____
This hardly seems necessary; there are only the two includes!

>+#include "nsStringApi.h"
Should be nsStringAPI.h in case somebody cross-compiles this.

>+  // Get the old value
>+  DWORD res = ::RegQueryValueExW(theKey, aValueName.get(),
>+                                 NULL, NULL, (LPBYTE)buf, &len);
>+
>+  // Set the new value
>+  if (REG_FAILED(res) || !aValue.Equals(nsDependentString(buf, len))) {
The documentation for RegQueryValueEx says that the string isn't guaranteed to be or not to be null-terminated. Now as it happens you always write null-terminated strings, which means that len is always one more than the actual length of the string (as used by nsDependentString). Except it isn't, because len is in bytes, not in characters...

Conveniently you used almost correct code in TestForDefault (ZeroMemory and Equals without the nsDependentString) so I don't need an additional review of the fix. (IMHO you need to set len to two less than the actual buffer size.)

>+  // Set the Options and Safe Mode start menu context menu item labels
Preferences!

>+  // Set the Options menu item
Preferences!

>+  suite/common/Makefile
>   suite/build/Makefile
>   suite/debugQA/Makefile
>   suite/debugQA/locales/Makefile
>   suite/common/Makefile
Oops ;-)

Comment 24

[[e-mail]]

Comment on attachment 316116 [details] [diff] [review]
Shell service only #5

>+  locale/@AB_CD@/communicator/defaultClientDialog.dtd                       (%chrome/common/defaultClientDialog.dtd)
Don't check this line in yet ;-)

Comment 25

[Frank Wein [:mcsmurf]]

Thanks for all the reviews and the s/options/preferences checks :-P.

Checking in suite/shell/Makefile.in;
/cvsroot/mozilla/suite/shell/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in suite/shell/public/Makefile.in;
/cvsroot/mozilla/suite/shell/public/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in suite/shell/public/nsIShellService.idl;
/cvsroot/mozilla/suite/shell/public/nsIShellService.idl,v  <--  nsIShellService.idl
new revision: 1.3; previous revision: 1.2
done
Checking in suite/shell/src/Makefile.in;
/cvsroot/mozilla/suite/shell/src/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in suite/shell/src/nsWindowsShellService.h;
/cvsroot/mozilla/suite/shell/src/nsWindowsShellService.h,v  <--  nsWindowsShellService.h
new revision: 1.3; previous revision: 1.2
done
Checking in suite/shell/src/nsWindowsShellService.cpp;
/cvsroot/mozilla/suite/shell/src/nsWindowsShellService.cpp,v  <--  nsWindowsShellService.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in suite/shell/src/nsSetDefault.js;
/cvsroot/mozilla/suite/shell/src/nsSetDefault.js,v  <--  nsSetDefault.js
new revision: 1.3; previous revision: 1.2
done
Checking in suite/installer/windows/packages;
/cvsroot/mozilla/suite/installer/windows/packages,v  <--  packages
new revision: 1.55; previous revision: 1.54
done
Checking in suite/build/Makefile.in;
/cvsroot/mozilla/suite/build/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in suite/build/nsSuiteModule.cpp;
/cvsroot/mozilla/suite/build/nsSuiteModule.cpp,v  <--  nsSuiteModule.cpp
new revision: 1.9; previous revision: 1.8
done
Checking in suite/locales/jar.mn;
/cvsroot/mozilla/suite/locales/jar.mn,v  <--  jar.mn
new revision: 1.36; previous revision: 1.35
done
Checking in suite/locales/en-US/chrome/common/shellservice.properties;
/cvsroot/mozilla/suite/locales/en-US/chrome/common/shellservice.properties,v  <--  shellservice.properties
new revision: 1.3; previous revision: 1.2
done
Checking in suite/browser/browser-prefs.js;
/cvsroot/mozilla/suite/browser/browser-prefs.js,v  <--  browser-prefs.js
new revision: 1.90; previous revision: 1.89
done
Checking in suite/Makefile.in;
/cvsroot/mozilla/suite/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
Checking in suite/makefiles.sh;
/cvsroot/mozilla/suite/makefiles.sh,v  <--  makefiles.sh
new revision: 1.12; previous revision: 1.11
done

Comment 26

[Frank Wein [:mcsmurf]]

Attachment: Patch that was checked in

This is the patch that was checked in.

Comment 27

[[e-mail]]

Comment on attachment 316623 [details] [diff] [review]
Patch that was checked in

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsICommandLineHandler]),
Sorry, but I only just noticed this :-(

Comment 28

[Frank Wein [:mcsmurf]]

Fixed that.

Comment 29

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <nsSetDefault.js> (s/handeFlag/handleFlag/) [Checkin: Comment 30]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008042003 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Fixes, (at application startup):
{{
Error: aCmdline.handeFlag is not a function
Source File: file:///.../seamonkey/components/nsSetDefault.js
Line: 64
}}

Frank, can you check it in (preemptively) ?

Comment 30

[Frank Wein [:mcsmurf]]

Checked in.

Comment 31

[Serge Gautherie (:sgautherie)]

Is this bug fixed ?
Or what is left to be done ?

Comment 32

[Frank Wein [:mcsmurf]]

The current UI needs to be changed to use the new shell service. I filed Bug 441050 for that. Closing this bug here.