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

Status: RESOLVED FIXED

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

Description

[Gene Lian (I already quit Mozilla)]

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.

Comment 1

[Gregor Wagner [:gwagner]]

Some Background: we implemented the SettingsService to read settings during startup where we don't have any windows or content processes.

Comment 2

[Gene Lian (I already quit Mozilla)]

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.

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: 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.

Comment 4

[Gene Lian (I already quit Mozilla)]

Hi Gregor, may I ping on this one for review? Thanks a lot!

Comment 5

[Gene Lian (I already quit Mozilla)]

Hi Gregor, sorry for bugging. Ping a bit on this one. Thanks!

Comment 6

[Gregor Wagner [:gwagner]]

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"

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Patch 1.1

:gwagner already had review+ at comment #6.

Comment 8

[Gene Lian (I already quit Mozilla)]

Waiting for try server: https://tbpl.mozilla.org/?tree=Try&rev=6bc4798a8b15

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 10

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/a04dc1a6fe29