Closed Bug 984399 Opened 7 years ago Closed 7 years ago

Contacts list is not properly updated when importing a new FB Friend

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
1.4 S4 (28mar)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jmcf, Assigned: crdlc)

Details

(Keywords: regression)

Attachments

(1 file)

STR:

Have a FB Account with more than 14 friends
Import all of them excepting the one which is on the top (name starts with A or the following)
Go to update FB Friends. Import the first friend (the one not imported before)
Then go to the Contacts List. The imported contact will appear without the photo and without the FB markers and information.
blocking-b2g: --- → 1.4?
QA Wanted to check behavior on 1.3.
Keywords: qawanted
QA Contact: lolimartinezcr
Hi,

   Basically I think that the regression was introduced here

https://github.com/mozilla-b2g/gaia/commit/3b22e091af653fd867c58ee1ce74fc293e262faa

   You can forget FB if you want, it happens adding a new contact with photo as well

STR:

   1) You have x contacts on the list in group 'A'
   2) Create a new one with photo that should be added to this group and it should appear on the viewport

Expected 

   The image should be displayed

Result:

   The image is not displayed before scrolling

I guess that if a contact is updated or created and it will be on the viewport, we should reload the image loader library. What do you think Ben?

Thanks a lot
Flags: needinfo?(bkelly)
Cristian, can you try moving this:

    if (imgLoader) {
      needImgLoaderReload = true;
    }

Earlier in |addToList()|?  Basically, before the calls to |addToGroup()|.  Perhaps the mutation events are synchronous, so |onscreen()| fires before we have set |needImgLoaderReload = true|.
Flags: needinfo?(bkelly) → needinfo?(crdlc)
I can take a look without doubt, thx
Flags: needinfo?(crdlc)
This method [1] is not invoked when a contact is added, at least it is that I am seeing..

