1120947 - mock_navigator_settings behaves radically different when used as a Promise

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Shared, defect)

Description

[Frederik Braun [:freddy]]

The mock settings object doesn't have as the normal settings object, when treated as a Promise:

Example code:
> var lock = navigator.mozSettings.createLock();
> var storedParams = Promise.all([
>         lock.get('key1'),
>         lock.get('key2'),
> ]);
> storedParams.then(values) => {
>         console.log(values);
> });

With the normal settings object you get an array:
> [ {'key1': 1}, {'key2: 2} ]
With the mock object you get an array as follows:
> [ {'result': {'key1': 1} }, {'result': {'key2: 2} } ]



I randomly CCd the last people who touched the mock object, as I couldnt find any meaningful instance of the string "result" in mock_navigator_moz_settings.js :/

Comment 1

[Julien Wajsberg [:julienw]]

I found [1] :)

But the real truth is that this file does not support "DOMRequest as thenable" yet. So you simply get a "DOMRequest-like" as result, because this is what createLock().get() is returning. See [2].

So the solution is to make this settingsRequest a thenable. Which is easier said than done, clearly, but is the only solution.

The easiest, I think, would be a function like this:

  then: function(cb) {
    this.addEventListener('success', cb);
  }

Note that addEventListener itself is quite incorrect because we can have only one handler, but if we eventually fix addEventListener we'll also fix then in the same time ;)

[1] https://github.com/mozilla-b2g/gaia/blob/ce6c9b72d863c80c842c65acf3b040baf4901a97/shared/test/unit/mocks/mock_navigator_moz_settings.js#L114

[2] https://github.com/mozilla-b2g/gaia/blob/ce6c9b72d863c80c842c65acf3b040baf4901a97/shared/test/unit/mocks/mock_navigator_moz_settings.js#L113-L118

Comment 2

[Frederik Braun [:freddy]]

I can't really rewrite all of my code just for the test. :|
Ah, do you think the fix you propose would be viable, if I had to somehow guarantee that there's just one .then call (i.e. just one event listener required)?

Comment 3

[Julien Wajsberg [:julienw]]

I think it would; although it would be easy to support an array of listeners in addEventListener too, if you need it and want to look at this.

Unfotunately I'm very busy these days, otherwise I could do a patch for you.

Comment 4

[Frederik Braun [:freddy]]

Attachment: pull request that also supports multiple event listeners

Comment 5

[Frederik Braun [:freddy]]

Attachment: less invasive pull request (just then-able)

Let's see which of them sticks ;)

Comment 6

[Julien Wajsberg [:julienw]]

Both seems to stick (I think the Gij issues are in master too, but we'd need to check), but I'd like that we use addEventListener because we'd get the functionality around "mSyncRepliesOnly" for free.

Tell me what you think !

Comment 7

[Frederik Braun [:freddy]]

For the then-able use case, I figured the developer wouldn't want to manually control when it fires – Since that's something a Promise deliberately hides away from you.

I can make sure that all follow-up calls will support the correct API by returning a Promise. The thing that's returned can be properly chained with additional .then and .catch calls. So I *need* to return a Promise.

But I could *resolve* the Promise using addEventListener. I'll try and take a look at it.

Comment 8

[Frederik Braun [:freddy]]

This is fixed in PR https://github.com/mozilla-b2g/gaia/pull/27384 now.
Please review :)

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8548901 [details] [review]
pull request that also supports multiple event listeners

added some nits; gonna wait for the result of the additional Gu runs that I requested.

Maybe you'd want to rebase on master too, to see if the Gij failures are resolved.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8548901 [details] [review]
pull request that also supports multiple event listeners

r=me with nits addressed.

I see no instance of this mock in marionette tests so I'm quite sure the failures are not because of this patch. Still let's rebase to see if this looks better, I think Kevin has backed out something and disabled some flaking tests.

Comment 11

[Frederik Braun [:freddy]]

green try (except for some intermittent failures): https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=fb85efa22e5e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/2d8b2d7245227143e212f646f093c21b9388215c

Comment 13

[Frederik Braun [:freddy]]

Wait, there is
/shared/test/unit/mocks/mock_navigator_moz_settings.js
as well as 
/apps/settings/test/unit/mock_navigator_settings.js

Do we have even more copies? Maybe this means reopening and fixing the other one too?
Any advise, Julien?

Comment 14

[Julien Wajsberg [:julienw]]

There is one in operatorvariant too.

I'd say these apps should just migrate to use the mock in /shared when they'll want to use a then-able with mozSettings.