145523 - Script warning should use the brand name instead of "mozilla"

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andy Lyttle]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc1)
Gecko/20020417
BuildID:    2002041711

In this alert dialog:

"A script on this page is causing mozilla to run slowly. If it continues to run,
your computer may become unresponsive.\n\nDo you want to abort the script?"

"mozilla" should be replaced with the brand name.  I've seen &brandShortName; in
other places; something equivalent should be used here (sorry, I'm not a
coder!).  In any case, Mozilla should be capitalized.

The text is in /dom/src/base/nsJSEnvironment.cpp

Reproducible: Always
Steps to Reproduce:
1. load a page with a script that produces that error

Actual Results:  "mozilla" isn't capitalized

Expected Results:  it should be capitalized, and for builds released as other
brand names, the appropriate name should be used.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Confirmed.  This code should

1)  Use a stringbundle for this string instead of hardcoding it!
2)  Use "brandShortName" from
    xpfe/global/resources/locale/en-US/brand.properties

Comment 2

[Phil Schwartau]

Browser, not engine. Not sure of correct component. DOM? 

Comment 3

[Boris Zbarsky [:bzbarsky]]

This would be a good first patch for someone.  ;)

Comment 4

[Andy Lyttle]

Hush!  My first patch was in bug 110945, which was much simpler. >:o

The problem is in dom/src/base/nsJSEnvironment.cpp lines 327-331:
  NS_NAMED_MULTILINE_LITERAL_STRING(msg,
      NS_L("A script on this page is causing mozilla to ")
      NS_L("run slowly. If it continues to run, your ")
      NS_L("computer may become unresponsive.\n\nDo you ")
      NS_L("want to abort the script?"));

The solution that bz suggested is ripping the text out of there entirely, and
using a string bundle in a .properties file instead.  In the .properties file,
we'd use %brandShortName% in place of "mozilla", which is defined in
xpfe/global/resources/locale/en-US/brand.dtd I believe.

So, the question is, where to put this text?  I couldn't find an obvious place,
but was thinking of creating dom/resources/locale/en-US and putting it in there.

bz, didn't you say xpfe doesn't always get build (on embedded systems, etc.)? 
So, would referring to something in xpfe/global/resources/locale/en-US/brand.dtd
be a bad thing?

Somebody who knows what they're doing, please comment. :-)

Comment 5

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 6

[Mikko Virkkilä]

Attachment: First patch

I have changed the wording slightly from the original to avoid the %brand%
issue.

Comment 7

[Mikko Virkkilä]

Attachment: Second version

Corrected some silly stuff.

Comment 8

[Mikko Virkkilä]

Attachment: Improved comments

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 139075 [details] [diff] [review]
Improved comments

>   // Open the dialog.
>-  if (NS_FAILED(prompt->Confirm(title.get(), msg.get(), &ret)))
>+  if (NS_FAILED(prompt->ConfirmEx(title, msg, 
>+                                  (BUTTON_TITLE_IS_STRING * BUTTON_POS_0) + 
>+                                  (BUTTON_TITLE_IS_STRING * BUTTON_POS_1),
>+                                  stopButton,
>+                                  waitButton,
>+                                  nsnull, //Third button
>+                                  nsnull, //Checkbox msg
>+                                  nsnull, //Checkbox val
>+                                  &ret)))
>     return JS_TRUE;
> 
>   return !ret;

the comments aren't needed, ConfirmEx is widely used and well defined in the
IDL.  The formatting could be tightened up to without confusing anyone.

if (NS_FAILED(prompt->ConfirmEx(title, msg, 
				(BUTTON_TITLE_IS_STRING * BUTTON_POS_0) + 
				(BUTTON_TITLE_IS_STRING * BUTTON_POS_1),
				stopButton, waitButton,
				nsnull, nsnull, nsnull, &ret)))

Comment 10

[Mikko Virkkilä]

Attachment: Version 4

Sorry 'bout the spam. Last patch wasn't tested well enough, this one is. I also
made the changes suggested by Mike Connor.

Comment 11

[Mikko Virkkilä]

A testcase for the patch/bug can be found here:
http://bugzilla.mozilla.org/attachment.cgi?id=72198&action=view

Comment 12

[Nicholas Allen]

There's a whole lot of work being duplicated between this and bug 78089.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 139094 [details] [diff] [review]
Version 4

+  nsXPIDLString title;
+  rv = bundle->GetStringFromName(NS_LITERAL_STRING("KillScriptTitle").get(),
getter_Copies(title));
+  //GetStringFromName can return NS_OK and NULL title
+  NS_ENSURE_TRUE(NS_SUCCEEDED(rv), JS_TRUE);
+  NS_ENSURE_TRUE(title, JS_TRUE);
+  
+  nsXPIDLString msg;
+  rv = bundle->GetStringFromName(NS_LITERAL_STRING("KillScriptMessage").get(),
getter_Copies(msg));
+  //GetStringFromName can return NS_OK and NULL msg
+  NS_ENSURE_TRUE(NS_SUCCEEDED(rv), JS_TRUE);
+  NS_ENSURE_TRUE(msg, JS_TRUE);
...

