980982 - [DSDS] MultiSimActionButton.getCardIndex should cache settings to avoid IO thrashing

Status: RESOLVED DUPLICATE of bug 987024

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

Description

[Doug Sherk (:drs) (inactive)]

Currently, we're doing an IO operation every time we get the card for a service. We should cache these values so that successive calls are quick.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17384

This is a pretty wonky patch because it's designed for future use. If you want to see how it's going to be used, check out the patch applied on top of it for bug 985888 here: https://github.com/DouglasSherk/gaia/commits/978814-active-call-snapshot

You'll probably be wondering why we're doing the following things:

1) No additional tests? The current tests cover this functionality perfectly already, since the caching is an implementation detail. The only test I could possibly think of adding is checking that we're never calling anything on mozSettings, but that seems overly pedantic to me.
2) Why bother using a callback in _getCardIndex(), or even have that function at all? In bug 985888, we're going to be adding additional checks here which require the use of a callback due to async loading (we don't need SimPicker now, but we will).
3) Why have a _settingsObserver() function when we can just add a |this._defaultCardIndex = cardIndex;| line to _updateUI? Because for bug 985888, we're again going to have to separate this functionality, since _updateUI() will be called from a function other than _settingsObserver(), where we won't want the default card index to be changed.

Comment 2

[Doug Sherk (:drs) (inactive)]

Also, we could just close this and fold the patch into bug 985888, which might make more sense.

Comment 3

[Doug Sherk (:drs) (inactive)]

Updated PR.

This is a pretty wonky patch because it's designed for future use. If you want to see how it's going to be used, check out the patch applied on top of it for bug 985888 here: https://github.com/DouglasSherk/gaia/commits/978814-active-call-snapshot

You'll probably be wondering why we're doing the following things:

1) No additional tests? The current tests cover this functionality perfectly already, since the caching is an implementation detail. The only test I could possibly think of adding is checking that we're never calling anything on mozSettings, but that seems overly pedantic to me.
2) Why have a _settingsObserver() function when we can just add a |this._defaultCardIndex = cardIndex;| line to _updateUI? Because for bug 985888, we're again going to have to separate this functionality, since _updateUI() will be called from a function other than _settingsObserver(), where we won't want the default card index to be changed.

Comment 4

[Anthony Ricaud (:rik)]

Comment on attachment 8394132 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17384

Let's be pedantic and check that we never call mozSettings. It will be easy though ;) Remove mock_navigator_moz_settings in the test.

There's also two indentation comments on Github.

Comment 5

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/b2545bf6eb976704b54d82cfc06b24f31fedee6a

Comment 6

[Doug Sherk (:drs) (inactive)]

This must be uplifted for its 1.4+ dependency, bug 985888.

Comment 7

[Jason Smith [:jsmith]]

This patch broke the ability to dial a phone call from the contacts app. As such, this needs to be backed out.

Comment 8

[Doug Sherk (:drs) (inactive)]

(In reply to Jason Smith [:jsmith] from comment #7)
> This patch broke the ability to dial a phone call from the contacts app. As
> such, this needs to be backed out.

I'm unable to repro this. Can you give STR?

Comment 9

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) from comment #8)
> I'm unable to repro this. Can you give STR?

Nevermind, I'm reading bug 987024.

Comment 10

[Doug Sherk (:drs) (inactive)]

If this gets 1.4+, we should also mark bug 987024 as 1.4+ since it's a followup fix.

Comment 11

[Doug Sherk (:drs) (inactive)]

Backed out in https://github.com/mozilla-b2g/gaia/commit/5ff9476ecd3acd70a320a368b5eb3f9f844f14bd

Comment 12

[David Scravaglieri [:scravag]]

Blocks a blocker

Comment 13

[Doug Sherk (:drs) (inactive)]

We landed this patch and the fix for it in bug 987024.