785298 - Settings API: set() can carry a customized message to notify "mozsettings-changed" observers

Status: RESOLVED FIXED

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

Description

[Gene Lian (I already quit Mozilla)]

We hope to support Settings API to be able to carry a customized message to notify "mozsettings-changed" observers in addition to key and value. This would be helpful when we want to distinquish who is calling the set(). For example, if we can know it is set from an internal service, we can check the message (containing the caller's identification) to avoid processing it again after receiving "mozsettings-changed" event.

Comment 1

[Gene Lian (I already quit Mozilla)]

I should be able to upload a patch later. ;)

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi :gwagner,

May I invite you to have a review on this one? Please see the Description for the motivation. These changes should be compatible with the current JS codes but not for C++ codes, so we need to add |JSVAL_NULL| to explicitly specify the new |aMessage| field. Please let me know if I can do anything better. :)

Comment 3

[Gene Lian (I already quit Mozilla)]

Oops... try server Win64 build is red when using |JSVAL_NULL| (https://tbpl.mozilla.org/?tree=Try&rev=b0999a0faaf8). I'll upload a new patch with |JS::NullValue()|.

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Upload a new patch using |JS::NullValue()| instead of |JSVAL_NULL| which fails to build for Win64.

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to Gene Lian [:gene] from comment #4)
> Created attachment 655900 [details] [diff] [review]
> Patch, V2
> 
> Upload a new patch using |JS::NullValue()| instead of |JSVAL_NULL| which
> fails to build for Win64.

Looks good to me but please add some tests for messages.

Comment 6

[Gene Lian (I already quit Mozilla)]

(In reply to Gregor Wagner [:gwagner] from comment #5)
> (In reply to Gene Lian [:gene] from comment #4)
> > Created attachment 655900 [details] [diff] [review]
> > Patch, V2
> > 
> > Upload a new patch using |JS::NullValue()| instead of |JSVAL_NULL| which
> > fails to build for Win64.
> 
> Looks good to me but please add some tests for messages.

Thanks Gregor! :) I was trying to write some xpcshell testing for this. However, I cannot get the service of @mozilla.org/observer-service;1.

EST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: file:///home/gene/build/dist/bin/components/SettingsService.js :: SettingsService :: line 152"  data: no]

Can't we get that service by a desktop build? What do you think where is the right place to test if message is successfully carried when receiving "mozsettings-changed" event? Any quick suggestion is highly appreciated! :)

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to Gene Lian [:gene] from comment #6)
> (In reply to Gregor Wagner [:gwagner] from comment #5)
> > (In reply to Gene Lian [:gene] from comment #4)
> > > Created attachment 655900 [details] [diff] [review]
> > > Patch, V2
> > > 
> > > Upload a new patch using |JS::NullValue()| instead of |JSVAL_NULL| which
> > > fails to build for Win64.
> > 
> > Looks good to me but please add some tests for messages.
> 
> Thanks Gregor! :) I was trying to write some xpcshell testing for this.
> However, I cannot get the service of @mozilla.org/observer-service;1.
> 
> EST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned
> failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)
> [nsIJSCID.getService]"  nsresult: "0x80570016
> (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame ::
> file:///home/gene/build/dist/bin/components/SettingsService.js ::
> SettingsService :: line 152"  data: no]
> 
> Can't we get that service by a desktop build? What do you think where is the
> right place to test if message is successfully carried when receiving
> "mozsettings-changed" event? Any quick suggestion is highly appreciated! :)

Did you write new tests or did you extend the current tests in 
https://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestSettingsAPI.cpp

Comment 8

[Gene Lian (I already quit Mozilla)]

(In reply to Gregor Wagner [:gwagner] from comment #7)
> Did you write new tests or did you extend the current tests in 
> https://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestSettingsAPI.
> cpp

Thanks for the suggestions. I think I can directly extend the TestSettingsAPI.cpp. Should be able to upload a patch for this today.

Comment 9

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, SettingsService

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Test Cases

Hi Gregor :)

I tried to extend the TestSettingsAPI.cpp by adding an observer to listen to the "mozsettings-changed" event. The test cases not only test if the new messages are successfully passed but also test the existing key and value. The following shows the testing log, where you can see 2 cases for observers are added.

TEST-PASS | Start TestSettingsAPI
Running TestSettingsAPI tests...
TEST-PASS | boolean
TEST-PASS | Lock order
TEST-PASS | integer
TEST-PASS | double
TEST-PASS | Lock order
TEST-PASS | null
TEST-PASS | observer
TEST-PASS | observer
TEST-PASS | END!
Finished running TestSettingsAPI tests.

Please feel free to let me know if I can do anything better. :)

Comment 11

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #10)
> Created attachment 656796 [details] [diff] [review]
> Part2, Test Cases
> 
> Hi Gregor :)
> 
> I tried to extend the TestSettingsAPI.cpp by adding an observer to listen to
> the "mozsettings-changed" event. The test cases not only test if the new
> messages are successfully passed but also test the existing key and value.
> The following shows the testing log, where you can see 2 cases for observers
> are added.
> 
> TEST-PASS | Start TestSettingsAPI
> Running TestSettingsAPI tests...
> TEST-PASS | boolean
> TEST-PASS | Lock order
> TEST-PASS | integer
> TEST-PASS | double
> TEST-PASS | Lock order
> TEST-PASS | null
> TEST-PASS | observer
> TEST-PASS | observer
> TEST-PASS | END!
> Finished running TestSettingsAPI tests.
> 
> Please feel free to let me know if I can do anything better. :)

