879299 - Integrate visibility_monitor into contacts app

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently the contacts app builds its list DOM incrementally as the ContactsAPI provides results.  Since the full DOM is being constructed immediately for each list item, the items are added slowly using a timer to avoid jank.  As discussed in bug 865747, an unfortunate side effect is it takes at least 12 seconds to fully load the list and allow jump scrolling to later letter group.

The visibility_monitor would allow contacts app to quickly populate a placeholder DOM for each item in the list and then only render the full item when it appears on the screen.  Initial testing suggests this improves the time to load the full list by around 30% to 40%.

It is possible this will also offer some improvement for initial app load as measured in bug 871823.

In order to do this correctly, however, we need vibility_monitor to support the concept of groups which is being added in bug 865750.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Alberto, I'm assigning this to myself for now to signal that we are working on it together.  Please feel free to take it from me, though, if you prefer to have it in your list.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Proof of concept for integrating the multilevel_visibility_monitor can be found here:

  https://github.com/wanderview/gaia/tree/contacts-vis-mon

Note, there are still some issues:

 - The jump scrolling needs to be hooked back up
 - Some severe jank

We need to investigate the jank more.  My gut feeling at the moment is that the visibility monitor is getting into a bad state.  I will post more about that on bug 865750.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Header refresh also needs to be fixed with this proof of concept.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Proof of concept branch updated to fix the severe jank issue.  It seems it was related to re-triggering the mutation observer when we rendered the contact.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Snapshot of proof-of-concept with reduced jank

I was able to smooth out a lot of the minor scroll jank by accumulating elements in an array and then rendering them in a batch on the nextTick.   Seems ok on my Buri.

This change is on my github branch referenced above.  Also, attaching a snapshot of the total patch here.

Things still to do:
  - Fix jump scrolling
  - Fix header refreshing
  - Update list when new contacts are added/removed

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Snapshot with major issues fixed

Here's an updated snapshot of the github branch.  Scrolling appears to be relatively jank free to me on my Buri.  The fixed headers, jump scrolling, and adding/removing contacts works.

I have not tested with facebook, images, etc.

If this looks good I can consolidate my branch into a clean pull request after bug 865750 lands.

Comment 7

[alberto.pastor]

Start looking good! The only thing remaining we should take care off is searching. We use data-search in the DOM for each element. If we don't render this dataset in the container (I think right now is in a children element), we won't be able to search until all the elements has been fully painted. I would suggest moving that dataset to the "li" element and changing the selectors in search.js
Thanks!

Comment 8

[Cristian Rodriguez (:crdlc)]

Hi Ben,

   I would like to help you because I know contacts app and Facebook integration so when you will have the patch and this one will be ready, I would like to review your patch in order to test some cases on Facebook as auto-synchronization every day, etc... Then we can test better all cases. Right? Well, I will be other reviewer, Alberto should be the main reviewer for example.

Thanks a lot

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Cristian Rodriguez de la Cruz (:crdlc) from comment #8)
>    I would like to help you because I know contacts app and Facebook
> integration so when you will have the patch and this one will be ready, I
> would like to review your patch in order to test some cases on Facebook as
> auto-synchronization every day, etc... Then we can test better all cases.
> Right? Well, I will be other reviewer, Alberto should be the main reviewer
> for example.

Sounds good Cristian.  I hope to have something for review tomorrow or Wednesday.  Unfortunately I got sidetracked on some another high priority bug which needs to be finished today.  Thanks!

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I have search mostly working and hope to have another snapshot posted tomorrow.  I realized, though, that I should probably also modify image loading.  I don't think the current code will work since it wants to add an image to all nodes in the list, but some nodes won't have been rendered fully yet.

I was thinking I could try adding renderPhoto() to the normal renderContact() function.  Since we are only calling this when the contact is displayed, it has the same effect of deferring the photo load.

My code also pre-renders the first 6 items in each letter group, so I guess I could still defer the photo render in those cases.

