123286 - show warning in Error Console when denying from "scripts & windows" a popup window, or other javascript functions (resizeTo,focus,etc)

Status: RESOLVED INACTIVE

(Core :: DOM: Core & HTML, enhancement, P5)

Description

[Nicolás Lichtmaier]

When denying a window.open() a warning should be sent to the JavaScript console,
so that problems can be debugged by power users.

Rationale: There are many sites with valid reasons to call window.open() (e.g.:
http://www.uol.com.ar/, try to open a chat window). Now it fails with no message
(if window.open is forbiden from preferences), if it would have sent a message
to the console I wouldn't have spent some time trying to find if it was an
evangelism bug. =)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Um.. there should be a message (uncaught exception, blah, blah, blah).  Over to
CAPS to investigate.

Comment 2

[Boris Zbarsky [:bzbarsky]]

actually... dom.disable_open_during_load seems to not generate any error....
confirming.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

I don't have time to work on this anytime soon...marking Future.

Comment 4

[Nicolás Lichtmaier]

I have this working, but without showing the JS line number. I'm working on it.

Comment 5

[Nicolás Lichtmaier]

Attachment: It works!

This is what I have so far. It still lacks i18n and perform some accesses to
internal JS structs, but I think it can't be done in other way.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 68337 [details] [diff] [review]
It works!

>+	    nsAutoString message;

No reason for this.  Just use NS_LITERAL_STRING("whatever").get() below.

Also, it would be nice to have the message even if you can't get a JSContext
and all that.... It won't have line numbers, but it's better than nothing.

>+	    nsAutoString sourceName;
>+	    sourceName.AssignWithConversion(fp->script->filename);

NS_ConvertUTF8toUCS2 sourceName(fp->script->filename);

No reason to set "rv" throughout -- you're returning NS_OK no matter what.

And yes, making the string localizable would be good (see nsIStringBundle
and nsIStringBundleService)

Comment 7

[Robert Ginda]

I think we should add a JS_GetContextFileLine(JSContext *cx, char **fileName,
uintN *line); entrypoint to jsdbgapi.h, so that you don't need to include all
those private js header files.

In general, only jsapi.h and jsdbgapi.h should be included by clients, and only
public (JS_*), not private (js_*) entrypoints should be used.

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, I agree.  Wanna toss an additional bug up for that?

Comment 9

[Robert Ginda]

Actually, I had hopes that Nick would roll it into his patch :)

Comment 10

[Nicolás Lichtmaier]

Yes, I'd like to do that. What you have said is exactly what I think should be
done. I'm working on it.
BTW, perhaps this logging code should be moved into a utility function used to
log errors/warnings to the console. Are there other places where this could be
of use? Although it would be a very small function... I don't know...

Comment 11

[Nicolás Lichtmaier]

Uhm.. I now see that it seems that everything could be done with the js debug
API, using JS_FrameIterator and friends. It wouldn't hurt to have an utility
function to do this somewhere though.

Comment 12

[Nicolás Lichtmaier]

Attachment: Now uses the js debug API

Now it uses only public functions from jsdbg.h. I have also reordered things a
little. Still lacks i18n but I think it can be commited (if the patch is ok, of
course).
PS. I am not allowed to mark the other attachment as obsolete. IMO one
shouldn't need a special permission to modify his own attachments.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 68627 [details] [diff] [review]
Now uses the js debug API

psourceName is useless, get rid of it.

Indentation here is a little off:

+	 if ( (scripterror = do_CreateInstance("@mozilla.org/scripterror;1",
&rv)) &&
+		 (consoleService =
do_GetService("@mozilla.org/consoleservice;1", &rv))) {

The second line of the if's condition should underhang the first clause, I say.
 Nit of the day!

Oops, another nit:

+	   if ( ( cs = do_GetService("@mozilla.org/js/xpc/ContextStack;1", &rv)
)
+		 && (!NS_FAILED(cs->Peek(&cx)))) {

This underhangs better, but inconsistently puts the logical connective (&&) on
the second line at the front, not at the end of the first line of the
condition.  I believe jst and I prefer putting that at the end of the first,
but he's module owner, so I defer to him.

Yet another nit: space after comma in parameter list is not used consistently,
e.g. here:

+	     JS_FrameIterator(cx,&fp);

Pleaes be consistent, my little mind's hobgoblin thanks you.

Another inconsistent style bugaboo:

+	       if(script && pc)

Use 'if (' as elsewhere, and in the file's prevailing style.

/be

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 68627 [details] [diff] [review]
Now uses the js debug API

>+          nsAutoString *psourceName = nsnull, sourceName;

What's the point of having psourceName?  You never really use it...

>+              sourceName.AssignWithConversion(JS_GetScriptFilename(cx,script));

Is this filenam ASCII?	Or UTF8?  AssignWithConversion assumes ASCII.

Comment 15

[John Bandhauer]

How about an unpopular opinion: I don't see any good reason to figure out the
script source name and linenumber here. What value is added that justifies more
code?

A message to the console seems to me to be reasonable (though not free
spacewise). But that message *should* indicate that the window.open was denied
because of the *user preference* - the current wording does nothing to make it
clear that the user pref had anything to do with the denial.

But, I don't see any real value added by calculating and showing which code was
denied. Normal users who turn off popups gain nothing - they never look at the
console and wouldn't care which code was running. Power users don't gain much
either - the mention of the pref is enough to clue them in to the idea that they
should turn the pref off if they want to see a popup. The exact location of the
window.open call is just fluf and does not justify ading extra code to the app.

Comment 16

[Brendan Eich [:brendan]]

filenames in JS are ASCII, or perhaps ISO-Latin-1 -- they get inflated by zero
padding each char when making the filename property of Exception objects.

I agree with jband (I just didn't want to be first to be unpopular :-).  Let's
keep footprint down by choosing not to put in small as well as large patches. 
Let's try to take code out, where possible.

/be

Comment 17

[Nicolás Lichtmaier]

This code is *very* small and it should be taken to a utility function so it can
be shared. I can't give reasons to finding the source file and line number,
becuse... it seems so obvious to me that this should be done! The goal should be
that *every error caused by JavaScript code should be correctly pointed to by
the error message*.

Again: Every error caused by Javascript code should be correctly labeled, where
it happened and why it happened. (IMO =) )

Comment 18

[John Bandhauer]

I'm not so sure I'm convinced. Have you identified other locations where this
pattern is useful? The normal error case involves thowing an exception through
xpconnect and letting the JS code deal with it or not. XPConnect logs the
console error for us automatically. Here it was apparently decided that it would
be less disruptive to existing JS web content to just return a null window
rather than throwing. So, the console mechanism is missing. Are there really
other similar places?

Nevertheless, what you want can be done without using the jsapi (much less the
jsdbgapi) since we know the call is coming in through xpconnect and xpconnect
has facilities for getting the JS caller for us. Note that since we are not yet
tracking JS source and linenumbers for event handlers there are plenty of places
where we won't have the info yet anyway.

Though, as I said, I'm not yet convinced, I coded up an alternative
implementation anyway. I'll attach it.

Comment 19

[John Bandhauer]

Attachment: An alternative patch that gets the caller info via xpconnect

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 68827 [details] [diff] [review]
An alternative patch that gets the caller info via xpconnect

Looks good to me, although scriptError (capital E) would match the
consoleService naming convention.

I don't think every null return should have a console message with filename and
line number, but perhaps this is a hard case -- we've made it harder by adding
the dom.disable_open_during_load  preference.

If this is satisfactory to all parties, I will sr= it.

/be

Comment 21

[John Bandhauer]

Hey, I copied nick's names. It's a 'scriptWarning' anyway :)

Isn't jst the module owner here? adding him to the cc list. I don't really care
if this goes in or not. You guys decide :)

