385872 - Strict warnings should only be enabled for chrome by default (in debug builds)

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Bug 383365 recently enabled strict warnings in debug builds by default, for both chrome and content.

Seeing a zillion strict warnings every time I load almost any web page is annoying.  Strict warnings should only be enabled by default (in debug builds) for chrome.

Comment 1

[Justin Dolske [:Dolske]]

Bug 248528, comment 12 suggests a solution.

Comment 2

[Brendan Eich [:brendan]]

You break it, you buy it. Please fix this soon, it's a noise tax on every developer using a DEBUG build.

/be

Comment 3

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Created attachment 271584 [details] [diff] [review]
patch

Comment 4

[Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important)]

Possibly nsContentUtils::IsCallerChrome?

Comment 5

[Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important)]

Er, ignore me, never mind, forgot what this bug was about.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 271584 [details] [diff] [review]
patch

>+#ifdef DEBUG
>+  // In debug builds, warnings are always enabled in chrome context
>+  // Note this callback is also called from context's InitClasses thus we don't
>+  // need to enable this directly from InitContext
>+  if ((newDefaultJSOptions & JSOPTION_STRICT) == 0) {
>+    nsIScriptGlobalObject *global = context->GetGlobalObject();
>+    if (global) {
>+      nsresult rv;
>+      nsCOMPtr<nsIDOMChromeWindow> chromeWindow(do_QueryInterface(global, &rv));
>+      if (NS_SUCCEEDED(rv))

Would prefer the one-argument form of do_QueryInterface and a null test of chromeWindow to the rv block local being passed and tested. Simpler source (don't care about compiled code here, but that may be shorter too).

Ok with me as SR given the above nit picked, asking jst to review.

/be

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 271584 [details] [diff] [review]
patch

+    nsIScriptGlobalObject *global = context->GetGlobalObject();
+    if (global) {
+      nsresult rv;
+      nsCOMPtr<nsIDOMChromeWindow> chromeWindow(do_QueryInterface(global, &rv));
+      if (NS_SUCCEEDED(rv))

Yeah, check chromeWindow here and don't bother with the nsresult, and also remove the if (global) check as do_QueryInterface() does that check for you.

r=jst with that.

Comment 8

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Created attachment 271634 [details] [diff] [review]
as checked in

mozilla/browser/app/profile/firefox.js 1.186
mozilla/dom/src/base/nsJSEnvironment.cpp 1.331