Do we have an easy way to test photo rendering?  Talking to jhylands, I don't think we have reference data for contact photos.

Thoughts?

Comment 11

[alberto.pastor]

Hi!
I'm ok adding renderPhoto to renderContact, but would be nice having a parameter in order to chose when rendering the photo. I think still deferring the photos from the 6 pre-rendered contacts of each letter is a good idea. Every second counts! :)

Regarding photos, probably the easiest way is to sync with facebook/google. 

Thanks!

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Updated WIP patch with working search, import, and unit tests.

Another snapshot of the visibility monitor integration.  I think its getting close, so I wanted to post for additional feedback.

Note that in addition to making search work with the new VM code, this patch also supports searching while contacts are still loading.  I also tested that importing works via google contacts.  The communications app unit tests pass cleanly.

Still TODO:

  - Examine image loading.  While testing imports, however, images loaded correctly so this is currently working.  I just want to make sure it makes sense given then new architecture.
  - Produce a clean pull request.  Right now I have lots of WIP commits on my github branch, etc.
  - Wait for the multilevel_visibility_monitor to get reviewed and committed.

Comment 13

[Cristian Rodriguez (:crdlc)]

Comment on attachment 763937 [details] [diff] [review]
Updated WIP patch with working search, import, and unit tests.

1) I wouldn't like to see this constant in the code /NUM_VISIBLE_CONTACTS = 6/ It is very dependent on screen size IMHO

2) We have to ensure that the image loader continues working correctly, it has just to load images on the viewport. Moreover there is a resolver module (fb_resolver.js) based on the same idea, update the info for FB contacts from IndexedDB when they are on the viewport

3) It is very important to check the automatic updates of FB contacts every day

4) I've seen new methods on import-ui library called getNodes, getFirstNode, bla bla. I would like to read more info about it from you in order to know what it is please. Are you cloning nodes during the import process assuming items are equal between import and list? If you clone a node with a revoked URL maybe you receive a fail revalidating this one during appending the node in the DOM.

The rest is ok for me, great work!

Comment 14

[Cristian Rodriguez (:crdlc)]

The last important thing is related to import UI, I guess that it is not a good idea to mix modules, IMHO, contacts list shouldn't know import_ui. Although Alberto and/or Francisco can have other opinion, I don't know, but for this reason my feedback could be -

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Thanks for the feedback!

(In reply to Cristian Rodriguez de la Cruz (:crdlc) from comment #13)
> 1) I wouldn't like to see this constant in the code /NUM_VISIBLE_CONTACTS =
> 6/ It is very dependent on screen size IMHO

Agree.  I'll look for a way to calculate this.  Note, there is an existing, similar constant in search.js called SEARCH_PAGE_SIZE.

> 2) We have to ensure that the image loader continues working correctly, it
> has just to load images on the viewport. Moreover there is a resolver module
> (fb_resolver.js) based on the same idea, update the info for FB contacts
> from IndexedDB when they are on the viewport
>
> 3) It is very important to check the automatic updates of FB contacts every
> day

This is on my TODO list for today.

> 4) I've seen new methods on import-ui library called getNodes, getFirstNode,
> bla bla. I would like to read more info about it from you in order to know
> what it is please. Are you cloning nodes during the import process assuming
> items are equal between import and list? If you clone a node with a revoked
> URL maybe you receive a fail revalidating this one during appending the node
> in the DOM.

Sure.

While doing my integration I found I needed to modify search.js to accommodate the changes in contacts_list.js.  For example, when cloning a contact node we might need to call renderContact() first since we were no deferring this rendering until visible.

While changing search.js, however, I realized that I might have broken importer_ui.js inadvertently because search.js assumes knowledge of the DOM in use.

To help me reason about things I refactored search.js to use an abstraction instead of operating directly on the DOM.  When Search.init() is called you now pass in a "source" object which provides the following functions:

  // Expects a contact source object providing the following functions:
  //  getNodes():           an Array of all contact DOM nodes
  //  getFirstNode():       first contact DOM node
  //  getNextNode(node):    given a node, find the next node
  //  clone(node):          clone the given contact node
  //  getNodeForId(id):     get the node matching the given ID, or null
  //  getSearchText(node):  get the search text from the given node
  //  click(event):         click event handler to use