Comment 22

[John Bandhauer]

Hmm, let me restate my foolish position...

I agree that emitting a console message is 100% appropriate.

My objections to the code for getting the sourcename and lineno are greatly
mitigated by how small the code shrank by going through xpconnect and the fact
that we are avoiding yet another direct use of js[dbg]api outside of mozilla/js.
I would *much* prefer this console message to be localized. AFAICT (on very
quick inspection) the DOM does not mess with string bundles. This is perhaps not
a good reason to add such a dependency.

Given all that, I'd vote for adding this code.

Comment 23

[Nicolás Lichtmaier]

This seems to be settled, so I think John's code should be checked in. I haven't
tried it, but it looks much nicer than mine.

Well... It's a pitty that it couldn't be my code the one that is checked in. But
I surely like the XPCOM approach better (I've even told rginda this on IRC
before this patch). In any case it was a good excuse to checkout a mozilla
source and see how everything works. Thanks to anyone who has helped me with
this intent to do something. =)

Comment 24

[John Bandhauer]

nick: I hope you are not actually discouraged. Even if some of the code you
wrote is not going in, this would not be happening at all had you not written
the code. There is plenty more to do on this project and we can certainly use
the help.

Comment 25

[Nicolás Lichtmaier]

Hi! I just wanted to note that the fix for bug 117707 makes this code useable in
many places. With that fix, all those settings cause Mozilla to fail without
explaining a thing to the user.

Comment 26

[Nicolás Lichtmaier]

Attachment: Now for some other JS functions too.

Building on John Bandhauer I've created another patch. I've moved the code to
its own function and added calls to it from everywhere.

Comment 27

[Nicolás Lichtmaier]

Attachment: New version of patch.

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 82177 [details] [diff] [review]
New version of patch.

>Index: nsGlobalWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v
>retrieving revision 1.516
>diff -u -r1.516 nsGlobalWindow.cpp
>--- nsGlobalWindow.cpp	27 Apr 2002 00:03:47 -0000	1.516
>+++ nsGlobalWindow.cpp	3 May 2002 07:45:16 -0000
>@@ -143,6 +143,9 @@
> #include "nsIBindingManager.h"
> #include "nsIXBLService.h"
> 
>+// For logging messages to the console
>+#include "nsIScriptError.h"
>+#include "nsIConsoleService.h"
> 
> static nsIEntropyCollector* gEntropyCollector = nsnull;
> static PRInt32              gRefCnt           = 0;
>@@ -214,6 +217,35 @@
>   return !prefValue;
> }
> 
>+static void sendWarningMessage(nsIXPConnect* sXPConnect, nsAFlatString& msg)
>+{
>+   nsCOMPtr<nsIScriptError> 
>+       scriptError(do_CreateInstance("@mozilla.org/scripterror;1"));
>+   nsCOMPtr<nsIConsoleService> 
>+       consoleService(do_GetService("@mozilla.org/consoleservice;1"));
>+   
>+   if (scriptError && consoleService) {
>+     PRInt32 lineno = 0;

Don't you want an unsigned int?  PRUint32?  Also, please use interCaps for the
varname.  lineNum might make more sense.

>+     nsAutoString sourceName;
>+     nsCOMPtr<nsIStackFrame> frame;
>+     sXPConnect->GetCurrentJSStack(getter_AddRefs(frame));
>+     if (frame) {
>+       frame->GetLineNumber(&lineno);
>+       nsXPIDLCString fileName;
>+       if (NS_SUCCEEDED(frame->GetFilename(getter_Copies(fileName)))) {
>+         sourceName.AssignWithConversion(fileName);

avoid AssignWithConversion in favor of NS_LITERAL_STRING, eg

   sourceName.Assign(NS_LITERAL_CSTRING(fileName));

>+       }
>+     }
>+
>+     if (NS_SUCCEEDED(scriptError->Init(msg.get(), sourceName.get(), 
>+                                        nsnull, (uintN)lineno,

And as a result of using a PRUint32 above, you can kill the cast here.

>+                                        0, nsIScriptError::warningFlag, 
>+                                        "content javascript"))) {
>+       consoleService->LogMessage(scriptError);
>+     }
>+   }
>+}
>+
<...>
>   if (!CanSetProperty("dom.disable_window_status_change") && !IsCallerChrome()) {
>+    static NS_NAMED_LITERAL_STRING(msg, 
>+        "Attempt to change window status message denied due to user preference");

We're in the JS console where users expect some code output.  So, please
mention window.status in your message.	Something like

   "Setting window.status denied due to..."

>+    sendWarningMessage(sXPConnect,msg);

Please put a space between arguments here and the same below.

>     return NS_OK;
>   }
> 
>@@ -1389,6 +1424,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_status_change") && !IsCallerChrome()) {
>+    static NS_NAMED_LITERAL_STRING(msg, 
>+        "Attempt to change window default status message denied due to user preference");

