If I mock the mozContacts.find() response to return on nextTick timing issues occur: 1. The `noContacts` layer in `toggleNoContactsScreen` is undefined, thus causing an unhandled exception 2. `utils.text` is undefined because the async scripts might not have completed loading (loading contacts start onDOMContentLoaded rather than asyncScriptsLoaded), thus having an unhandled exception when formatting the contact (in `getHighlightedName`).
Created attachment 726658 [details] [diff] [review] Patch for this issue Before the contacts list gets loaded the ContactsList first needs to be initialized. Thereafter we need to have loaded the async scripts before starting to render the actual contacts because we rely on certain util scripts to be there (escapeHTML f.e.). This PR addresses that. On the expected reply: "but it works on my device". Sure it does, but there are timing issues, and they will probably show up at one edge case or another, I have already run into them so probably someone else will too.
This is also needed when testing in the browser using the real mozContacts API which is now possible with the runtime plugin. +1 from me.
Comment on attachment 726658 [details] [diff] [review] Patch for this issue changing reviewer to Alberto as he is the author of that code. And asking feedback from Francisco
I would like a different approach in the solution. Instead of waiting for another event on the buildContacts methods, we should ensure that method is not called until we are able to build the contacts list. Also it breaks the unit test, which for me is stopper to merge it: https://travis-ci.org/mozilla-b2g/gaia/builds/5632429 Thanks
Attachment #726658 - Flags: feedback?(francisco.jordano) → feedback-
After profiling Contacts app, we realize we are not saving any time trying to request contacts onDOMContentLoaded. I would suggest just move the getFirstContacts method after the contactsList initialization.
Comment on attachment 726658 [details] [diff] [review] Patch for this issue Francisco, Alberto, I have adjusted the PR. Looks a lot more maintainable this way. The unit tests now also run as expected again.
Attachment #726658 - Flags: feedback- → feedback?(francisco.jordano)
Hey, could you please rebase the patch?
Has been rebased. Feel free to pull :)
Comment on attachment 726658 [details] [diff] [review] Patch for this issue Looking good to me, great job!
Attachment #726658 - Flags: feedback?(francisco.jordano) → feedback+
Unit Tests passing manually. Performance test not showing any loose. Code looking good: r+!
Good. Can you land this as well? I don't have merge rights.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.