Most of these functions previously existed, but were directly implemented in search.js.  This created a tight coupling between search.js, contact_list.js, and importer_ui.js.

Having contact_list.js and importer_ui.js provide these functions to search.js via the source handle helps remove this tight coupling.

So if you look in the changes for importer_ui.js you should see that these functions are defined within a "searchSource" object that is then passed to Search.init().

When I clean up the pull request I can break this refactoring out into a separate commit if you prefer.

> The last important thing is related to import UI, I guess that it is not a
> good idea to mix modules, IMHO, contacts list shouldn't know import_ui.
> Although Alberto and/or Francisco can have other opinion, I don't know, but
> for this reason my feedback could be -

I agree with you here.  Reducing coupling between these modules was the motivation in (4) above.

Or are you saying there is a place where contacts_list.js directly references importer_ui.js that I missed?  Happy to look at that as well.

Comment 16

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

Comment on attachment 763937 [details] [diff] [review]
Updated WIP patch with working search, import, and unit tests.

Review of attachment 763937 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Ben,

Couldn't try this agains master, the patch is not applying, but took a look to the code and is looking pretty good to me.

Just left some comments, more of them are minor stuff, could you please rebase so I can try on the device?

::: apps/communications/contacts/js/contacts_list.js
@@ +58,5 @@
> +      monitor.resumeMonitoringMutations();
> +    }, 0);
> +  };
> +
> +  var offscreen = function(el) {

Really like the idea of calling the renderPhoto when needed, but we should use this PR to free some resources, as you comment, the photos.

One problem we have is contacts get easily killed and we need to do often cold starts.

Will be happy to help here :)

@@ +98,2 @@
>  
> +    monitor = monitorChildWithTagVisibility(scrollable, 600, 300, 4, 'li',

We will need to precalculate those parameters based on screen resolutions.

Or we can leave this to the |monitorChildWithTagVisibility| function, wdyt?

@@ +401,4 @@
>  
>    var renderedChunks = 0;
>    var CHUNK_SIZE = 20;
> +  var NUM_VISIBLE_CONTACTS = 6;

I guess this number fits the case of the Peak, but again we can try to calculate it based on device specs.

Not really worry about it now.

::: apps/communications/contacts/style/contacts.css
@@ +566,5 @@
>    height: 5rem;
>  }
> +
> +#groups-container-box {
> +  max-height: 410px;

;)

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Francisco Jordano [:arcturus] from comment #16)
> Comment on attachment 763937 [details] [diff] [review]
> Updated WIP patch with working search, import, and unit tests.
> 
> Review of attachment 763937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Ben,
> 
> Couldn't try this agains master, the patch is not applying, but took a look
> to the code and is looking pretty good to me.
> 
> Just left some comments, more of them are minor stuff, could you please
> rebase so I can try on the device?

Sure.  I rebased my github branch:

  https://github.com/wanderview/gaia/tree/contacts-vis-mon

Note, I do not recommend updating to latest mozilla-central, though, due to bug 881970.

I'll respond to the other items after I've had a chance to investigate more.  Thanks for the feedback!

Comment 18

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