Mention window.defaultStatus similar to above.

>+    sendWarningMessage(sXPConnect,msg);
>     return NS_OK;
>   }
> 
>@@ -1448,6 +1486,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_move_resize") && !IsCallerChrome()) {
>+    static NS_NAMED_LITERAL_STRING(msg, 
>+        "window.setInnerWidth() denied due to user preference");

I'd like to prepend "Calling " on to the messages here (and below).  eg,
"Calling window.foo() denied...."

>+    sendWarningMessage(sXPConnect,msg);
>     return NS_OK;
>   }
> 
<...>
>@@ -2864,6 +2938,9 @@
> #ifdef DEBUG
>     printf ("*** Blocking window.open.\n");
> #endif

Should we remove the ifdef above?  We'll have two messages output in debug mode
(one for the printf here, and one for the JS warning)...

>+    static NS_NAMED_LITERAL_STRING(msg, 
>+        "window.open denied due to user preference");
>+    sendWarningMessage(sXPConnect,msg);
>     *_retval = nsnull;
>     return NS_OK;
>   }

Comment 29

[Boris Zbarsky [:bzbarsky]]

>   sourceName.Assign(NS_LITERAL_CSTRING(fileName));

That would _so_ not work.   fileName is not a literal string, after all...

My original question stands however.  Is fileName ASCII?  UTF-8? 
Platform-encoded?  The current code assumes it's ASCII.

Comment 30

[Nicolás Lichtmaier]

Attachment: patch

>>+	PRInt32 lineno = 0;
> Don't you want an unsigned int?  PRUint32?

getFileNumner() expects this.

I have applied everything else.

bz: I've traced the filename. For JavaScipt it seems to be an opaque item.  
I've ran Mozilla in gdb and disocvered who called JS_Evaluate*(). I've found
out 
that "filename" gets assigned from url.get(). So? Is that just ASCII?

Comment 31

[Boris Zbarsky [:bzbarsky]]

That's almost certainly UTF8, then.  So you want to use:

sourceName = NS_ConvertUTF8toUCS2(fileName);

Comment 32

[Nicolás Lichtmaier]

Uhm.. but wouldn't an URL be already "URL-encoded"? I guess this is just plain
ASCII always.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Our internal representation of a URL is almost always UTF8.

Comment 34

[Nicolás Lichtmaier]

Attachment: Now with NS_ConvertUTF8toUCS2!

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

Re-assigning to nick who is working on this. Sorry for being hasty earlier with
my first review.

Anyway, I am against this patch in general because of not many people turning on
strict warnings, its not really even in the UI for some of our browsers.  It
just seems to be extra fluff that we don't really need.  Those users who are
smart enough to turn on strict warnings probably don't need to be told why
scripts are breaking if applicable.  The JS console sucks down enough memory
with real warnings.  I don't think that adding fake warnings to remind me that I
blocked setting window.status is necessary at all.  Were it up to me, this would
be a WONTFIX bug. Of course, this is all IMO.  

That said, the patch looks fine to me and r=caillon if you need it.  However, I
would prefer if you got jband to do the r= since your work is built in part on
some of his work (and vice versa).

Additionaly, I would like to see this question addressed:

Do we really want to warn for *all* of these items?  Warning for window.status
as an example seems a bit overboard.  Back when this bug was filed, setting
window.status while the preference blocked it broke scripts because we used
CAPS, but with my checkin to bug 117707, they are now silently ignored.  I think
that since the only one of these preferences which truly break scripts today is
the window.open one, then that should be the only one we warn about.  When I
block something, I expect it to be silently ignored everywhere.  I don't want to
be reminded in my console everytime someone tries to set window.status.  (For
example those sites with scrolling window.statuses would be unbearable both in
my debug console and my JS console)...

Comment 36

[Johnny Stenback (:jst)]

Comment on attachment 82725 [details] [diff] [review]
Now with NS_ConvertUTF8toUCS2!

+static void sendWarningMessage(nsIXPConnect* sXPConnect, nsAFlatString& msg)

Make this a static member function of GlobalWindowImpl (and uppercase the first
character in the name), that way you don't need to pass in sXPConnect all the
time...

>+    static NS_NAMED_LITERAL_STRING(msg, 
>+        "Setting window.status denied due to user preference");

I don't see a need for those named literals to be static, no need for those
literal string wrappers to remain live after they're used... Remove the static
keyword from all of these...

Change that and I'll sr.

Comment 37

[Nicolás Lichtmaier]

Reply to Christopher:
Stricts warnings? This warnings would be shown to everybody. I haven't checked
the "show JS strict warnings" myself.
I agree that the window.open is the only one that could break scripts, and it's
the one that provide the justification. But adding a warning to the other ones
id free, all the "bloat" has already happened.
Besides, as I said earlier, Mozilla will be used by millions ( =)! ) and we'll
get a lot of bug reports of users who claim that Mozilla is broken, only because
they activated this preference (or a friend did so for them when he installed
mozilla).
> When I block something, I expect it to be silently ignored everywhere.  I
> don't want to be reminded in my console everytime someone tries to set
> window.status.
It's like logging to syslog when somebody tries to break into your computer.
Besides I compare this to accessing window.event, Mozilla logs that every time
and if the code is in a mouse handler you'll see it repeated. However perhaps
something could be done in the future to log once per window (per message type).
I don't know, but anyway I would commit this and make any tidyness work later...
What do you think?

