Closed Bug 1120947 Opened 5 years ago Closed 5 years ago

mock_navigator_settings behaves radically different when used as a Promise

Categories

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

x86_64
Linux
defect
Not set

Tracking

(b2g-master fixed)

RESOLVED FIXED
2.2 S4 (23jan)
Tracking Status
b2g-master --- fixed

People

(Reporter: freddyb, Assigned: freddyb)

References

Details

Attachments

(1 file, 1 obsolete file)

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 :/
Blocks: 1100945
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
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)?
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.
Assignee: nobody → fbraun
Attachment #8548901 - Flags: review?(felash)
Let's see which of them sticks ;)
Attachment #8548902 - Flags: review?(felash)
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 !
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.
Attachment #8548902 - Attachment is obsolete: true
Attachment #8548902 - Flags: review?(felash)
This is fixed in PR https://github.com/mozilla-b2g/gaia/pull/27384 now.
Please review :)
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 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.
Attachment #8548901 - Flags: review?(felash) → review+
green try (except for some intermittent failures): https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=fb85efa22e5e
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/2d8b2d7245227143e212f646f093c21b9388215c
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S4 (23jan)
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?
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
You need to log in before you can comment on or make changes to this bug.