Strict warnings should only be enabled for chrome by default (in debug builds)
RESOLVED
FIXED
in Firefox 3 alpha7
Status
()
People
(Reporter: jruderman, Assigned: mano)
Tracking
({regression})
Bug Flags:
Firefox Tracking Flags
(Not tracked)
Details
Attachments
(1 attachment, 1 obsolete attachment)
4.40 KB,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Bug 248528, comment 12 suggests a solution.
Comment 2•12 years ago
|
||
You break it, you buy it. Please fix this soon, it's a noise tax on every developer using a DEBUG build. /be
Assignee: nobody → mano
(Assignee) | ||
Comment 3•12 years ago
|
||
Created attachment 271584 [details] [diff] [review] patch
Attachment #271584 -
Flags: superreview?(brendan)
Attachment #271584 -
Flags: review?(brendan)
(Assignee) | ||
Updated•12 years ago
|
Severity: minor → normal
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Firefox 3 M7
Comment 4•12 years ago
|
||
Possibly nsContentUtils::IsCallerChrome?
Comment 5•12 years ago
|
||
Er, ignore me, never mind, forgot what this bug was about.
Comment 6•12 years ago
|
||
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
Attachment #271584 -
Flags: superreview?(brendan)
Attachment #271584 -
Flags: superreview+
Attachment #271584 -
Flags: review?(jst)
Attachment #271584 -
Flags: review?(brendan)
Comment 7•12 years ago
|
||
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.
Attachment #271584 -
Flags: review?(jst) → review+
(Assignee) | ||
Comment 8•12 years ago
|
||
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
Attachment #271584 -
Attachment is obsolete: true
(Assignee) | ||
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•