To jst:
Oops.. I didn't realize it was a *static* member. I could make this a member
function. I had thought that nothing was gained since the this pointer would
have needed to be passed anyway, but if it's static member...

Comment 38

[Nicolás Lichtmaier]

Attachment: Now SendWarning is a static member of the class.

This is it.
But I have a doubt.. couldn't I replace those
NS_NAMED_LITERAL_STRING(msg,"...") and just use NS_LITERAL_STRING("...") inside
the function call?

PS: Ugh.. the bug is assigned to me and I'm still not authorized to obsolete my
own attachments.. :(

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 82771 [details] [diff] [review]
Now SendWarning is a static member of the class.

Yes, you could do that, but this is IMO a bit cleaner, and doesn't cost any
more...

sr=jst

Comment 40

[Jesse Ruderman]

I agree with caillon that changing window.status should not put a warning in the
js console.  The most common use for changing window.status is in mouseover
events and the console would fill up quickly.  Besides, it would be silly to put
a warning in the status bar (bug 47128) just to tell the user that a script has
been denied permission to write text in the status bar.

The only prefs that should create warnings are opening windows and maybe
resizing windows.  Focusing windows, changing status bar text, and swapping
images aren't important enough that we need to grab the user's attention, and
they're called so often that the console would fill up quickly (mlk).  The
cookie prefs shouldn't even be there -- scripts should be allowed to store
cookies if and only if the server is allowed to store cookies.

The error message for opening a window should include the URL that the site was
trying to open.  Clicking the link in the console should open it with the
correct referrer and security domain.  (I think the source-code links in the
console already get this right.)  The link should obey the "re-use browser
windows for links opened from other apps" pref (bug 75138), but for now I don't
care whether it opens a new window or re-uses the most recent browser window.

Comment 41

[Nicolás Lichtmaier]

I agree with almost everything you've just said. I will modify the patch.
But I don't think I will be able to implement the "open the window as it would
have been opened by the script". I am not an expert but that would probably need
modifications to the JS console. And.. besides it would be a little
inconsistent. The JS console shows issues in JS code, I wouldn't expect it to
open a content window.

Comment 42

[Boris Zbarsky [:bzbarsky]]

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

Comment 43

[Nicolás Lichtmaier]

Would it be ok if I added a member variable PRUint32 mSomeBits (to be used to
miscelaneous bits =) ) to the GlobalWindowImpl class?

(With a typedef enum { miscBitAlreadyShownWindoStatusMessgae = 1 << 0 })

This bit would be used to show the window.status message just once per document
loaded.

I would clear that bit in GlobalWindowImpl::SetNewDocument().

Comment 44

[Johnny Stenback (:jst)]

Do we really need to go there? I don't think this is important enough, either
show all these messages always, or don't show them at all. Just my thoughts...

Comment 45

[Nicolás Lichtmaier]

What about doing something like this?

    static GlobalWindowImpl *last = NULL; 
    if(last==NULL || this!=last) {
      NS_NAMED_LITERAL_STRING(msg, 
          "Setting window.status denied due to user preference");
      last=this;
    }
?
It would log the first time for each window, with the weird side effect that
logging for a window "clears the flag" for other windows. But the hack would be
self contained.

Comment 46

[Nicolás Lichtmaier]

Attachment: Added i18n

I've removed the logging of window.status by now, I've added the logging of
parameters. I've tested this and it's very nice seeing in the console where did
a server wanted to take us with window.open(). =)
I have added i18n with a helper function, which is a generalization of a
current function (makeScriptDialogTitle), so this won't add any extra bloat.
Now I must make the other code use my generalized function.

Comment 47

[Steve VanSlyck]

Have an icon on the bottom taskbar of the Browser, when Mozilla kills
a pop-up it will show a tiny icon of a bubble bursting, or a baloon
popping - something to show it's working. It can also show new users
(desktop) how good Mozilla is at keeping those pop-ups under control.

Additionally, it would be nice if you could find out the window it
killed. Maybe a log of some sort? And maybe a "pop" sound when it
kills a pop-up, along with the animation.

This entire feature should be modular, though. I mean, you should be
able to disable it in the preferences area.

Comment 48

[Nicolás Lichtmaier]

Attachment: New version

Please, somebody review this version and (hopefuly) check it in. Thanks.

Comment 49

[Johnny Stenback (:jst)]

Comment on attachment 86123 [details] [diff] [review]
New version

+static nsAFlatString &getLocalizedMessage(nsAFlatString &out, const
nsAFlatString &name,
+				 const nsAFlatString *in1 = nsnull,
+				 const nsAFlatString *in2 = nsnull)

Make the next line arguments line up with the first argument on the first line,
and please split the first like so that the return type is on its own line.