(In reply to Ben Kelly [:bkelly] from comment #17)
> (In reply to Francisco Jordano [:arcturus] from comment #16)
> > Comment on attachment 763937 [details] [diff] [review]
> > Updated WIP patch with working search, import, and unit tests.
> > 
> > Review of attachment 763937 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hi Ben,
> > 
> > Couldn't try this agains master, the patch is not applying, but took a look
> > to the code and is looking pretty good to me.
> > 
> > Just left some comments, more of them are minor stuff, could you please
> > rebase so I can try on the device?
> 
> Sure.  I rebased my github branch:
> 
>   https://github.com/wanderview/gaia/tree/contacts-vis-mon
> 
> Note, I do not recommend updating to latest mozilla-central, though, due to
> bug 881970.
> 
> I'll respond to the other items after I've had a chance to investigate more.
> Thanks for the feedback!

Sorry, when talking about master, I meant gaia-master.

What's the better combination for trying this?

Thanks for the quick answer!

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Francisco Jordano [:arcturus] from comment #18)
> Sorry, when talking about master, I meant gaia-master.
> 
> What's the better combination for trying this?

Yea, just wanted to make sure in case you were doing a full update on the phone.

Any m-c revision earlier or at 134166:12cdc8931e48 should work.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Pushed changes to calculate values based on viewport size instead of using constants:

  https://github.com/wanderview/gaia/commit/af343bb523a51eae888b4dbb53abf5d82479dff2

Note, the tests pass, but from debugging I know that the sizes are not accurate during the automated test.  I think this is due to the way the DOM is built up in resetDom() without any CSS, etc.  For example, the overall contacts list view height shows up as 16.5 pixels.

Is it worth trying to fix this issue?  Is there a straightforward way to use the real DOM and CSS?  I'm still trying to come up to speed on our test infrastructure, so the answers are not immediately obvious to me.  Thanks!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10582

Pointer to Github pull-request

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 766812 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10582

Here is the cleaned up pull request for integrating visibility_monitor into contacts app.  I apologize for the size of the changes, but it ended up touching a lot after fixing it to work with search, image loading, and the importer.

Listing Francisco as the reviewer since Alberto has been a bit more scarce recently.

Finally, keep in mind we will need bug 865750 to land before committing this.  Its under review now.

Thanks!

Comment 23

[Cristian Rodriguez (:crdlc)]

Can I test it before landing bug 865750? thanks

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Standalone multilevel_visibility_monitor patch.

(In reply to Cristian Rodriguez de la Cruz (:crdlc) from comment #23)
> Can I test it before landing bug 865750? thanks

Here is a standalone patch that you can apply on top of the pull request to test.  It includes the proposed multilevel_visibility_monitor plus some null reference fixes Evan is evaluating as part of review.

Thanks!

Comment 25

[Cristian Rodriguez (:crdlc)]

Great, I will test it tomorrow! Thanks a lot for the patch

Comment 26

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

Hi Ben,

will start the review this afternoon, thanks a lot for the hard work :)

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Francisco Jordano [:arcturus] from comment #26)
> will start the review this afternoon, thanks a lot for the hard work :)

Thanks Francisco!  Note, I made a comment in the PR for an issue I found today.  I thought I would just comment instead of fixing to avoid churning the repo during review.

Comment 28

[Cristian Rodriguez (:crdlc)]

I've found one minor problem:

* Empty list > Added "García Marquez" > Go to search, type "ff" -> I don't see "No contacts found"

Maybe it is not working on master, I don't know, could you take a look?

Thanks

Comment 29

[Cristian Rodriguez (:crdlc)]

I remember a lot of conversations about removing images from DOM when they are not on the viewport with my colleagues here in Telefonica when I implemented the image loader lib. Right now, I can read that you decide remove them :) It is ok for me, but I guess that you forget something

function imageReleased(item) {
  var image = item.querySelector('img[data-src]');
  if (!image) {
    return;
  }
  
  image.src = null;
  item.dataset.visited = 'false';
}

1) visited attribute to false makes sense for me but I guess that we should remove the contact image
2) imageReleased -> releaseImage ??

What do you think?

Comment 30

[Cristian Rodriguez (:crdlc)]

Other minor bug:

* Empty list
* Added a new contacts named "García Marquez" or whatever :)
* Go to list and "Garcia Marquez" is there
* Edit this contact and add a new picture
* Go to list and the image is not available

When you have a lot of contacts and the scroll feature is available, the same behavior happens but if you try to move a bit the list, the new image appears

Comment 31

[Cristian Rodriguez (:crdlc)]

