60896 - Title bar on "New folder" window shows a wrong text

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P3)

Description

[Koike Kazuhiko]

Title bar on "New folder" window in bookmarks manager shows a wrong text
if you unchecked "Remember passwords for sites that require me to log in"
in [Preferences]-[Advanced]-[Passwords].

This problem was reported on Japanese Bugzilla.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=445

Bug 55787 may be related.

Comment 1

[Teruko Kobayashi]

I can reproduce this on US system.  I do not think this is US problem.

Comment 2

[Teruko Kobayashi]

Correction of my previous comment.
"I do think this is US problem."

The title of New Bookmark is displayed as garbage.

Comment 3

[nhottanscp]

This is a generic problem with JS Prompt() when the password pref is off.
Change component and reassign to [e-mail].

Comment 4

[Stephen P. Morse]

OK, I found the problem.  The single-signon module handles all such dialogs.  
But, unfortunately, it takes two different paths, depending on whether or not 
the password-manager is enabled.  And the non-enabled path was not filling in 
the dialog title.

Attaching patch.

Comment 5

[Stephen P. Morse]

Attachment: patch to initialize dialog title

Comment 6

[Stephen P. Morse]

Note: ignore the last line of the patch regarding the removal of BUFLEN2.  It is 
an unused declaration and is being removed as part of the patch for bug 62057.  
But that patch hasn't been checked in yet (still working its way through the 
approval process), so that explains why the change is showing up in this diff.

Comment 7

[Stephen P. Morse]

Message received from Neeti on Wed, 13 Dec 2000 09:22:56 -0800:

   Looks good to me. r=neeti 

   Neeti 

Comment 8

[Brendan Eich [:brendan]]

My comments prefixed by ***:

   if (!si_GetSignonRememberingPref()){
     nsString realm; // XXX hack

*** realm should be nsAutoString.

     CopyASCIItoUCS2(nsLiteralCString(passwordRealm), realm);
-    return dialog->PromptUsernameAndPassword(dialogTitle, text,
realm.GetUnicode(),
+    PRUnichar * prompt_string = (PRUnichar*)dialogTitle;

*** Why are you casting away const?  That's almost never the right thing.  Here
it is much better to cast away the const, using NS_CONST_CAST of course, only
where absolutely necessary: at the call to Recycle, below.

+    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
+      prompt_string = Wallet_Localize("PromptForData");
+    }
+    res = dialog->PromptUsernameAndPassword(prompt_string, text,
realm.GetUnicode(),
                                              savePassword, user, pwd, pressedOK);

*** [nit] The second line does not underhang the first actual parameter -- it's
overindented by one space.

+    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {

*** don't retest a complicated condition, do synthesize a boolean or equivalent
to test here.  In this case, you already have a cheaper test: (dialogTitle ==
prompt_string), which can be tested just like that provided you keep the type of
prompt_string |const PRUnichar*|.

+      Recycle(prompt_string);

*** NS_CONST_CAST(PRUnichar*, prompt_string);

+    }
+    return res;
   }

*** Please write a common subroutine so the non-trivial code here is not copied
in those two other places changed by the patch.

/be

Comment 9

[Stephen P. Morse]

Attaching new patch to address all of brendan's comments except for the last one 
(common subroutine).  If I'm understanding the request correctly, it would 
involve adding the routine:

PRIVATE const PRUnichar *
si_dialogTitle(dialogTitle) {
  const PRUnichar * prompt_string = dialogTitle;
  if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
    prompt_string = Wallet_Localize("PromptForData");
  }
  return prompt_string;
}

and the replacing the following lines of code (or their equivalents) in all 
three routines:

-    const PRUnichar * prompt_string = dialogTitle;
-    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
-      prompt_string = Wallet_Localize("PromptForData");
-    }
+    const PRUnichar prompt_string = si_dialogTitle(dialogTitle);

I'll do this if you really want, but I don't think that it buys us that 
much and it makes it a little harder to read.

Comment 10

[Stephen P. Morse]

Attachment: patch to address reviewers comments

Comment 11

[Stephen P. Morse]

Of course I meant to write "const PRUnichar*" instead of "const PRUnichar" in 
the last line of code above.

Comment 12

[Brendan Eich [:brendan]]

Look at the patch I reviewed, and tell me what differs among (1):

 if (!si_GetSignonRememberingPref()){
     nsString realm; // XXX hack
     CopyASCIItoUCS2(nsLiteralCString(passwordRealm), realm);
-    return dialog->PromptUsernameAndPassword(dialogTitle, text,
realm.GetUnicode(),
+    PRUnichar * prompt_string = (PRUnichar*)dialogTitle;
+    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
+      prompt_string = Wallet_Localize("PromptForData");
+    }
+    res = dialog->PromptUsernameAndPassword(prompt_string, text,
realm.GetUnicode(),
                                              savePassword, user, pwd, pressedOK);
+    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
+      Recycle(prompt_string);
+    }
+    return res;
   }