+{
+  out.Truncate(0);
+  nsresult rv;
+  nsCOMPtr<nsIStringBundleService> stringBundleService =
+     do_GetService(kCStringBundleServiceCID, &rv);
+
+  if (NS_SUCCEEDED(rv) && stringBundleService) { 

No need to get the rv from do_GetService() and no need to check it, just check
for stringBundleService being non-null, anything else is just redundant.

+    nsCOMPtr<nsIStringBundle> stringBundle;
+    rv = stringBundleService->CreateBundle(kDOMBundleURL,
+	getter_AddRefs(stringBundle));

Since rv is not used, why assign into it?

+    if (stringBundle) {
+      int num;
+      nsXPIDLString tempString;
+      const PRUnichar *formatStrings[2];
+      if (in1!=nsnull) {
+	 formatStrings[0] = in1->get();
+	 if (in2!=nsnull) {

Don't do "if (ptr!=nsnull)", simply "if (ptr)" is more readable.

+	   formatStrings[1] = in2->get();
+	   num=2;
+	 } else
+	   num=1;
+      } else
+	 num=0;
+      rv = stringBundle->FormatStringFromName(name.get(),
+	 formatStrings, num, getter_Copies(tempString));
+      if (tempString)
+	 out.Assign(tempString.get());

Loose the .get() here, that only makes this operation more expensive.

+static inline nsAFlatString &getLocalizedMessage(nsAFlatString &out, const
nsAFlatString &name,
+						PRInt32 anInt)

Again, make sure the arguments line up...

+{
+	nsAutoString anIntString;
+	anIntString.AppendInt(anInt);
+	return getLocalizedMessage(out, name, &anIntString);
+}

Does this really need to be inline? And only indent the code 2 spaces, no
tabs!!!

+
+static inline nsAFlatString &getLocalizedMessage(nsAFlatString &out, const
nsAFlatString &name,
+						PRInt32 anInt, PRInt32
anotherInt)

Same here, argument indentation, and does this really need to be inline?

+{
+	nsAutoString anIntString, anotherIntString;
+	anIntString.AppendInt(anInt);
+	anotherIntString.AppendInt(anotherInt);
+	return getLocalizedMessage(out, name, &anIntString, &anotherIntString);
+}

Again, 2 space indentation, no tabs!!!

+void GlobalWindowImpl::SendWarningMessage(const nsAFlatString& msg)

Put the return type on its own line...

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

Comment 50

[Nicolás Lichtmaier]

Attachment: Tidier version

jst: I've done what you asked. Could you review it?

Thanks!

Comment 51

[Johnny Stenback (:jst)]

Comment on attachment 86563 [details] [diff] [review]
Tidier version

sr=jst

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 86563 [details] [diff] [review]
Tidier version

>+    stringBundleService->CreateBundle(kDOMBundleURL,
>+       getter_AddRefs(stringBundle));

This indentation is odd; the "getter" should line up with "kDOMBundleURL", no?

>+    nsAutoString msg, status(aDefaultStatus);
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                                           "WindowDefaultStatusDenied"),
>+                                           &status));

use PromiseFlatString() instead of the |status| temporary?

>+      nsAutoString aFlatTitle(aTitle);
>+      getLocalizedMessage(title, NS_LITERAL_STRING("ScriptDlgTitle"), &aFlatTitle);

again, use PromiseFlatString()

>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                       "WindowMoveByDenied"),aXDif,aYDif));

spaces after those commas before aXdif, aYdif?

>+                       "WindowResizeToDenied"),aWidth,aHeight));

spaces?

>+                       "WindowResizeByDenied"),aWidthDif,aHeightDif));

spaces?

>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                                           "WindowOpenDenied"),&url));

space before "&url"?

Fix those and r=bzbarsky

Comment 53

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 86563 [details] [diff] [review]
Tidier version

I agree with bz's issues but I have a few more things to add:

>Index: dom/src/base/nsGlobalWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v

> 
>+static nsAFlatString&
>+getLocalizedMessage(nsAFlatString &out,
>+                    const nsAFlatString &name,
>+                    const nsAFlatString *in1 = nsnull,
>+                    const nsAFlatString *in2 = nsnull)

Please use more descriptive names for the arguments.  Use aParam as the style
for the argument name.	Also the out parameter should be the last parameter. 
Just make sure you adjust any code for nsnull in params (which it appears
you're already checking for so you should be fine).  Also, why do you even have
an outparameter if you are just returning it?  That is redundant.  In this
case, I would suggest returning an nsresult (or even void I guess would be
fine), keep the out parameter and not inlining your functions which will reduce
the long lines in other parts of the code.

static
nsresult
getLocalizedMessage(const nsAFlatString *aName,
		    const nsAFlatString *aString1,
		    const nsAFlatString *aString2,
		    nsAFlatString &aResult);

>+{
>+  out.Truncate(0);
>+  nsCOMPtr<nsIStringBundleService> stringBundleService =
>+     do_GetService(kCStringBundleServiceCID);
>+
>+  if (stringBundleService) {
>+    nsCOMPtr<nsIStringBundle> stringBundle;
>+    stringBundleService->CreateBundle(kDOMBundleURL,
>+       getter_AddRefs(stringBundle));
>+    
>+    if (stringBundle) {
>+      int num;
>+      nsXPIDLString tempString;
>+      const PRUnichar *formatStrings[2];
>+      if (in1) {
>+        formatStrings[0] = in1->get();
>+        if (in2) {
>+          formatStrings[1] = in2->get();
>+          num=2;
>+        } else
>+          num=1;
>+      } else
>+        num=0;

I find the |} else foo| hard to read.  If you have brackets in the if, then
also use brackets on the else.	(Yes I know this an uber-nit, but I've had to
pick nits like this from sr=s in the past and I'm just passing it on since I
agree).

>+      stringBundle->FormatStringFromName(name.get(),
>+        formatStrings, num, getter_Copies(tempString));

Please indent the arguments so they are below the first one, eg:

      stringBundle->FormatStringFromName(name.get(),
					 formatStrings,
					 num,
					 getter_Copies(tempString));

>+      if (tempString)
>+        out.Assign(tempString);
>+    }
>+  }
>+  
>+  // Just in case
>+  if (out.IsEmpty()) {
>+    NS_WARNING("could not get string from string bundle");
>+    out=NS_LITERAL_STRING("<unknown>");

out.Assign() ?

>+  }
>+  return out;
>+}
>+
>+static nsAFlatString&
>+getLocalizedMessage(nsAFlatString &out, const nsAFlatString &name,
>+                    PRInt32 anInt)

Again some better param names, move the out param to the end, and if you can't
fit all the params on one line, then stack them under each other.  Same for all
other places below.

<...>

