Timing issues in Contacts app

RESOLVED FIXED

Status

Firefox OS
Gaia::Contacts
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: janjongboom, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Attachment #726658 - Flags: review?(etienne)
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 3

5 years ago
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
Attachment #726658 - Flags: review?(etienne)
Attachment #726658 - Flags: review?(alberto.pastor)
Attachment #726658 - Flags: feedback?(francisco.jordano)
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-

Comment 5

5 years ago
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)

Comment 7

5 years ago
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+

Updated

5 years ago
Attachment #726658 - Flags: review?(alberto.pastor) → review+

Comment 10

5 years ago
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.

Comment 12

5 years ago
https://github.com/mozilla-b2g/gaia/commit/87e6e5bdf371d5a37c66dc0921366540fb22fac2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 862338
You need to log in before you can comment on or make changes to this bug.