287943 - Rebrand bootstrap's Windows bits including nsNativeAppSupportWin.cpp

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Ian Neal]

See list below (though there may be more):

/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 274 -- * Mozilla registers the
DDE application "Mozilla" and currently
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 737 -- #define
MOZ_DDE_APPLICATION "Mozilla"
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 900 --
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -browser" );
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 903 --
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -mail" );
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 906 --
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -editor" );
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 909 --
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -addressbook" );
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 912 --
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -kill" );
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 970 --
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla" );
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 1437 -- url.Insert( "mozilla
-url ", 0 );
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 1611 -- url.Insert( "mozilla
-url ", 0 );
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 1851 -- // window when given
"mozilla -foobar" or the like.
/xpfe/bootstrap/nsNativeAppSupportWin.cpp, line 2155 -- static char
procPropertyName[] = "MozillaProcProperty";

(perhaps nsWindowsHooks.cpp too)

Comment 1

[Robert Kaiser]

Attachment: part of a patch, only things that I felt remotely sure about :)

OK, this patch should be a base to build upon. I didn't build it and fixed
mostly comments, along with the DDE name (be sure to have my current rebranding
patch in, else you won't get the MOZ_APP_DISPLAYNAME from Makefile.in!).

I was preparing this as part of my rebranding patch, but then noticed this bug
is around.
That patch also gives you #defines for MOZ_APP_DISPLAYNAME (earlier "Mozilla")
and MOZ_APP_NAME (the binary's name, earlier "mozilla") so you can use those in
thise file as well.

Those are the bits I was still unsure of and didn't touch:

mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:900:		 
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -browser" );
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:903:		 
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -mail" );
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:906:		 
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -editor" );
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:909:		 
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -addressbook" );
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:912:		 
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla -kill" );
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:970:	     
(void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)"mozilla" );
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:1437:			 
url.Insert( "mozilla -url ", 0 );
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:1611:		  url.Insert(
"mozilla -url ", 0 );
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:1851:	  // window when given
"mozilla -foobar" or the like.
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:2155:static char
procPropertyName[] = "MozillaProcProperty";
mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:2417:		 
exitText.Assign( NS_LITERAL_STRING("E&xit Mozilla") );
mozilla/xpfe/bootstrap/splash.rc:70: ID_DDE_APPLICATION_NAME,		
"Mozilla"
mozilla/xpfe/bootstrap/splash.rc:71:	IDS_STARTMENU_APPNAME,		   
"Mozilla"

Comment 2

[Alex Vincent [:WeirdAl]]

Accidentally reset bug owner.  Sorry.

Comment 3

[Robert Kaiser]

add
mozilla/xpfe/bootstrap/module.ver:1:WIN32_MODULE_DESCRIPTION=Mozilla
to the list of things that need a look for win32...

...and yes, this module.ver is lame... I have a better one lying around here :)

Comment 4

[Robert Kaiser]

This is a tier-1 platform and therefore it's blocking 1.0alpha.
If we have no generic solution in place within a week or so (when Gecko trunk
freezes), we should just do hardcoded rebranding of those bits for now.

Comment 5

[Bernd]

when changing this file, please fix bug 289860 along the lines

Comment 6

[Robert Kaiser]

Attachment: patch v1: untested, but hopefully working

This patch should fix all but comment 5 actually (I don't have a clue about
that stuff). Please test it, I have no windows build system available and don't
really know C++, so it can easily be broken or non-functional...

Comment 7

[Robert Kaiser]

Attachment: patch including registry fixup, still untested

This patch includes the requested registry function fixup (comment 5). Thanks
to CTho for doing that part of it :)

It still needs testing, as I'm not on Windows...

Comment 8

[Alex Vincent [:WeirdAl]]

m:/mozilla\xpfe\bootstrap\nsNativeAppSupportWin.cpp(2420) : error C2308:
concatenating mismatched wide strings
make[3]: *** [nsNativeAppSupportWin.obj] Error 2

Comment 9

[Robert Kaiser]

Attachment: patch that hopefully compiles (v3)

Thanks ajvincent, I should have listened better to what Neil said, the concat
doesn't work there. Here's a patch that should actually compile...

Comment 10

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

Comment on attachment 180474 [details] [diff] [review]
patch that hopefully compiles (v3)

(can you add -p to your diff flags?)

-#define MOZ_DDE_APPLICATION	"Mozilla"
+#define MOZ_DDE_APPLICATION	MOZ_APP_DISPLAYNAME

hmm, that actually seems unused... mind removing it? (same for the OS/2
version)

+      const unsigned char ddeexec[] = "\"%1\",,-1,0,,,,";

the type of the argument is "const BYTE*", so why not use BYTE instead of
unsigned char here?

+			::strlen( mAppName ) );

this is wrong for the Ex version:

"If the data is of type REG_SZ, REG_EXPAND_SZ, or REG_MULTI_SZ, cbData must
include the size of the terminating null character or characters."

(ie, add +1 to the length)

+      const unsigned char topic[] = "WWW_OpenURL";

as above (BYTE)

+	     exitText.Assign( NS_LITERAL_STRING( "E&xit " ) );

exitText.AssignLiteral( "E&xit" ) );

hmm, shouldn't this be localizable...

+	     exitText.Append( NS_LITERAL_STRING(
NS_STRINGIFY(MOZ_APP_DISPLAYNAME) ) );

AppendLiteral

+WIN32_MODULE_COMPANYNAME=Mozilla

hmm, not mozilla.org?

Comment 11

[Robert Kaiser]

> (can you add -p to your diff flags?)

OK, I've added it in .cvsrc so my future patches should use it...

> -#define MOZ_DDE_APPLICATION	"Mozilla"
> +#define MOZ_DDE_APPLICATION	MOZ_APP_DISPLAYNAME
> 
> hmm, that actually seems unused... mind removing it? (same for the OS/2
> version)

We don't use it anywhere? Interesting...
That means the comments above are wrong as well?

[RegSetValueEx things were done by CTho, I hope he can comment on them]

> +	     exitText.Assign( NS_LITERAL_STRING( "E&xit " ) );
> 
> exitText.AssignLiteral( "E&xit" ) );

nope.
Neil asked bsmedberg about that, and bsmedberg said that
AssignLiteral/AppendLiteral may not be safe in nsNativeAppSuport*, so we better
should leave it this way.
 
> hmm, shouldn't this be localizable...

Also no. Actually, it is. The code here is only called as a failover if no
localized strings are found. That's why it's inside a check for exitText.IsEmpty()

> +WIN32_MODULE_COMPANYNAME=Mozilla
> 
> hmm, not mozilla.org?

Good point. We have "Mozilla" ther also for Firefox, but mozilla.org may
actually be better.

Comment 12

[Robert Kaiser]

Attachment: patch v4: address biesi's review nits

This patch fixes biesi's nits. I also added a comment to note that the DDE
application name is set via splash.rc (at least it's the only other place where
something like that is set, and the define of nsNativeSupport* really looks
unused)

Comment 13

[Robert Kaiser]

Comment on attachment 180579 [details] [diff] [review]
patch v4: address biesi's review nits

requesting approval: SeaMonkey-only change for making Win navtive app support
rebrandable

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 180579 [details] [diff] [review]
patch v4: address biesi's review nits

a=asa

Comment 15

[Robert Kaiser]

checked in.