>+
>+void GlobalWindowImpl::SendWarningMessage(const nsAFlatString& msg)
>+{
>+  NS_ASSERTION(msg.get(), "SendWarningMessage() called with msg.get() == NULL");
>+  nsCOMPtr<nsIScriptError> 
>+      scriptError(do_CreateInstance(kScriptErrorContractID));
>+  nsCOMPtr<nsIConsoleService> 
>+      consoleService(do_GetService(kConsoleServiceContractID));
>+  
>+  if (scriptError && consoleService) {
>+    PRInt32 lineNumber = 0;
>+    nsAutoString sourceName;
>+    nsCOMPtr<nsIStackFrame> frame;
>+    sXPConnect->GetCurrentJSStack(getter_AddRefs(frame));
>+    if (frame) {
>+      frame->GetLineNumber(&lineNumber);
>+      nsXPIDLCString fileName;
>+      if (NS_SUCCEEDED(frame->GetFilename(getter_Copies(fileName)))) {
>+        sourceName = NS_ConvertUTF8toUCS2(fileName);
>+      }
>+    }
>+
>+    if (NS_SUCCEEDED(scriptError->Init(msg.get(), sourceName.get(), 
>+                                       nsnull, (uintN)lineNumber,
>+                                       0, nsIScriptError::warningFlag, 
>+                                       "content javascript"))) {
>+      consoleService->LogMessage(scriptError);
>+    }
>+  }
>+}
>+
> //*****************************************************************************
> //***    GlobalWindowImpl: Object Management
> //*****************************************************************************
>@@ -1368,6 +1465,10 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_status_change") && !IsCallerChrome()) {
>+    nsAutoString msg, status(aDefaultStatus);
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                                           "WindowDefaultStatusDenied"),

Ack.  At first glance this looks like "WindowDefaultStatusDenied" is a string
parameter by itself to getLocalizedMessage() because of your indentation.  If
you follow my suggestion above, then yuo can do


  getLocalizedMessage(msg, NS_LITERAL_STRING("WindowDefaultStatusDenied"),
&status);
  SendWarningMessage(status);

which avoids so much happening all on one line...


>+                                           &status));
>     return NS_OK;
>   }
> 
>@@ -1428,6 +1529,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_move_resize") && !IsCallerChrome()) {
>+    nsAutoString msg;
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                       "WindowSetInnerWidthDenied"),aInnerWidth));

Same as above.	Fix this anywhere else it happens in the code.

>     return NS_OK;
>   }
> 

<...>

>@@ -2151,7 +2232,8 @@
>   const PRUnichar *title = nsnull;
>   nsresult rv = CheckSecurityIsChromeCaller(&isChrome);
>   if (NS_FAILED(rv) || !isChrome) {
>-      MakeScriptDialogTitle(NS_LITERAL_STRING(""), newTitle);
>+      NS_NAMED_LITERAL_STRING(emptyString, "");
>+      getLocalizedMessage(newTitle,NS_LITERAL_STRING("ScriptDlgTitle"), &emptyString);
>       title = newTitle.get();
>   }
>   NS_WARN_IF_FALSE(!isChrome, "chrome shouldn't be calling alert(), use the prompt service");
>@@ -2182,7 +2264,8 @@
>   const PRUnichar *title = nsnull;
>   nsresult rv = CheckSecurityIsChromeCaller(&isChrome);
>   if (NS_FAILED(rv) || !isChrome) {
>-      MakeScriptDialogTitle(NS_LITERAL_STRING(""), newTitle);
>+      NS_NAMED_LITERAL_STRING(emptyString, "");
>+      getLocalizedMessage(newTitle, NS_LITERAL_STRING("ScriptDlgTitle"), &emptyString);
>       title = newTitle.get();
>   }
>   NS_WARN_IF_FALSE(!isChrome, "chrome shouldn't be calling confirm(), use the prompt service");
>@@ -2226,7 +2309,8 @@
>   PRBool isChrome = PR_FALSE;
>   rv = CheckSecurityIsChromeCaller(&isChrome);
>   if (NS_FAILED(rv) || !isChrome) {
>-      MakeScriptDialogTitle(aTitle, title);
>+      nsAutoString aFlatTitle(aTitle);
>+      getLocalizedMessage(title, NS_LITERAL_STRING("ScriptDlgTitle"), &aFlatTitle);
>   }
>   else
>   {
>@@ -2328,6 +2412,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_flip") && !IsCallerChrome()) {
>+    nsAutoString msg;
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                                           "WindowFocusDenied")));
>     return NS_OK;
>   }
> 
>@@ -2473,6 +2560,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_move_resize") && !IsCallerChrome()) {
>+    nsAutoString msg;
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                       "WindowMoveToDenied"),aXPos,aYPos));
>     return NS_OK;
>   }
> 
>@@ -2498,6 +2588,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_move_resize") && !IsCallerChrome()) {
>+    nsAutoString msg;
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                       "WindowMoveByDenied"),aXDif,aYDif));
>     return NS_OK;
>   }
> 
>@@ -2527,6 +2620,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_move_resize") && !IsCallerChrome()) {
>+    nsAutoString msg;
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                       "WindowResizeToDenied"),aWidth,aHeight));
>     return NS_OK;
>   }
> 
>@@ -2552,6 +2648,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_move_resize") && !IsCallerChrome()) {
>+    nsAutoString msg;
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                       "WindowResizeByDenied"),aWidthDif,aHeightDif));
>     return NS_OK;
>   }
> 
>@@ -2582,6 +2681,9 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_move_resize") && !IsCallerChrome()) {
>+    nsAutoString msg;
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                                           "WindowSizeToContentDenied")));
>     return NS_OK;
>   }
> 
>@@ -2851,17 +2953,6 @@
>   NS_ENSURE_STATE(sXPConnect);
>   nsresult rv = NS_OK;
> 
>-  /* If we're in a commonly abused state (top level script, running a timeout,
>-   * or onload/onunload), and the preference is enabled, block the window.open().
>-   */
>-  if (CheckForAbusePoint()) {
>-#ifdef DEBUG
>-    printf ("*** Blocking window.open.\n");
>-#endif
>-    *_retval = nsnull;
>-    return NS_OK;
>-  }
>-
>   nsCOMPtr<nsIXPCNativeCallContext> ncc;
> 
>   rv = sXPConnect->GetCurrentNativeCallContext(getter_AddRefs(ncc));
>@@ -2893,6 +2984,17 @@
>         nsJSUtils::ConvertJSValToString(options, cx, argv[2]);
>       }
>     }
>+  }
>+
>+  /* If we're in a commonly abused state (top level script, running a timeout,
>+   * or onload/onunload), and the preference is enabled, block the window.open().
>+   */
>+  if (CheckForAbusePoint()) {
>+    nsAutoString msg;
>+    SendWarningMessage(getLocalizedMessage(msg, NS_LITERAL_STRING(
>+                                           "WindowOpenDenied"),&url));
>+    *_retval = nsnull;
>+    return NS_OK;
>   }
> 
>   return OpenInternal(url, name, options, PR_FALSE, nsnull, 0, nsnull,
>Index: dom/src/base/nsGlobalWindow.h
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.h,v
>retrieving revision 1.187
>diff -u -r1.187 nsGlobalWindow.h
>--- dom/src/base/nsGlobalWindow.h	22 May 2002 00:39:21 -0000	1.187
>+++ dom/src/base/nsGlobalWindow.h	6 Jun 2002 06:42:13 -0000
>@@ -257,6 +257,8 @@
>                        PRBool searchInFrames, PRBool showDialog, 
>                        PRBool *aReturn);
> 
>+  static void SendWarningMessage(const nsAFlatString& msg);
>+
> protected:
>   // When adding new member variables, be careful not to create cycles
>   // through JavaScript.  If there is any chance that a member variable