Related to comment 29, I've just seen this 

https://github.com/mozilla-b2g/gaia/pull/10582/files#L1R570

IMHO the image loader should release the images and not the contact list library

What do you think?

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Thanks for the feedback Cristian!  I put some comments in Github and working to make changes now.  I will try to make small isolated commits since Francisco is about to start his review.

Also, I just noticed that two of the unit tests related to image rendering are failing.  I think this might have happened when I moved from my working branch to the PR and rebased to master.  I will fix these as well.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

I've pushed commits for most of the comments in the pull request.  I still need to investigate the issues from comment 28 and comment 30.  I will work on that later today.

The test failure I mentioned in comment 32 was actually just a stale profile build from some other work I was doing.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Cristian Rodriguez de la Cruz (:crdlc) from comment #28)
> * Empty list > Added "García Marquez" > Go to search, type "ff" -> I don't
> see "No contacts found"
> 
> Maybe it is not working on master, I don't know, could you take a look?

I get the same behavior on master without my pull request applied.  A brief glance at the code suggests that the way we reuse search results may be part of the issue.  For example, if you have results for 'q' and then move to 'qq' with no results, I think state.count will still be non-zero.  The no-results page is only showed when state.count equals zero.

Can we open this as a separate bug?

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Cristian Rodriguez de la Cruz (:crdlc) from comment #30)
> Other minor bug:
> 
> * Empty list
> * Added a new contacts named "García Marquez" or whatever :)
> * Go to list and "Garcia Marquez" is there
> * Edit this contact and add a new picture
> * Go to list and the image is not available
> 
> When you have a lot of contacts and the scroll feature is available, the
> same behavior happens but if you try to move a bit the list, the new image
> appears

Fixed here:

  https://github.com/wanderview/gaia/commit/18900c12132b8bfb9c6b5ef4983be3f50a686e30

This was caused because imgLoader.reload() was happening before the visibility monitor called onscreen() for the new contact causing renderPhoto().  To work around this just force new contacts to have renderPhoto().  We may get some additional photo's temporarily, but the offscreen() calls should purge them fast enough.

Comment 36

[Cristian Rodriguez (:crdlc)]

> I get the same behavior on master without my pull request applied.  A brief glance at the code suggests that the way we reuse search results may be part of the issue.  For example, if you have results for 'q' and then move to 'qq' with no results, I think state.count will still be non-zero.  The no-results page is only showed when state.count equals zero.

> Can we open this as a separate bug?

Yep, it makes sense my friend, more bugs jjajajaja

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

I've pushed fixes for or responded to all the comments in the Github PR.  I also opened bug 887801 to track the missing "no results" message in search.

Once bug 865750 lands I'll rebase, retest, and ask for official r+.

Thanks again for the reviews!

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Updated multilevel_visibility_monitor.js file

Updated monitor code after addressing review feedback.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

I've updated the pull request to account for changes to the multilevel_visibility_monitor API.  Still waiting for it to land, but seems close.

I'm now going to rebase master to clear any conflicts.  I also want to test against proposed patch in bug 887801.  I have the feeling I don't handle this progress/no-results display quite right in search.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Actually I'm going to wait to rebase.  I realized bug 888465 exists and I would like to land that before merging this with master since they overlap in the code.  Hopefully this won't be a problem since the multilevel_visibility_monitor still needs to land.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

I've rebased the github pull request to master.  This required some merging and fixing of tests.  We are still waiting for bug 865750 to complete review.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Attachment: New tag_visibility_monitor.js

I've updated the pull request to use Evan's new tag_visibility_monitor.js.  To test you now need this file in shared/js instead of the old multilevel_visibility_monitor.js.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Rebased to master again now that bug 865750 has landed.  I just need to tweak a few things to account for the progress/no-results pane now getting displayed thank to bug 887801.  After that I will ask you guys to look at it one last time.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Pushed a final commit so that the progress and no-results search overlays work properly while contacts are still loading.

