907907 - [Contacts] Implement a lazily loaded DOM, based on WebComponents

Status: RESOLVED FIXED

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

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 793685 [details]
Github pull request pointer

Hi Francisco - When you get a chance please take a look and add any other people that you think should take a look. Thanks!

Comment 3

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

Comment on attachment 793685 [details]
Github pull request pointer

Great job!

I'm sure the datazilla will be super happy.

Just left two issues on github, that are basically:

- The search dom must be loaded when we are about to export to any source, actually is not export, but use the list in select mode.
- Now that we are loading the overlay dom asynchronously we will need to transform the rest of the code calls to asynchronous, since before we were returning the dom node to allow the app to update the overlay.

A part from that, just have to thank you for the fantastic job, and sorry for the delay ;)

Cheers,
F.

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 793685 [details]
Github pull request pointer

Thanks for the awesome review Francisco! Update the PR with some issues addressed.

Comment 5

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

Comment on attachment 793685 [details]
Github pull request pointer

Looking perfect for me.

Adding Jose as a reviewer in case we want to double check the FB part.

Thanks a lot Kevin for the hard work, definitely will make a difference.

Comment 6

[Kevin Grandon :kgrandon]

According to Jose, this patch is currently too big. I'm going to open a few dependent bugs for the smaller pieces, and leave the big one as-is for now. Patches to come.

Current diff shows: 36 changed files with 748 additions and 638 deletions.

Comment 7

[Kevin Grandon :kgrandon]

With some refactoring of work, I was able to open multiple pull requests. The main pull request is now mainly HTML movement. The commit stats are as follows:

Old: 36 changed files with 748 additions and 638 deletions.
New: 13 changed files with 549 additions and 512 deletions. 

Note, almost the entirety of the new pull request is moving HTML around.

Comment 8

[Jose Manuel Cantera]

I will be reviewing this patches in a low priority thread, as they are not blocking v1.2 which is currently our priority.

Comment 9

[Kevin Grandon :kgrandon]

