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

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

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

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

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

Corrected some silly stuff.

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

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

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

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.

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

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

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

Updated patch to use brandShortName, and try to incorporate jsts comments. jst: Could you take another look at this?

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.

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

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 on attachment 173191 [details] [diff] [review] version 1.0 Either way would've been fine with me :) r+sr=jst

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)

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