Closed Bug 980982 Opened 6 years ago Closed 6 years ago
Sim Action Button .get Card Index should cache settings to avoid IO thrashing
46 bytes, text/x-github-pull-request
|Details | Review|
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.
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.
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+
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
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.
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 ago → 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 987024
You need to log in before you can comment on or make changes to this bug.