Oops.. try server build doesn't pass () :(

Hi Gregor,

May I ask what is the root cause of the following line when trying to use nsObserverService? I'll also try to figure it out at the same time but would appreciate if you could please give any quick hints.

###!!! ASSERTION: Using observer service after XPCOM shutdown!: 'Error', file ../../../xpcom/ds/nsObserverService.cpp, line 110

You can find the full log at: https://tbpl.mozilla.org/?tree=Try&rev=1c111923081b

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #11)
> ###!!! ASSERTION: Using observer service after XPCOM shutdown!: 'Error',
> file ../../../xpcom/ds/nsObserverService.cpp, line 110

Hmm... it seems it happens when it tries to call RemoveObserver() where the XPCOM has already been shut down. I'll try to call sTestSettingsObserver = nullptr; to explicitly do it before the end of main().

https://tbpl.mozilla.org/?tree=Try&rev=1935ff88abb2

Rush to home! Hope to get good news tomorrow!

Comment 13

[Gregor Wagner [:gwagner]]

Comment on attachment 656793 [details] [diff] [review]
Part1, SettingsService

> 
>-[scriptable, uuid(3ab3cbc0-8513-11e1-b0c4-0800200c9a66)]
>+[scriptable, uuid(d7a395a0-e292-11e1-834e-1761d57f5f99)]
> interface nsISettingsServiceLock : nsISupports
> {
>-  void set(in string aName, in jsval aValue, in nsISettingsServiceCallback aCallback);
>+  void set(in string aName,
>+           in jsval aValue,
>+           in nsISettingsServiceCallback aCallback,
>+           [optional] in jsval aMessage);


Why is aMessage a jsval and not a string? Isn't the common use-case that you
want to pass a string-message? I don't think a boolean or numeric message is very useful.

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, SettingsService, V2

Hi Gregor,

Change the message from jsval to a string type, which should already cover common use cases. Thanks for the suggestions! :)

Comment 15

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Test Cases, V2

Objectives:

1. Test if the observer can receive "mozsettings-changed" event when calling .set().
2. If yes, check if we can get the right 'key'.
3. If yes, check if we can get the right 'value'.
4. If yes, check if we can get the right 'message', which should be null for case #1 and "test.observer.message" for case #2.

The remaining parts are some minor code clean-ups. Also, please find try server result (green!) at https://tbpl.mozilla.org/?tree=Try&rev=1a4df439af55 and let me know if I can do anything better. :)

Comment 16

[Gene Lian (I already quit Mozilla)]

This one should also be a basecamp-blocker because it blocks bug 729877 which is basecamp+.

Comment 17

[Gregor Wagner [:gwagner]]

Comment on attachment 657180 [details] [diff] [review]
Part1, SettingsService, V2


>-[scriptable, uuid(3ab3cbc0-8513-11e1-b0c4-0800200c9a66)]
>+[scriptable, uuid(d7a395a0-e292-11e1-834e-1761d57f5f99)]
> interface nsISettingsServiceLock : nsISupports
> {
>-  void set(in string aName, in jsval aValue, in nsISettingsServiceCallback aCallback);
>+  void set(in string aName,
>+           in jsval aValue,
>+           in nsISettingsServiceCallback aCallback,
>+           [optional] in string aMessage);
>+

We will have to change string to astring but that's a followup.

Comment 18

[Gregor Wagner [:gwagner]]

Comment on attachment 657181 [details] [diff] [review]
Part2, Test Cases, V2


> 
>+static bool VerifyJSValue(
>+  JSContext *cx, const JS::Value &value, const char *string) {
>+  JSBool match;
>+  if (!value.isString() ||
>+      !JS_StringEqualsAscii(cx, value.toString(), string, &match) ||
>+      (match != JS_TRUE)) {
>+    return false;
>+  }
>+  return true;
>+}

You only check for string values so please rename to
VerifyJSValueIsSting or similar.



>   while (callbackCount > 0)
>     NS_ProcessNextEvent(current);
> 
>+  while (observerCount > 0 && !errors)
>+    NS_ProcessNextEvent(current);
>+

Combine the two whiles.

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Test Cases, V2.1

:gwagner already has review+ at comment 18.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(Remarkably) Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=cf7f719d7f47

https://hg.mozilla.org/integration/mozilla-inbound/rev/dff1055c2f50
https://hg.mozilla.org/integration/mozilla-inbound/rev/304f86e12b60

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dff1055c2f50
https://hg.mozilla.org/mozilla-central/rev/304f86e12b60