nsISettingsServiceCallback shouldn't use [implicit_jscontext]

RESOLVED FIXED in Firefox 18

Status

()

defect
RESOLVED FIXED
7 years ago
a month ago

People

(Reporter: bholley, Assigned: gwagner)

Tracking

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
If they're implement in JS, we don't support [implicit_jscontext] or [optional_arg]. We can use the JS context stack here.
(Reporter)

Updated

7 years ago
Summary: nsISettingsService and friends shouldn't use [implicit_jscontext] → nsISettingsServiceCallback shouldn't use [implicit_jscontext]
(Assignee)

Comment 1

7 years ago
Posted patch patch (obsolete) — Splinter Review
Let's hope it's that simple.
(Assignee)

Updated

7 years ago
Assignee: bobbyholley+bmo → anygregor
(Assignee)

Updated

7 years ago
blocking-basecamp: --- → ?
(Reporter)

Comment 3

7 years ago
Awesome, thanks gregor!

FWIW you can just use nsContentUtils::GetCurrentJSContext() and nsContentUtils::GetSafeJSContext.
(Reporter)

Comment 4

7 years ago
Looks like there was some compile nastiness with the thread context stack stuff. Try just including "nsContentUtils.h" and using nsContentUtils::GetCurrentJSContext().
(Assignee)

Comment 5

7 years ago
(In reply to Bobby Holley (:bholley) from comment #4)
> Looks like there was some compile nastiness with the thread context stack
> stuff. Try just including "nsContentUtils.h" and using
> nsContentUtils::GetCurrentJSContext().

Yes I did another try run earlier with some fixes: 
https://tbpl.mozilla.org/?tree=Try&rev=ec51cc01c3ca
(Reporter)

Comment 6

7 years ago
Nice! Some comments:

Please rev the IID on nsISettingsServiceCallback.

Just use NS_ENSURE_TRUE(cx, NS_OK) to check the safe JS context.

You need to enter the compartment of aResult before creating the dependent string.

Updated

7 years ago
blocking-basecamp: ? → +
(Assignee)

Comment 7

7 years ago
Posted patch patchSplinter Review
Attachment #681232 - Attachment is obsolete: true
Attachment #681608 - Flags: review?(bobbyholley+bmo)
(Reporter)

Comment 8

7 years ago
Comment on attachment 681608 [details] [diff] [review]
patch

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

Looks good. r=bholley
Attachment #681608 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 10

7 years ago
Huh B2G doesn't start any more.
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b809cfffa5f7
(Assignee)

Comment 11

7 years ago
Posted patch followupSplinter Review
(Assignee)

Comment 12

7 years ago
Comment on attachment 681676 [details] [diff] [review]
followup

r=mrbkap standing right next to me
Attachment #681676 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/369fe18f5a45
https://hg.mozilla.org/mozilla-central/rev/65a144464450
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This is marked as blocking-basecamp+ but doesn't apply cleanly to Aurora due to it not having dom/system/gonk/TimeZoneSettingObserver.cpp. Please either post a branch-specific patch or let me know if it's OK to just ignore the changes to that file.
(Assignee)

Comment 16

7 years ago
(In reply to Ryan VanderMeulen from comment #15)
> This is marked as blocking-basecamp+ but doesn't apply cleanly to Aurora due
> to it not having dom/system/gonk/TimeZoneSettingObserver.cpp. Please either
> post a branch-specific patch or let me know if it's OK to just ignore the
> changes to that file.

Good question. Let's ask cjones if we want dom/system/gonk/TimeZoneSettingObserver.cpp in aurora.
Flags: needinfo?(jones.chris.g)
(Reporter)

Comment 17

7 years ago
Can we get this landed on aurora? the branch is almost here.
(In reply to Gregor Wagner [:gwagner] from comment #16)
> (In reply to Ryan VanderMeulen from comment #15)
> > This is marked as blocking-basecamp+ but doesn't apply cleanly to Aurora due
> > to it not having dom/system/gonk/TimeZoneSettingObserver.cpp. Please either
> > post a branch-specific patch or let me know if it's OK to just ignore the
> > changes to that file.
> 
> Good question. Let's ask cjones if we want
> dom/system/gonk/TimeZoneSettingObserver.cpp in aurora.

What is this fellow?  Why isn't it on aurora already?
Flags: needinfo?(jones.chris.g)
(Assignee)

Comment 19

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> (In reply to Gregor Wagner [:gwagner] from comment #16)
> > (In reply to Ryan VanderMeulen from comment #15)
> > > This is marked as blocking-basecamp+ but doesn't apply cleanly to Aurora due
> > > to it not having dom/system/gonk/TimeZoneSettingObserver.cpp. Please either
> > > post a branch-specific patch or let me know if it's OK to just ignore the
> > > changes to that file.
> > 
> > Good question. Let's ask cjones if we want
> > dom/system/gonk/TimeZoneSettingObserver.cpp in aurora.
> 
> What is this fellow?  Why isn't it on aurora already?

It's bug 792290 and it was not marked as blocking-basecamp.
Blocks: 813278
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.