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

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: Gene Lian (I already quit Mozilla))

Tracking

Trunk
mozilla18
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S])

Attachments

(1 attachment, 1 obsolete attachment)

3.76 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.

Updated

5 years ago
Blocks: 784220
(Assignee)

Comment 2

5 years ago
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: --- → ?
(Assignee)

Comment 3

5 years ago
Created attachment 659147 [details] [diff] [review]
Patch

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: ? → +
(Assignee)

Comment 4

5 years ago
Hi Gregor, may I ping on this one for review? Thanks a lot!
Whiteboard: [LOE:S]
(Assignee)

Comment 5

5 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 662151 [details] [diff] [review]
Patch 1.1

:gwagner already had review+ at comment #6.
Attachment #659147 - Attachment is obsolete: true
Attachment #662151 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
(In reply to Gene Lian [:gene] from comment #8)
> Waiting for try server: https://tbpl.mozilla.org/?tree=Try&rev=6bc4798a8b15

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a04dc1a6fe29
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a04dc1a6fe29
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.