773999 - Switch windows shell service to nsIWindowsRegKey

Status: RESOLVED FIXED

(SeaMonkey :: OS Integration, defect)

Description

[[e-mail]]

This also allows us to port two new background constants from Firefox.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Ian Neal]

Comment on attachment 642281 [details] [diff] [review]
Proposed patch

On code inspection / comparison:
>+    int style = 0;
>+    switch (aPosition) {
>+      case BACKGROUND_TILE:
>+        style = 1;
In the Firefox code both BACKGROUND_TILE and BACKGROUND_CENTER have style = 0. Looking at http://code.msdn.microsoft.com/windowsdesktop/CSSetDesktopWallpaper-2107409c the valid values are 0, 2, 6 and 10. I guess, at the moment, the registry ignores the first bit but we probably shouldn't rely on this undocumented (potentially depreciated - information from 8-10 years ago does mention 1) behaviour.

>+    if (aPosition != BACKGROUND_TILE)
>+      value.Assign('0');
Depending on the resolution to the first point, this might have to change.

r=me with that addressed.

Comment 3

[[e-mail]]

Attachment: Fixed patch

You're right, I totally overlooked that. (Perhaps I need to switch vim to a colour scheme with more contrast; I already have trouble reading comments in the default dark blue...)

f? is because IanN can't actually test this, so additional verification would be great. (Tweak the BACKGROUND_ setting in nsContextMenu.js if you want to give the patch a serious workout.)

Comment 4

[Ian Neal]

Comment on attachment 642363 [details] [diff] [review]
Fixed patch

r=me assuming the feedback is good

Comment 5

[Edmund Wong (:ewong)]

Comment on attachment 642363 [details] [diff] [review]
Fixed patch

Runs well with no hiccups that I've noticed on my Vista build.

Comment 6

[Philip Chee]

>  [e-mail] 2012-07-15 16:39:05 PDT
> Blocks: 774130nsIWindowsRegKey 
Surely Gnome doesn't use nsIWindowsRegKey?

Comment 7

[[e-mail]]

It needs the .idl change.

Comment 8

[Philip Chee]

Comment on attachment 642363 [details] [diff] [review]
Fixed patch

Tested on Windows 7. Also tested various options FILL, TILE, FIT.

> -  nsAutoString path;
> +  nsString path;
Just curious, why this change?
(I also notice that Firefox shell service is using the external string API).

Comment 9

[[e-mail]]

(In reply to Philip Chee from comment #8)
> > -  nsAutoString path;
> > +  nsString path;
> Just curious, why this change?
a) nsAutoString is just a typedef in the external API anyway.
b) The string never gets extended, which is where an nsAutoString shines
   (an nsString often needs to be reallocated before it can be extended.)

Comment 10

[[e-mail]]

Pushed comm-central changeset c79e0a077cac.