[Contacts] Refactor the FB sync schedule and remove it from the app critical path

VERIFIED FIXED in Firefox OS v2.1

Status

Firefox OS
Gaia::Contacts
--
major
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: arcturus, Assigned: Adrián de la Rosa)

Tracking

unspecified
2.1 S4 (12sep)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.1 verified, b2g-v2.2 verified)

Details

Attachments

(1 attachment)

481 bytes, text/html
crdlc
: review+
arcturus
: review+
crdlc
: feedback+
Details
+++ This bug was initially created as a clone of Bug #1050684 +++

We should move away from the critical path the sync scheduling.

As this is working now, we can do this in a task during the stabilisation phase.
No longer depends on: 1052352
No longer depends on: 1050684
(Assignee)

Comment 1

3 years ago
Created attachment 8478144 [details]
23263.html

I propose this as a way to execute every method that needs to be executed after the list is rendered. I've also moved facebook sync scheduling away from the critical path, but I would like to receive feedback about how it would fit in the long term. For example, bug 1053477 have to be build upon this one.
Attachment #8478144 - Flags: feedback?(francisco)
Attachment #8478144 - Flags: feedback?(crdlc)
Comment on attachment 8478144 [details]
23263.html

I see this very clear
Attachment #8478144 - Flags: feedback?(crdlc) → feedback+
(Assignee)

Updated

3 years ago
Attachment #8478144 - Flags: review?(francisco)
Attachment #8478144 - Flags: review?(crdlc)
Attachment #8478144 - Flags: feedback?(francisco)
Assignee: nobody → hola
Status: NEW → ASSIGNED
Comment on attachment 8478144 [details]
23263.html

The code looks good although I would like to perform that logic in Contacts module instead of List library. I guess that the list shouldn't know anything related to Facebook synchronization stuffs (it should be an independent module) because IMHO Contacts is the brain of the app and List is just a renderer for an specific view. In the Contact's initialization after https://github.com/mozilla-b2g/gaia/pull/23263/files#diff-802d515575a6a6e55da9ef74c9ece9b1R239 we could do something like this:

window.addEventListener('listRendered', function postRendering() {
  window.removeEventListener('listRendered', postRendering);
  checkFacebookSynchronization(config);
});

But please, double check with Francisco who knows more than me about Contact's architecture.

BTW my review is positive although my comment would be more than welcome if you want to take into consideration

Thanks for your excellent work Adrian as usual!
Attachment #8478144 - Flags: review?(crdlc) → review+
Comment on attachment 8478144 [details]
23263.html

LGTM, suggestiong in naming by Cristian also looking interesting.

Thanks for the patch!
Attachment #8478144 - Flags: review?(francisco) → review+
Sorry,

just sync with Cristian, and explained me his pov, and he is totally right.

The list already sends the event, but we don't have to execute the FB operations in the list itself. IMHO, we should extract the functionality to an external file, and listen to the list change in the list, where we will be loading this external file and executing the operation.
(Assignee)

Comment 6

3 years ago
Comment on attachment 8478144 [details]
23263.html

I updated the pull request with a solution that uses a new module which waits until it receives the `listRendered` event to load. I would like to know if this is similar to what you suggested before I continue with the unit tests.

Thanks.
Attachment #8478144 - Flags: feedback?(francisco)
(Assignee)

Updated

3 years ago
Attachment #8478144 - Flags: review?(francisco)
Attachment #8478144 - Flags: review+
Attachment #8478144 - Flags: feedback?(francisco)
(Assignee)

Comment 7

3 years ago
Submitting patch for review. The patch is working on the device. I created a new module that listens to the listRendered event to trigger actions that can be delayed, as suggested. At this moment, only the scheduling of a FB sync when contacts have been synced from FTU is using this new module. 

I also made a new test suite, and the test is passing.
Comment on attachment 8478144 [details]
23263.html

Left some tiny comments on the PR.

But granting the r+ since they don't affect the code.
Attachment #8478144 - Flags: review?(francisco) → review+
Oh ... just got an idea.

What if this instead of being loaded always and listen by himself to the listRendered event. Is lazyload when that event is triggered.

Let's say, contacts.js listens to the listRendered event and lazyloads this file and executes .init.

With that way we also don't have to load and parse this javascript until is really needed.

How do you see this Adrian?
Flags: needinfo?(hola)
(Assignee)

Comment 10

