Closed Bug 940305 Opened 6 years ago Closed 6 years ago

Move the extraWarnings flag from ContextOptions to RuntimeOptions

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ejpbruel, Assigned: bholley)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files)

No description provided.
Blocks: 934418
No longer blocks: 934419
Attached patch patchSplinter Review
Try run for this patch:

https://tbpl.mozilla.org/?tree=Try&rev=deea483870b5
Blocks: 909997
No longer blocks: 909997
Depends on: 939562
We intentionally only set extraWarnings only for chrome code. It doesn't seem like we can do this.
Yes, the patch here is wrong. It will need to have a per-compartment override.
Shouldn't extraWarnings be in CompartmentOptions, then?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Shouldn't extraWarnings be in CompartmentOptions, then?

If we don't care about twiddling the default runtime-wide, then yes.
Ideally we'd have some notion of an OptionsGroup or something, and each compartment would just specify its OptionsGroup. Then the runtime would specify the right options for each OptionsGroup.
(And the browser would have one group for chrome and one for content.)
wrong bug number.
Flags: needinfo?(wmccloskey)
Oops. For anyone looking, that checkin was for bug 980558.
Flags: needinfo?(wmccloskey)
Kyle for the workers stuff, Jan for everything else.

I should have split out the workers stuff, but I didn't realized how involved
it would be until I was already partway through it.
Attachment #8455494 - Flags: review?(khuey)
Attachment #8455494 - Flags: review?(jdemooij)
Comment on attachment 8455494 [details] [diff] [review]
Move extraWarnings to RuntimeOptions with a per-compartment override. v1

Review of attachment 8455494 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, sorry for the delay.
Attachment #8455494 - Flags: review?(jdemooij) → review+
It might be nice to the strict option apply only for chrome code, as the strict.debug option does now. It's really weird that they behave differently. Also, I can't imagine anyone wanting to turn on extra warnings for all web content. At most you'd want something in devtools to turn it on for specific pages (which would be an amazingly awesome feature by the way).
(In reply to Bill McCloskey (:billm) from comment #16)
> It might be nice to the strict option apply only for chrome code, as the
> strict.debug option does now. It's really weird that they behave
> differently. Also, I can't imagine anyone wanting to turn on extra warnings
> for all web content. At most you'd want something in devtools to turn it on
> for specific pages (which would be an amazingly awesome feature by the way).

Please file a followup - I'm unlikely to do that in this bug.
Thought I'd check in on this. I think it's just waiting for an okay from Kyle?
(In reply to Ben McCann from comment #18)
> Thought I'd check in on this. I think it's just waiting for an okay from
> Kyle?

Yes. jst is going to ping kyle about this in person later this week. :-)
(In reply to Bobby Holley (:bholley) from comment #19)
> (In reply to Ben McCann from comment #18)
> > Thought I'd check in on this. I think it's just waiting for an okay from
> > Kyle?
> 
> Yes. jst is going to ping kyle about this in person later this week. :-)

If you want to take some b2g blockers let me know.
https://hg.mozilla.org/mozilla-central/rev/d4be8edb114b
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.