Comment 54

[Nicolás Lichtmaier]

caillon: I agree with almost nothing of your review.

Anyway, I've started to do the changes out of tiredness (I won't move the out
parameter to be first, that doesn't make sense in this printf wannabe). I'll do
the rest even if I don't agree, I'll send a patch tommorrow.

Comment 55

[Nicolás Lichtmaier]

Attachment: Tidiest version.

Well, I've done all of bz suggestions and some of caillon ones. I've havent
done these things:
1) I haven't put the out argument last. This is supposed to be a function
similar to sprintf, where you have a variable list of arguments at the end.
2) I still return the out value. The idea of a "getTranslatedMessage" IMO is
get out of the view of the casual reader. See how gettext is used in GNU
programs.

I hope this is ok. Please, sbdy sr= (again =( ) and r= this.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91304 [details] [diff] [review]
Tidiest version.

>+      if (in1) {
>+        formatStrings[0] = in1->get();
>+        if (in2) {
>+          formatStrings[1] = in2->get();
>+          num=2;
>+        } else {
>+          num=1;
>+        }
>+      } else {
>+        num=0;
>+      }

This is assuming that you don't get a null |in1| with a non-null |in2|.  Please
add an assertion to that effect (NS_PRECONDITION) to the beginning of your
function to catch possible bad callers in the future.

>+      if (tempString)
>+        out.Assign(tempString);

Could we make that check be |if (!tempString.IsEmpty())| ?

>+      nsAutoString aFlatTitle(aTitle);
>+      getLocalizedMessage(title, NS_LITERAL_STRING("ScriptDlgTitle"), &aFlatTitle);

Like I said in comment 52, use PromiseFlatString here.

Fix those (the first one is _very_ important, the other two I'd like to see
fixed as well) and r/sr=bzbarsky

Comment 57

[Christopher Aillon (sabbatical, not receiving bugmail)]

At the least could you use the "aArgument" style for your function params?  Just
because there is existing code that doesn't follow our style guidelines is no
reason to introduce new code that breaks them... And I still think the names of
your variables are completely useless, but at the least with using aIn1, aIn2,
you'll be able to tell at a glance that it's a parameter, and not just a local
variable...

Comment 58

[Nicolás Lichtmaier]

Attachment: Do I still need a description? =)

Now it uses PromiseFlatString (I had to store it somewhere because I can't take
the address of a temporary).
I've renamed in1 and in2 to aFirstParam and aSecondParam. (My own naming logic
is: short parameter names for short scope variables and long descriptive for
class member or global symbols. That's why I would have used just in1 and in2.)

Comment 59

[Boris Zbarsky [:bzbarsky]]

Two things:

1)  Why can't you take the address of a temporary exactly?
2)  If you really can't:

const nsAString & flatTitle = PromiseFlatString(aTitle);

This using a reference instead of creating another object with a copy
constructor, and not using a "aFoo" pattern (reserved for arguments) for a local
stack variable.

Comment 60

[Nicolás Lichtmaier]

Attachment: Correctly flattened strings

Uhm... PromiseFlatString returns an object by value. After this function has 
returned flatTitle will be a reference to what? Will it be constructed in   
the stack and destroyed when the function ends? I see this is the way is
being done in other parts of the code, but I don't quite understand what's  
happening. Ok, I'll trust you and I'll do it =) (I'll use nsAFlatString& as 
the target type though).

Ah! I thought the "a" was an article, like.. aParameter, anOrange,	    
anObject... like it's seen in some OO languages. =)

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91509 [details] [diff] [review]
Correctly flattened strings

r/sr=bzbarsky

The reference is a reference to the object that was returned; the object will
be destroyed once all references go out of scope.

Comment 62

[Nicolás Lichtmaier]

Attachment: Standarized names for function arguments

Can I just copy the sr= ? I've only changed variable names...

Comment 63

[Nicolás Lichtmaier]

Attachment: Indentation

Comment 64

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 92314 [details] [diff] [review]
Indentation

So it looks like most of my nits I gave nick via IRC were fixed.  A few aren't,
and that's just as well because I have a few more to dish out.

>Index: dom/src/base/nsGlobalWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v
>retrieving revision 1.533
>diff -u -r1.533 nsGlobalWindow.cpp
>--- dom/src/base/nsGlobalWindow.cpp	10 Jul 2002 04:58:59 -0000	1.533
>+++ dom/src/base/nsGlobalWindow.cpp	23 Jul 2002 01:25:58 -0000
...
>@@ -169,6 +172,8 @@
> 
> static const char *kDOMBundleURL = "chrome://global/locale/commonDialogs.properties";
> 
>+static const char *kConsoleServiceContractID = NS_CONSOLESERVICE_CONTRACTID;
>+static const char *kScriptErrorContractID = NS_SCRIPTERROR_CONTRACTID;
> 

Please move those 2 new contract IDs down with the other contract IDs right
below it.  Fix indentation so the = operators align (modifying the others'
indentation if needed)

