852516 - Timing issues in Contacts app

Status: RESOLVED FIXED

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

Description

[Jan Jongboom [:janjongboom] (inactive)]

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`).

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 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.

Comment 2

[Kevin Grandon :kgrandon]

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

[Jose Manuel Cantera]

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

Comment 4

[Francisco Jordano [:arcturus] [: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

Comment 5

[alberto.pastor]

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 6

[Jan Jongboom [:janjongboom] (inactive)]

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.

Comment 7

[alberto.pastor]

Hey, could you please rebase the patch?

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

Has been rebased. Feel free to pull :)

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 726658 [details] [diff] [review]
Patch for this issue

Looking good to me, great job!

Comment 10

[alberto.pastor]

Unit Tests passing manually. Performance test not showing any loose. Code looking good: r+!

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

Good. Can you land this as well? I don't have merge rights.

Comment 12

[alberto.pastor]

https://github.com/mozilla-b2g/gaia/commit/87e6e5bdf371d5a37c66dc0921366540fb22fac2