(2):

 if (!si_GetSignonRememberingPref()){
     nsString realm; // XXX hack
     CopyASCIItoUCS2(nsLiteralCString(passwordRealm), realm);
-    res = dialog->PromptPassword(dialogTitle,
+    PRUnichar * prompt_string = (PRUnichar*)dialogTitle;
+    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
+      prompt_string = Wallet_Localize("PromptForData");
+    }
+    res = dialog->PromptPassword(prompt_string,
                                  text, realm.GetUnicode(), savePassword,
                                  pwd, pressedOK);
+    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
+      Recycle(prompt_string);
+    }
     return res;
   }

and (3):

 if (!si_GetSignonRememberingPref()){
     nsString realm; // XXX hack
     CopyASCIItoUCS2(nsLiteralCString(passwordRealm), realm);
-    res = dialog->Prompt(dialogTitle, text, realm.GetUnicode(), savePassword,
defaultText, resultText, pressedOK);
+    PRUnichar * prompt_string = (PRUnichar*)dialogTitle;
+    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
+      prompt_string = Wallet_Localize("PromptForData");
+    }
+    res = dialog->Prompt(prompt_string, text, realm.GetUnicode(), savePassword,
defaultText, resultText, pressedOK);
+    if (dialogTitle == nsnull || nsCRT::strlen(dialogTitle) == 0) {
+      Recycle(prompt_string);
+    }
     return res;
   }

Why could not all three of these be replaced by calls to one common subroutine,
with appropriate parameters?

/be

Comment 13

[Stephen P. Morse]

They differ in the routine being called and the type and number of parameters 
passed to that routine.

In (1) we have:

   res = dialog->PromptUsernameAndPassword(prompt_string,
                                           text,
                                           realm.GetUnicode(),
                                           savePassword,
                                           user,
                                           pwd,
                                           pressedOK);

In (2) we have:

   res = dialog->PromptPassword(prompt_string,
                                text,
                                realm.GetUnicode(),
                                savePassword,
                                pwd,
                                pressedOK);

and in (3) we have:

   res = dialog->Prompt(prompt_string,
                        text,
                        realm.GetUnicode(),
                        savePassword,
                        defaultText,
                        resultText,
                        pressedOK);

Comment 14

[Brendan Eich [:brendan]]

The only type and number difference is with dialog->PromptUsernameAndPassword,
which takes an extra in wstring param, user.  It's not hard to unify the glue
code here (use a mighty if statement, use function pointers, use a combination
of those two).  It saves brainprint, source complexity, and probably instruction
footprint (but the last needs to be verified on a major platform).

/be

Comment 15

[Stephen P. Morse]

Attachment: patch that uses subroutine

Comment 16

[Brendan Eich [:brendan]]

Thanks for trying the subroutine approach out -- does it actually save code
space?  One final nit: how about using switch (dlg) {...} rather than an if-else
chain?  switch fits dense enum better and lets the compiler optimize with a jump
table.

/be

Comment 17

[Brendan Eich [:brendan]]

Optimized code space, of course.  Win32 and Linux would be interesting, whatever
is convenient.

/be

Comment 18

[Stephen P. Morse]

Attachment: patch using switch instead of if-statement

Comment 19

[Stephen P. Morse]

I doubt that it actually saves code space.  There's so much additional parameter 
passing to this new routine.

Comment 20

[Brendan Eich [:brendan]]

You doubt wrongly.  Optimized Linux build, gcc 2.91.66 / RH6.1, your "patch to
address reviewers comments" (12/13/00 12:12):

 text	   data	    bss	    dec	    hex	filename
  41643	   1336	      0	  42979	   a7e3	singsign.o

"patch using switch instead of if-statement":

 text	   data	    bss	    dec	    hex	filename
  41371	   1336	      0	  42707	   a6d3	singsign.o

There is a gcc warning about res possibly being used before set due to the
default: case in the switch.  I think it's ok to remove that case, given the
static-ness of the subroutine and the sole proprietorship of the source file. An
alternative that doesn't affect optimized code (where the assertion is off, so
there's no point in the default: case as written) is to #ifdef DEBUG the default
case.

/be

Comment 21

[Stephen P. Morse]

Attachment: patch with default case if-def'ed out

Comment 22

[Brendan Eich [:brendan]]

sr=[e-mail], with a cautionary tale about avoiding break; after the
last case in a switch: that cost AT&T a big black eye when one of their premier
digital switches in the late 80's crashed at the wrong time, due to a missing
break (it had been last, then someone added another case below it, if memory
serves -- perils of C's fall-through semantics).

Nit, for future reference really: Mozilla house style founded on principles of
name collision avoidance ("prompt" is very common) would make those enumerators
ALL_CAPS, not interCaps, in name-style.

/be

Comment 23

[Stephen P. Morse]

Fix checked in.

Comment 24

[Terri Preston]

This is an old bug and a coding issue, therefore marking verified