Closed Bug 788076 Opened 8 years ago Closed 8 years ago

Settings API: Need to notify content processes about the settings changes when calling SettingsService.set() from chrome process

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

(Whiteboard: [LOE:S])

Attachments

(1 file, 1 obsolete file)

Please see https://groups.google.com/d/topic/mozilla.dev.b2g/xl4pbdJWtJY/discussion for the discussion thread. In summary, Gecko should be responsible for notifying all the content processes about the settings changes even if they're changed by the chrome process.
Some Background: we implemented the SettingsService to read settings during startup where we don't have any windows or content processes.
According to :hsinyi's saying, bug 784220 (telephony) recently needs this one to be fixed, because the content process has to keep track of the settings status that could be changed from chrome process. So... we got some business here. ;) I think I can support to upload a patch for solving this issue in a couple of days.

As mentioned by :sicking, this should be a basecamp-blocker now.
Assignee: nobody → clian
blocking-basecamp: --- → ?
Attached patch Patch (obsolete) — Splinter Review
Hi :gwagner :)

I'd like to invite you to take a look on this patch. Some notes:

1. I directly use the existing SettingsChangeNotifier.jsm to listen to the "mozsettings-changed" observer event fired from the chrome process.

2. After receiving the "mozsettings-changed" observer event, it will broadcast "Settings:Change:Return:OK" messages to all the content processes.

3. Note that to avoid redundantly dealing with the "mozsettings-changed" observer event that could be requested from content processes, I use a marker to "fromSettingsChangeNotifier" to distinguish that.
Attachment #659147 - Flags: review?(anygregor)
blocking-basecamp: ? → +
Hi Gregor, may I ping on this one for review? Thanks a lot!
Whiteboard: [LOE:S]
Hi Gregor, sorry for bugging. Ping a bit on this one. Thanks!
Comment on attachment 659147 [details] [diff] [review]
Patch


>+      case kMozSettingsChangedObserverTopic:
>+      {
>+        let setting = JSON.parse(aData);
>+        if (setting.message && setting.message === kFromSettingsChangeNotifier)
>+          return;

Please add a comment here explaining when this happens: "setting.message === kFromSettingsChangeNotifier"
Attachment #659147 - Flags: review?(anygregor) → review+
Attached patch Patch 1.1Splinter Review
:gwagner already had review+ at comment #6.
Attachment #659147 - Attachment is obsolete: true
Attachment #662151 - Flags: review+
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/a04dc1a6fe29
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.