[1] var onscreen = function(row) {
(In reply to Cristian Rodriguez (:crdlc) from comment #5)
> This method [1] is not invoked when a contact is added, at least it is that
> I am seeing..
> 
> [1] var onscreen = function(row) {

That's very surprising.  The visibility_monitor should be getting a mutation event indicating the new contact was inserted into the list.

Is this contact inserted via the |addToList()| function?  And the contact is in the viewable range of the list?
    This issue does not reproduce on latest Buri v1.3 with the STRs found in Comment 0.  The updated contact receives a photo and facebook link icon.

v1.3 Environmental Variables:
    Device: Buri v1.3 MOZ
    BuildID: 20140317004001
    Gaia: 0ab8a9cbcef5f23cec904a3d7f7675e44de29951
    Gecko: f824e9d91a2d
    Version: 28.0
    Firmware Version: v1.2-device.cfg
Keywords: qawanted
(In reply to Ben Kelly [:bkelly] from comment #6)
> (In reply to Cristian Rodriguez (:crdlc) from comment #5)
> > This method [1] is not invoked when a contact is added, at least it is that
> > I am seeing..
> > 
> > [1] var onscreen = function(row) {
> 
> That's very surprising.  The visibility_monitor should be getting a mutation
> event indicating the new contact was inserted into the list.
> 
> Is this contact inserted via the |addToList()| function?  And the contact is
> in the viewable range of the list?

I think so...

case 'create':
  contactsList.refresh(event.contactID, checkPendingChanges,....

refresh -> refreshContact -> remove(contact.id); addToList(contact, enriched);
triage: 1.4+ regression
blocking-b2g: 1.4? → 1.4+
Keywords: regression
Jose, could you take this one ?
Flags: needinfo?(jmcf)
Attached file Patch v1
Just to clarify, the issue was when we try to add a new item as first or last element in the scrollable container. In that case, the mutation observer was woken up when the new node was inserted in the DOM but it had defined legacy state.firstChild and state.lastChild variables at that moment and it needs re-compute them in order to check if the new one is the first or last in the list
Attachment #8393409 - Flags: review?(bkelly)
Flags: needinfo?(jmcf)
Assignee: nobody → crdlc
Comment on attachment 8393409 [details]
Patch v1

Since this is a change to tag_visibility_monitor.js in shared/js, I think we need review from David.  Also, lets get feedback from Evan as the tag_visibility_monitor author.

Overall this looks ok to me.  My one thought was, can limit our calls to |recomputeFirstAndLastOnscreen()| to only one per invocation of |mutationHandler()|?

I think the computation can be expensive, trigger sync reflows, etc.  Also, the state of the DOM should be fixed even if we have not looked at all the mutations yet.  So the first and last elements should not change while we are iterating within |mutationHandler()|.
Attachment #8393409 - Flags: review?(dflanagan)
Attachment #8393409 - Flags: review?(bkelly)
Attachment #8393409 - Flags: feedback?(eshapiro)
I agree with you, we could move 

mutation.addedNodes.length && recomputeFirstAndLastOnscreen(); 

to:

https://github.com/crdlc/gaia/blob/bug-984399/shared/js/tag_visibility_monitor.js#L190
Also, really nice job tracking this down!
(In reply to Cristian Rodriguez (:crdlc) from comment #13)
> I agree with you, we could move 
> 
> mutation.addedNodes.length && recomputeFirstAndLastOnscreen(); 
> 
> to:
> 
> https://github.com/crdlc/gaia/blob/bug-984399/shared/js/
> tag_visibility_monitor.js#L190

Hmm, so not check for |child.nodeType === ELEMENT_NODE && child.tagName === tag| before doing the recompute?  I guess that's fair as the addition of other nodes may cause things to shift around on the screen.

I guess I was more just thinking of wrapping your existing call to recompute in an |if (!recomputed) { recomputed=true; recompute(); }|.
Addressed your comment and added the if statement, thx mate
Status: NEW → ASSIGNED
set target milestone to 1.4 S4 for now
Target Milestone: --- → 1.4 S4 (28mar)
Hi David, when could you take a look at this one? Thanks
Flags: needinfo?(dflanagan)
David, if you feel comfortable doing so, you could delegate the review to me.  I think I'm the only other person who has looked at tag_visibility_monitor.js besides you and Evan.

Cristian, looking at it again, I think it might be nice to extend the unit test to catch this problem.  So something that fails without your fix and succeeds with your fix.  If I recall correctly the tests are pretty well factored and it should not be too difficult.  They are under gallery.
Done Ben
Comment on attachment 8393409 [details]
Patch v1

Delegating the review to Ben. I wrote the original visibility monitor, but have never really been familiar with this version of it.
Attachment #8393409 - Flags: review?(dflanagan) → review?(bkelly)
Flags: needinfo?(dflanagan)
Comment on attachment 8393409 [details]
Patch v1

The new test looks good and travis is green.  r=me if you have tested this on device.

Thanks!
Attachment #8393409 - Flags: review?(bkelly) → review+
I tested it but I thought that reviewers do it as well :)
Merged in master:

https://github.com/mozilla-b2g/gaia/commit/42541cd4751fb37bb6c866fc404c53328af55712
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Cristian Rodriguez (:crdlc) from comment #23)
> I tested it but I thought that reviewers do it as well :)

I normally do, but my devices currently all have badly hacked up gecko... so it would take me an hour or so to test then get back to my current state.  Since we have a new unit test specifically for this I was willing to rely on your device testing.  Thanks!
Today's (4/2) master build and working:
Device: Hamachi
BuildId: 20140402071651
Gaia: adde0f6
Gecko: 011a30c
Platform version: 31.0a1

Today's (4/2) 1.4 build and working:
Device: Hamachi
BuildId: 20140402094724
Gaia: b8e6e61
Gecko: 6bfa4cb
Platform version: 30.0a2
Status: RESOLVED → VERIFIED
Attachment #8393409 - Flags: feedback?(eshapiro)
You need to log in before you can comment on or make changes to this bug.