Closed Bug 980982 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

(blocking-b2g:1.4+)

RESOLVED DUPLICATE of bug 987024
blocking-b2g 1.4+

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(1 file)

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.
Depends on: 946866
Blocks: b2g-dsds-1.4
Summary: SimSettingsHelper should cache settings to avoid IO thrashing → MultiSimActionButton.getCardIndex should cache settings to avoid IO thrashing
Also, we could just close this and fold the patch into bug 985888, which might make more sense.
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.
Blocks: 985888
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.
Attachment #8394132 - Flags: review?(anthony) → review+
https://github.com/mozilla-b2g/gaia/commit/b2545bf6eb976704b54d82cfc06b24f31fedee6a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
This must be uplifted for its 1.4+ dependency, bug 985888.
blocking-b2g: --- → 1.4?
Summary: MultiSimActionButton.getCardIndex should cache settings to avoid IO thrashing → [DSDS] MultiSimActionButton.getCardIndex should cache settings to avoid IO thrashing
Depends on: 987024
This patch broke the ability to dial a phone call from the contacts app. As such, this needs to be backed out.
(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?
(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.
If this gets 1.4+, we should also mark bug 987024 as 1.4+ since it's a followup fix.
Backed out in https://github.com/mozilla-b2g/gaia/commit/5ff9476ecd3acd70a320a368b5eb3f9f844f14bd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks a blocker
blocking-b2g: 1.4? → 1.4+
We landed this patch and the fix for it in bug 987024.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 987024
No longer depends on: 987024
Depends on: 1008974
You need to log in before you can comment on or make changes to this bug.