3 years ago
Great idea! I implemented it in the updated version of my pr. Take a look at it and tell me if that is what you meant.
Flags: needinfo?(hola) → needinfo?(francisco)
Perfect work, you read my mind!

Unfortunately we cannot merge it, could you rebase and we will be ready to land
Flags: needinfo?(francisco) → needinfo?(hola)
(Assignee)

Comment 12

3 years ago
Rebased and ready to merge! I'll wait for tbpl to finish and then I'll ping someone to merge it. Thanks!
Flags: needinfo?(hola)
\o/

Landed

https://github.com/mozilla-b2g/gaia/commit/8aa4c496438f2776a527c160a3ed6f5b2d4a9ea8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
status-b2g-v2.1: --- → fixed
Target Milestone: --- → 2.1 S4 (12sep)

Updated

3 years ago
Blocks: 1067303

Updated

3 years ago
Blocks: 1060101
This didn't land before v2.1 branched. Please request Gaia v2.1 approval on this when you get a chance.
status-b2g-v2.1: fixed → affected
status-b2g-v2.2: --- → fixed
Flags: needinfo?(hola)

Comment 15

3 years ago
Comment on attachment 8478144 [details]
23263.html

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

This bug blocks a blocker so we need approval
Attachment #8478144 - Flags: approval-gaia-v2.1?
(Assignee)

Comment 16

3 years ago
Thanks for requesting v2.1 approval, Jose Manuel!
Flags: needinfo?(hola)
Attachment #8478144 - Flags: approval-gaia-v2.1?
Please don't do empty approval requests. But it's very unlikely that we uplift this patch at this point.

Comment 18

3 years ago
(In reply to Fabrice Desré [:fabrice] from comment #17)
> Please don't do empty approval requests. But it's very unlikely that we
> uplift this patch at this point.

Fabrice,

This bug blocks a blocker, so we would need it to be uplifted

thanks

Comment 19

3 years ago
Comment on attachment 8478144 [details]
23263.html

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Removing IAC between Contacts and FTU to improve performance
[User impact] if declined: Performance of the Contacts App can suffer
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low risk
[String changes made]:
Attachment #8478144 - Flags: approval-gaia-v2.1?

Updated

3 years ago
Attachment #8478144 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1?(fabrice)
(In reply to Jose Manuel Cantera from comment #19)
> Comment on attachment 8478144 [details]
> 23263.html
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): Removing IAC between Contacts
> and FTU to improve performance

Can you point me to the IAC changes in the PR? I could not see anything IAC related.
Flags: needinfo?(jmcf)

Comment 21

3 years ago
(In reply to Fabrice Desré [:fabrice] from comment #20)
> (In reply to Jose Manuel Cantera from comment #19)
> > Comment on attachment 8478144 [details]
> > 23263.html
> > 
> > [Approval Request Comment]
> > [Bug caused by] (feature/regressing bug #): Removing IAC between Contacts
> > and FTU to improve performance
> 
> Can you point me to the IAC changes in the PR? I could not see anything IAC
> related.

Fabrice, 

The root of these changes comes from the IAC migration to datastore done in bug 1045521. There were a series of follow-ups including one to set up the next FB synchronization cycle once an importation has been made from the FTU. Afterwards we decided to move this code out of the critical app path, and this is the bug that addressed the problem.

I hope this clarifies

thanks
Flags: needinfo?(jmcf)
Attachment #8478144 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
v2.1: https://github.com/mozilla-b2g/gaia/commit/891ff91e02106c483f320612b69166165139732c
status-b2g-v2.1: affected → fixed

Comment 23

3 years ago
Hi Francisco,
This isssue is to say FB contacts only support once in FTU after refactor the FB sync schedule?
Flags: needinfo?(francisco)
Right, we refactor the sync after the separation, and with this patch we removed the sync from the critical path, so now it's executed after we render the contacts list.
Flags: needinfo?(francisco)

Comment 25

3 years ago
This issue has been verified successfully on Flame2.1&2.2,
Please see Flame2.1 detial information in Bug 1050684 Comment 12

Flame 2.2 build: 
Gaia-Rev        e17c5656dbf517d48fb61ac9bc92119e023fd717
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/be1f49e80d2d
Build-ID        20141210040201
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141210.074809
FW-Date         Wed Dec 10 07:48:20 EST 2014
Bootloader      L1TC00011880
Status: RESOLVED → VERIFIED
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
You need to log in before you can comment on or make changes to this bug.