Closed
Bug 940305
Opened 11 years ago
Closed 10 years ago
Move the extraWarnings flag from ContextOptions to RuntimeOptions
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: ejpbruel, Assigned: bholley)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jandem
:
review+
khuey
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Comment 1•11 years ago
|
||
Try run for this patch:
https://tbpl.mozilla.org/?tree=Try&rev=deea483870b5
We intentionally only set extraWarnings only for chrome code. It doesn't seem like we can do this.
Assignee | ||
Comment 3•11 years ago
|
||
Yes, the patch here is wrong. It will need to have a per-compartment override.
Comment 4•11 years ago
|
||
Shouldn't extraWarnings be in CompartmentOptions, then?
Assignee | ||
Comment 5•11 years ago
|
||
(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.)
This turned out to be pretty spammy so I backed it out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fec954d58b6
Oops. For anyone looking, that checkin was for bug 980558.
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
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).
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Blocks: 1040974
Blocks: 1040976
Comment 18•10 years ago
|
||
Thought I'd check in on this. I think it's just waiting for an okay from Kyle?
Assignee | ||
Comment 19•10 years ago
|
||
(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. :-)
Attachment #8455494 -
Flags: review?(khuey) → review+
(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.
Assignee | ||
Comment 21•10 years ago
|
||
Comment 22•10 years ago
|
||
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1046964
You need to log in
before you can comment on or make changes to this bug.
Description
•