Francisco and Cristian, can you take a final look?  If things look good to you and you are willing to r+ I can squash my commits and merge.

Thanks!

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 770273 [details]
New tag_visibility_monitor.js

Obsolete since this file has landed in the tree.

Comment 46

[Cristian Rodriguez (:crdlc)]

Hi, great work!! I will check the functionality tomorrow because I am busy today and I don't have enough time, right?

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Cristian Rodriguez de la Cruz (:crdlc) from comment #46)
> Hi, great work!! I will check the functionality tomorrow because I am busy
> today and I don't have enough time, right?

No problem.  Its a holiday in the US, so I won't really be able to respond to feedback until Monday anyways.  Thanks!

Comment 48

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

Hi Ben,

I'm traveling back to UK today, but will take a look during the weekend :)

Comment 49

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

Hi Ben,

just r+plused bug 883269, that is a leo+ and solves an ordering problem in the current contact list. The change is pretty small, but you probably will need to rebase.

As well, this could be already saved with your patch, but since the other one was a leo+ and this one is (still) not, gave it a bit of priority.

Hopefully the rebase shouldn't be complicated at all.

Thanks again,
F.

Comment 50

[Cristian Rodriguez (:crdlc)]

Comment on attachment 766812 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10582

Hi, 

   You implemented all my suggestions and I've tested your patch with contacts from social networks, local contacts, with and without pictures,... and I don't find any regression right now. You fixed the problem about setting a picture and refreshing this one on the list. Then my feedback is super +!

Thanks

Great work!

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Francisco Jordano [:arcturus] from comment #49)
> just r+plused bug 883269, that is a leo+ and solves an ordering problem in
> the current contact list. The change is pretty small, but you probably will
> need to rebase.
> 
> As well, this could be already saved with your patch, but since the other
> one was a leo+ and this one is (still) not, gave it a bit of priority.
> 
> Hopefully the rebase shouldn't be complicated at all.

Actually, I have concerns about how bug 883269 was fixed and I think it could cause a performance problem.  The sorting logic in addToGroup() is not as efficient as what gecko does and is going to introduce a lot of CPU usage at load time.  I'll test with my changes and see what the impact is.  Sorry I didn't see that bug until now.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Rebased my github branch with master to include bug 883269.  With the addToGroup() change scrolling is now janky and the time to complete the load of the contacts went from 7.6 seconds to 9.6 seconds.

I think we need to fix the sort cache issue in gecko in order to incorporate a fix for bug 883269 with this pull request.

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Ok, I talked to Mihai and we backed out bug 883269 for now until we can fix the issue in Gecko.  I rebased this pull request again to include the backout.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Sorry to ping you again Francisco.  I haven't had any luck catching you on IRC the last couple of days.

I was just wondering if you think we can move forward with the visibility monitor integration changes given that bug 883269 was backed out for now.  (I believe Reuben is working it on the Gecko side of things.)

I'd like to land this soon, if possible, to avoid bit rot.

Thanks!

Comment 55

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

Comment on attachment 766812 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10582

Sorry I've been a bit busy this two days.

I couldn't merge the patch with master, but downloaded your branch saw that you implemented all the suggestions, tried on the phone, unit test are passing, now the world is better and safer with this PR.

Thanks a lot Ben for the work here, definitely an incredible and talented PR.

Just one thing, just r+ this, but could you rebase (well, this is needed to do the PR) and squash the changes in a single commit?

Thanks a lot!
Francisco.

Comment 56

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

Hi,

good idea to back out  bug 883269. Now this should land once rebased and squashed (like how it sounds :))

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

Things still looked good after the rebase, so I went with the squash and merge.  See:

  https://github.com/mozilla-b2g/gaia/commit/d88d4066965961de9b0ba194b2a68afc32d91f98

Thank you again both Francisco and Cristian!

Comment 58

[Joe Cheng [:jcheng] (please needinfo)]

1.2 is still on master/m-c, if it's resolved fixed, it's in v1.2