> 
> static const char * const kCryptoContractID = NS_CRYPTO_CONTRACTID;
>@@ -188,6 +193,102 @@
>   return !prefValue;
> }
> 
>+static nsAFlatString&
>+getLocalizedMessage(nsAFlatString &aOut,
>+                    const nsAString &aName,
>+                    const nsAFlatString *aString1 = nsnull,
>+                    const nsAFlatString *aString2 = nsnull)
>+{
>+  aOut.Truncate();
>+  nsCOMPtr<nsIStringBundleService> stringBundleService =
>+     do_GetService(kCStringBundleServiceCID);
>+
>+  if (stringBundleService) {
>+    nsCOMPtr<nsIStringBundle> stringBundle;
>+    stringBundleService->CreateBundle(kDOMBundleURL,
>+                                      getter_AddRefs(stringBundle));
>+    
>+    if (stringBundle) {
>+      PRUint32 num;
>+      nsXPIDLString tempString;
>+      const PRUnichar *formatStrings[2];
>+      if (aString1) {
>+        formatStrings[0] = aString1->get();
>+        if (aString2) {
>+          formatStrings[1] = aString2->get();
>+          num=2;

Please add spaces between the operands and the operator.   |num = 2;|.	Do the
same for the other instances in this function.

>+        } else {
>+          num=1;
>+        }
>+      } else {
>+        NS_ASSERTION(aString2==nsnull, "getLocalizedMessage() string1 == NULL && string2 != NULL");
>+        num=0;
>+      }
>+      stringBundle->FormatStringFromName(PromiseFlatString(aName).get(),
>+                                         formatStrings, num,
>+                                         getter_Copies(tempString));
>+      if (!tempString.IsEmpty())
>+        aOut.Assign(tempString);
>+    }
>+  }
>+  

Don't add a line of just whitespace.  The extra newline is fine, just make sure
there aren't any spaces there.

>+  // Just in case
>+  if (aOut.IsEmpty()) {
>+    NS_WARNING("could not get string from string bundle");
>+    aOut.Assign(NS_LITERAL_STRING("<unknown>"));
>+  }
>+  return aOut;
>+}
>+
>+static nsAFlatString&
>+getLocalizedMessage(nsAFlatString &aOut, const nsAString &aName,


This is extremely redundant and unncessary.  Don't return a value and also pass
it as an out parameter.  In subsequent code you are doing:

   if (foo) {
     nsAutoString bar;
     getLocalizedMessage(bar, baz, bat);
   }

Please pick either an out parameter and returning an nsresult or just return
the string.
Also, after you do that, please put each parameter on separate lines.

>+                    PRInt32 aInt)
>+{
>+  nsAutoString string;
>+  string.AppendInt(aInt);
>+  return getLocalizedMessage(aOut, aName, &string);
>+}
>+
>+static nsAFlatString&
>+getLocalizedMessage(nsAFlatString &aOut, const nsAString &aName,
>+                    PRInt32 aInt1, PRInt32 aInt2)

Parameters on separate lines as above.	E.G.

  static nsAFlatString&
  getLocalizedMessage(nsAFlatString &aOut, 
		      const nsAString &aName,
		      PRInt32 aInt1, PRInt32 aInt2)


>+{
>+  nsAutoString string1, string2;
>+  string1.AppendInt(aInt1);
>+  string2.AppendInt(aInt2);
>+  return getLocalizedMessage(aOut, aName, &string1, &string2);
>+}
>+
>+void GlobalWindowImpl::SendWarningMessage(const nsAFlatString& aMessage)

Please put the void on a line by itself.

void
GlobalWindowImpl::....

>+{
>+  NS_ASSERTION(aMessage.get(), "SendWarningMessage() called with aMessage.get() == NULL");
>+  nsCOMPtr<nsIScriptError> 
>+      scriptError(do_CreateInstance(kScriptErrorContractID));

We use 2 space indent in this file.  Not 4.


>+  nsCOMPtr<nsIConsoleService> 
>+      consoleService(do_GetService(kConsoleServiceContractID));
>+  

Same as above, plus you have a line of just whitespace.  Remove the extra space
chars please.


>+  if (scriptError && consoleService) {
>+    PRInt32 lineNumber = 0;
>+    nsAutoString sourceName;
>+    nsCOMPtr<nsIStackFrame> frame;
>+    sXPConnect->GetCurrentJSStack(getter_AddRefs(frame));
>+    if (frame) {
>+      frame->GetLineNumber(&lineNumber);
>+      nsXPIDLCString fileName;
>+      if (NS_SUCCEEDED(frame->GetFilename(getter_Copies(fileName)))) {
>+        sourceName = NS_ConvertUTF8toUCS2(fileName);
>+      }
>+    }
>+
>+    if (NS_SUCCEEDED(scriptError->Init(aMessage.get(), sourceName.get(), 
>+                                       nsnull, (uintN)lineNumber,
>+                                       0, nsIScriptError::warningFlag, 
>+                                       "content javascript"))) {

I bypassed this my first time around, but I won't this time.  The arguments are
hard to read like this.  Please break each arg up onto a separate line and
please comment what 0 means here...

   if (...Init(aMessage.get(),
	       sourceName.get(),
	       nsnull,
	       (uintN)lineNumber,
	       0, // column 0
	       nsIScriptError::warningFlag,
	       "content javascript"))) {

>+      consoleService->LogMessage(scriptError);
>+    }
>+  }
>+}
>+
> //*****************************************************************************
> //***    GlobalWindowImpl: Object Management
> //*****************************************************************************
>@@ -1367,6 +1468,11 @@
>    */
> 
>   if (!CanSetProperty("dom.disable_window_status_change") && !IsCallerChrome()) {
>+    nsAutoString msg;

Argh, useless var because of my earlier comment.  And so on through out the
patch...

Comment 65

[Jesse Ruderman]

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

Comment 66

[Wayne Mery (:wsmwk)]

needs someone to take over patch

Comment 67

[3zhangliang3]

[Tracking Requested - why for this release]:

[Blocking Requested - why for this release]:

Comment 68

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.