811414 - nsISettingsServiceCallback shouldn't use [implicit_jscontext]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

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

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Let's hope it's that simple.

Comment 2

[Gregor Wagner [:gwagner]]

Try build:
https://tbpl.mozilla.org/?tree=Try&rev=0f1f47b59b9c

Comment 3

[Bobby Holley (:bholley)]

Awesome, thanks gregor!

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Gregor Wagner [:gwagner]]

(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

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 8

[Bobby Holley (:bholley)]

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

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

Looks good. r=bholley

Comment 9

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f4b5d48c73

Comment 10

[Gregor Wagner [:gwagner]]

Huh B2G doesn't start any more.
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b809cfffa5f7

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: followup

Comment 12

[Gregor Wagner [:gwagner]]

Comment on attachment 681676 [details] [diff] [review]
followup

r=mrbkap standing right next to me

Comment 13

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65a144464450
https://hg.mozilla.org/integration/mozilla-inbound/rev/369fe18f5a45

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/369fe18f5a45
https://hg.mozilla.org/mozilla-central/rev/65a144464450

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 16

[Gregor Wagner [:gwagner]]

(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.

Comment 17

[Bobby Holley (:bholley)]

Can we get this landed on aurora? the branch is almost here.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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?

Comment 19

[Gregor Wagner [:gwagner]]

(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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af6d073474d1
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ab2a011cbaf