Closed Bug 145523 Opened 18 years ago Closed 15 years ago

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

Categories

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

defect
Not set
trivial

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla7, Assigned: vhaarr+bmo)

References

()

Details

(Keywords: helpwanted, Whiteboard: [good first bug])

Attachments

(2 files, 6 obsolete files)

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.
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

Status: UNCONFIRMED → NEW
Ever confirmed: true
Browser, not engine. Not sure of correct component. DOM? 
Assignee: rogerl → jst
Component: JavaScript Engine → DOM Other
QA Contact: pschwartau → gerardok
This would be a good first patch for someone.  ;)
Keywords: helpwanted
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. :-)
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
Whiteboard: [good first bug]
Attached patch First patch (obsolete) — Splinter Review
I have changed the wording slightly from the original to avoid the %brand%
issue.
Attached patch Second version (obsolete) — Splinter Review
Corrected some silly stuff.
Attachment #139073 - Attachment is obsolete: true
Attached patch Improved comments (obsolete) — Splinter Review
Attachment #139074 - Attachment is obsolete: true
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)))
Attached patch Version 4 (obsolete) — Splinter Review
Sorry 'bout the spam. Last patch wasn't tested well enough, this one is. I also
made the changes suggested by Mike Connor.
Attachment #139075 - Attachment is obsolete: true
Attachment #139094 - Flags: review?(jst)
A testcase for the patch/bug can be found here:
http://bugzilla.mozilla.org/attachment.cgi?id=72198&action=view
Status: NEW → ASSIGNED
There's a whole lot of work being duplicated between this and bug 78089.
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.
Attachment #139094 - Flags: review?(jst) → review-
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...
Attached patch A more concise patch (obsolete) — Splinter Review
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()?
Attachment #139094 - Attachment is obsolete: true
Attachment #139371 - Flags: review?(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.
Attachment #139371 - Flags: review?(jst) → review+
*** Bug 162690 has been marked as a duplicate of this bug. ***
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? 
That sounds like a flash plugin dialog, not a mozilla one.
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.
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.
*** Bug 248151 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.0+
Flags: blocking-aviary1.0+ → blocking-aviary1.0?
Flags: blocking-aviary1.0? → blocking-aviary1.0-
Attached patch version 0.3 (obsolete) — Splinter Review
Updated patch to use brandShortName, and try to incorporate jsts comments.

jst: Could you take another look at this?
Assignee: general → bugmail
Attachment #139371 - Attachment is obsolete: true
Attachment #172804 - Flags: superreview?(jst)
Attachment #172804 - Flags: review?(jst)
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.
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).
> +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 on attachment 172804 [details] [diff] [review]
version 0.3

r+sr=jst with the suggested changes to the wording in this dialog.
Attachment #172804 - Flags: superreview?(jst)
Attachment #172804 - Flags: superreview+
Attachment #172804 - Flags: review?(jst)
Attachment #172804 - Flags: review+
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
(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...
Yeah, I'll second Neil's proposal.
> 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
Attached patch version 1.0Splinter Review
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?
Attachment #172804 - Attachment is obsolete: true
Attachment #173191 - Flags: superreview?(jst)
Attachment #173191 - Flags: review?(jst)
Comment on attachment 173191 [details] [diff] [review]
version 1.0

Either way would've been fine with me :) r+sr=jst
Attachment #173191 - Flags: superreview?(jst)
Attachment #173191 - Flags: superreview+
Attachment #173191 - Flags: review?(jst)
Attachment #173191 - Flags: review+
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)
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 281026
*** Bug 308359 has been marked as a duplicate of this bug. ***
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.