(In reply to Jose M. Cantera from comment #8)
> I will be reviewing this patches in a low priority thread, as they are not
> blocking v1.2 which is currently our priority.

I think we need to work together to find a better solution than that. Typically the priority should be reviews over anything else - this allows other people to move much faster. Overall the entire gaia team can move much faster if we knock out reviews quicker.

Comment 10

[Kevin Grandon :kgrandon]

(In reply to Jose M. Cantera from comment #8)
> I will be reviewing this patches in a low priority thread, as they are not
> blocking v1.2 which is currently our priority.

I suppose the proper thing to do is to get this bug properly prioritized. Because this bug blocks a 1.2 bug for us (871823), I think it makes sense to have this one block as well.

Comment 11

[Jose Manuel Cantera]

As I said to Dietrich in a private e-mail 

"Do you have any estimation about the perf win that these patches provide?
In my opinion it can be risky to land this code in 1.2 as now the Contacts
App is very stable and IMHO the regression risk is high with all that
code."

Comment 12

[Kevin Grandon :kgrandon]

(In reply to Jose M. Cantera from comment #11)
> "Do you have any estimation about the perf win that these patches provide?
> In my opinion it can be risky to land this code in 1.2 as now the Contacts
> App is very stable and IMHO the regression risk is high with all that
> code."

My back of the envelop estimation would be on the order of saving ~100ms once all of these patches land. I'll try to do some more profiling to get exact numbers, but they should be significant.

I was under the impression that contacts app performance was blocking 1.2. If it's not, then there's no problem moving these to 1.3.

Comment 13

[Mike Lee [:mlee]]

(In reply to Kevin Grandon :kgrandon from comment #12)
> ...
> I was under the impression that contacts app performance was blocking 1.2.
> If it's not, then there's no problem moving these to 1.3.

It *is* blocking 1.2, see bug 915073 for the regression since 1.1.

Comment 14

[Jose Manuel Cantera]

This patch is only saving us 100ms and the amount of new code and regression risk is high. As according to bug 915073 we need to save 1 sec I believe this is not the performance win we need. It seems to me that this is a cosmetic change that is not going to make a difference.

Comment 15

[Jose Manuel Cantera]

(In reply to Kevin Grandon :kgrandon from comment #9)
> (In reply to Jose M. Cantera from comment #8)
> > I will be reviewing this patches in a low priority thread, as they are not
> > blocking v1.2 which is currently our priority.
> 
> I think we need to work together to find a better solution than that.
> Typically the priority should be reviews over anything else - this allows
> other people to move much faster. Overall the entire gaia team can move much
> faster if we knock out reviews quicker.

yes, but reviewers time is precious and they shouldn't be spending time in reviewing changes that do not make a real difference and that only introduce regression risks.

Comment 16

[Kevin Grandon :kgrandon]

(In reply to Jose M. Cantera from comment #14)
> This patch is only saving us 100ms and the amount of new code and regression
> risk is high. As according to bug 915073 we need to save 1 sec I believe
> this is not the performance win we need. It seems to me that this is a
> cosmetic change that is not going to make a difference.

100ms is absolutely a huge win. If we have 10 wins at 100ms each, we will meet our goal of 1 sec performance improvements. You're not going to find a one-liner at this point which will shave off 1s of load time. It's going to be a long hard fight, and we're trying to help with that.

Comment 17

[Dietrich Ayala (:dietrich)]

100ms is *huge*. Jose, please review the UX guidelines for performance and responsiveness at https://mozilla.app.box.com/s/aww17rx74k7fjds5vada.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jose M. Cantera from comment #15)
> (In reply to Kevin Grandon :kgrandon from comment #9)
> > (In reply to Jose M. Cantera from comment #8)
> > > I will be reviewing this patches in a low priority thread, as they are not
> > > blocking v1.2 which is currently our priority.
> > 
> > I think we need to work together to find a better solution than that.
> > Typically the priority should be reviews over anything else - this allows
> > other people to move much faster. Overall the entire gaia team can move much
> > faster if we knock out reviews quicker.
> 
> yes, but reviewers time is precious and they shouldn't be spending time in
> reviewing changes that do not make a real difference and that only introduce
> regression risks.

You don't need to do a full review since Francisco has already done one. I think he asked for an additional review mostly to check if nothing will broke for the facebook code path.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

As a data point, I added class=hide to the offscreen panel <section> elements.  This seems to improve load time by 200ms to 300ms.  I think landing this will be a big win.

We may still want to consider using hide class when a panel is dismissed and goes offscreen, though.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Video of v1.2 with these patches applied.  Launch time is about 1.9s which seems to be about 100ms improvement as Kevin originally predicted.  My times in comment 19 were measured from JS, so did not capture the entire picture.

  https://www.dropbox.com/s/g11ykbfu8ga66w5/20130923_lazy_html.mov

Comment 21

[Jose Manuel Cantera]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #18)
> (In reply to Jose M. Cantera from comment #15)
> > (In reply to Kevin Grandon :kgrandon from comment #9)
> > > (In reply to Jose M. Cantera from comment #8)
> > > > I will be reviewing this patches in a low priority thread, as they are not
> > > > blocking v1.2 which is currently our priority.
> > > 
> > > I think we need to work together to find a better solution than that.
> > > Typically the priority should be reviews over anything else - this allows
> > > other people to move much faster. Overall the entire gaia team can move much
> > > faster if we knock out reviews quicker.
> > 
> > yes, but reviewers time is precious and they shouldn't be spending time in
> > reviewing changes that do not make a real difference and that only introduce
> > regression risks.
> 
> You don't need to do a full review since Francisco has already done one. I
> think he asked for an additional review mostly to check if nothing will
> broke for the facebook code path.

And because the patch is huge and as we say in Spain, "four eyes can see more than two", and in my case as I wear glasses they are six :)

Comment 22

[Jose Manuel Cantera]

Kevin,

I see a major regression with this patch. The path is:

Enter into 'Search Mode' by putting the cursor on the search box in the Contacts list. Then, exit search mode by clicking on 'Cancel' an ugly and spureous white screen will appear. If you do the same with current master, you will see that the transition between the search mode and list mode is quite smooth. 

Please could you review that issue? It is the *only* issue I can see in the patch

thanks

Comment 23

[Jose Manuel Cantera]

(In reply to Jose M. Cantera from comment #22)
> Kevin,
> 
> I see a major regression with this patch. The path is:
> 
> Enter into 'Search Mode' by putting the cursor on the search box in the
> Contacts list. Then, exit search mode by clicking on 'Cancel' an ugly and
> spureous white screen will appear. If you do the same with current master,
> you will see that the transition between the search mode and list mode is
> quite smooth. 
> 
> Please could you review that issue? It is the *only* issue I can see in the
> patch
> 

Another major regression. Go to Contacts/Import Gmail. Import some contacts and then the status message with the number of imported contacts will not appear. In the console you will find

E/GeckoConsole( 4245): [JavaScript Error: "TypeError: statusMsg.querySelector(...) is null" {file: "app://communications.gaiamobile.org/contacts/js/utilities/status.js" line: 10}]


I have set r-. Please react on this problems and ask for a new review. 

> thanks

Comment 24

[Kevin Grandon :kgrandon]

I distinctly remember testing these issues ~1 month ago when the patch was submitted and they were working so perhaps there is some bit-rot. I will investigate and add tests where possible. Thanks for testing!

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 793685 [details]
Github pull request pointer

Hi Jose,

I think that the fact that the applying of the patch for this one was a bit confusing due to the multi-patch was causing some issues.

Now that the dependent bugs have landed, I have rebased this against master and it should be easier to apply. With the simplier patch I've checked that the white flash screen is not there, and that the gmail import shows the counter properly. Please verify and let me know if you have any issues. Thanks!

Comment 26

[Kevin Grandon :kgrandon]

@Jose - Actually I may have mus-interpreted your comment, but I did go through and find the statusMsg error you were talking about. The PR has been updated to reflect the necessary change. Thanks.

Comment 27

[Jose Manuel Cantera]

Comment on attachment 793685 [details]
Github pull request pointer

now the reported issues seem to have gone.

Comment 28

[Jose Manuel Cantera]

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

Comment 29

[James Lal [:lightsofapollo]]

Uplifted f1b488cec4f1757c99fe3e2eb8a16b44ed2d287d to:
v1.2: 33eabf186cd1d6980872502dfe7fa4104b5645b4