This is very verbose (as most of our string bundle code seems to be), maybe
combine this into something like this to cut down in the lines of code here?:

+  nsXPIDLString title, msg, ...;
+  rv = bundle->GetStringFromName(NS_LITERAL_STRING("KillScriptTitle").get(),
getter_Copies(title));
+  rv |=
bundle->GetStringFromName(NS_LITERAL_STRING("KillScriptMessage").get(),
getter_Copies(msg));
...
+  //GetStringFromName can return NS_OK and NULL msg
+
+  if (NS_FAILED(rv) || !title || !msg || !...) {
+    NS_ERROR("Failed to get strings from dom.properties!");
+
+    return JS_TRUE;
+  }

(note the "rv |=")

Change that, and I'll have one more look.

Comment 14

[Johnny Stenback (:jst)]

Oh, and btw, I'd rather see this go in first and then we can worry about the
added stuff that bug 78089 does, smaller steps...

Comment 15

[Mikko Virkkilä]

Attachment: A more concise patch

This is a more concise version of the patch, implementting the changes
suggested in comment #13. I also changed the cases when I was using two
consecutive NS_ENSURE_TRUEs to just using one. Is this level of brevity enough
or should all checking be done in the if()?

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 139371 [details] [diff] [review]
A more concise patch

+  nsCOMPtr<nsIStringBundleService>
+    stringService(do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv));
+  NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && stringService, JS_TRUE);

Checking one thing is enough here, I'd suggest you eliminate the &rv argument
to do_GetService() and just check for a non-null stringService.

+  nsCOMPtr<nsIStringBundle> bundle;
+  rv = stringService->CreateBundle(kDOMStringBundleURL,
getter_AddRefs(bundle));
+  NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && bundle, JS_TRUE);

Same here, just check for bundle being non-null, IMO.

+  PRInt32 buttonPressed = 1; //In case user exits dialog by clicking X

   // Open the dialog.
-  if (NS_FAILED(prompt->Confirm(title.get(), msg.get(), &ret)))
+  if (NS_FAILED(prompt->ConfirmEx(title, msg, 
+				   (nsIPrompt::BUTTON_TITLE_IS_STRING * 
+				    nsIPrompt::BUTTON_POS_0) + 
+				   (nsIPrompt::BUTTON_TITLE_IS_STRING * 
+				    nsIPrompt::BUTTON_POS_1),
+				   stopButton, waitButton,
+				   nsnull, nsnull, nsnull, &buttonPressed)))
     return JS_TRUE;

-  return !ret;
+  return buttonPressed != 0;

Wanna just store the return from ConfirmEx() in rv and end the method with:

+  return NS_FAILED(rv) || buttonPressed != 0;

r=jst with those changes.

Comment 17

[Johnny Stenback (:jst)]

*** Bug 162690 has been marked as a duplicate of this bug. ***

Comment 18

[[e-mail]]

I am seeing a similar dialog when loading http://www.amena.com/ : "A script in
this movie is causing Macromedia Flash Player 7 to run slowly. If it continues
to run, your computer may become unresponsive. Do you want to abort the script?" 
The only button shown is "OK" - there is no "Cancel" or "No" button - so the
whole dialog does not make any sense.

This appears in both FF 0.9.3 and Mozilla 1.8a3 (both Linux). Is this related to
this bug here? 

Comment 19

[Johnny Stenback (:jst)]

That sounds like a flash plugin dialog, not a mozilla one.

Comment 20

[[e-mail]]

Attachment: Screenshot of the dialog

Here is a screenshot of the dialog - it is word for word the same as the one
mentioned here apart from "Macromedia Flash Player 7". I am pretty sure it is
NOT a flash dialog.

Comment 21

[Johnny Stenback (:jst)]

I get a dialog too, but it's got a Yes and a No button in it, and it's not
through our code (verified in the debugger). Maybe they copied the wording from
Mozilla (which copied the wording from IE initially)? It's odd that you're only
getting an Ok button... Our code should present you with an Ok and a Cancel button.

Comment 22

[jon rekai]

*** Bug 248151 has been marked as a duplicate of this bug. ***

Comment 23

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.3

Updated patch to use brandShortName, and try to incorporate jsts comments.

jst: Could you take another look at this?

Comment 24

[Robert Kaiser]

I'd propose the following wording for those dom.properties strings:

+KillScriptTitle=Warning: Script still running
+# %S will be replaced by brandShortName
+KillScriptMessage=A script on this page is still running and might slow down
%S. You can stop execution of the script or continue to run it, which might make
your computer unresponsive.
+StopScriptButton=Stop script
+WaitForScriptButton=Continue

Reasons:
- The title in patch v 0.3 looks like we were bad on script performance. The
script isn't actually slow ;-)
- We should still tell of a script "on this page", and we should probably
explain what the buttons do. (Perhaps it would be good to add an empty line
between "%S." and "You can" in my wording)
- It's good if the stop button is bigger than the continue button (it's easier
clicked), and from the explanation, a user does see what "Continue" does mean here.

Comment 25

[Vidar Haarr (not reading bugmail)]

Thanks for looking into the wording, Kairo. I'll make the necessary changes
after we've had code review (also in case someone would like to comment on your
proposal).

Comment 26

[Boris Zbarsky [:bzbarsky]]

> +KillScriptMessage=A script on this page is still running and might slow down
> %S.

I don't really like this, (esp the "still running" part), but I'm not sure what
a good way to express "this script may be an infinite loop" is...

> You can stop execution of the script or continue to run it, which might make
> your computer unresponsive.

This sentence is unclear as to what it is that might make the computer
unresponsive.  It sounds like stopping execution could do it too.  Perhaps, "If
the script continues to run, your computer may become unresponsive.  Do you want
to keep running the script?"

With "Yes" and "No" as possible responses.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 172804 [details] [diff] [review]
version 0.3

r+sr=jst with the suggested changes to the wording in this dialog.

Comment 28

[[e-mail]]

How about:
+KillScriptTitle=Warning: Unresponsive script
+KillScriptMessage=A script on this page may be busy, or it may have stopped
responding. You can stop the script now, or you can continue to see if the
script will complete.
+StopScriptButton=Stop script
+WaitForScriptButton=Continue

Comment 29

[Robert Kaiser]

(In reply to comment #26)
> I don't really like this, (esp the "still running" part), but I'm not sure what
> a good way to express "this script may be an infinite loop" is...

I don't know a better worind either... but then, you're the native english
speaker here :)

> > You can stop execution of the script or continue to run it, which might make
> > your computer unresponsive.
> 
> This sentence is unclear as to what it is that might make the computer
> unresponsive.  It sounds like stopping execution could do it too.  Perhaps, "If
> the script continues to run, your computer may become unresponsive.  Do you want
> to keep running the script?"
> 
> With "Yes" and "No" as possible responses.

I'm with you that it may be unclear about that. The problem with your wording is
though that users tend to click "Yes", which would mean continuing the script
with your wording. That's one reason I like the dialog with "Stop Script" and
"Continue" buttons, it's easier to see which button means what.

Reversing my explanantion ("You can continue to run the script, which might make
your computer unresponsive, or stop its execution.") clears up the meaning but
is completely against the bzutton order, which is why I don't like it as well...

(In reply to comment #28)
> How about:
> +KillScriptTitle=Warning: Unresponsive script
> +KillScriptMessage=A script on this page may be busy, or it may have stopped
> responding. You can stop the script now, or you can continue to see if the
> script will complete.
> +StopScriptButton=Stop script
> +WaitForScriptButton=Continue

Now I know why Neil is the UI owner! ;-)
That sounds quite good to me...

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yeah, I'll second Neil's proposal.

Comment 31

[Andy Lyttle]

> I'm with you that it may be unclear about that. The problem with your wording is
> though that users tend to click "Yes", which would mean continuing the script
> with your wording. That's one reason I like the dialog with "Stop Script" and
> "Continue" buttons, it's easier to see which button means what.

Precisely.  Yes and No require the user to read the text carefully in order to determine what the buttons 
will do; labeling the buttons with verbs allow the impatient user to merely glance at the text.  From 
Apple's Human Interface Guidelines:  "Button names should correspond to the action the user performs 
when pressing the button—for example, Erase, Save, or Delete."
http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/
XHIGDialogs/chapter_9_section_2.html

Comment 32

[Vidar Haarr (not reading bugmail)]

Attachment: version 1.0

Removes the brand bundle.
Uses Neils proposed wording.

jst: I considered being bold and forwarding your r/sr, but thought the better
of it .. Or?

Comment 33

[Johnny Stenback (:jst)]

Comment on attachment 173191 [details] [diff] [review]
version 1.0

Either way would've been fine with me :) r+sr=jst

Comment 34

[Vidar Haarr (not reading bugmail)]

Checked in by silver%warwickcompsoc.co.uk.
Thanks! Marking FIXED.

(please note that the original title and description of this bug don't apply
anymore, since we don't use names in the dialog now)

Comment 35

[:Gavin Sharp [email: [e-mail]]]

*** Bug 308359 has been marked as a duplicate of this bug. ***