1112135 - [Messages] When starting as an activity/notification/, delay threads rendering until the right panel is open

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

While working on bug 1109745, we see that the logic to open the right panel competes with the thread rendering that happens in background.

In this bug, the goal is to delay the thread rendering so that opening the right panel is really faster.

Comment 1

[Oleg Zasypkin [:azasypkin]]

Some updates:

* Delaying threads rendering (and retrieving as currently it's highly coupled) works quite good when next panel is Composer("share" activity and "new" activity when contact doesn't have existing thread);

* For "notification" and "new" activity(for contact with existing thread) cases we have dependencies for the thread object (Thread.active) and Drafts that are fulfilled during thread list rendering. At the first look "new" activity kind of partially works as ActivityHandler creates corresponding Thread object (via Threads.registerMessage), but we'll still miss draft for this thread if it exists.

Will think how we can overcome this.

Comment 2

[Julien Wajsberg [:julienw]]

We can use Drafts.request() ?
(eventually we need to convert this to promise, btw :) )

Comment 3

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> We can use Drafts.request() ?
> (eventually we need to convert this to promise, btw :) )

Yep!

Comment 4

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Hey Julien,

Here is the WIP, without units for now. I played a bit with it, and it feels better on flame + heavy workload. 

The only thing is that there's noticeable delay when you open Thread with draft via "new" activity.

Could you please give it a try?

Thanks!

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8539435 [details] [review]
GitHub pull request URL

I tested "share" and "new" activities, and both are really faster now. I couldn't "notification" but I suspect it's the same :) Good work!

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8539435 [details] [review]
GitHub pull request URL

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #5)
> Comment on attachment 8539435 [details] [review]
> GitHub pull request URL (WIP)
> 
> I tested "share" and "new" activities, and both are really faster now. I
> couldn't "notification" but I suspect it's the same :) Good work!

Hey Julien,

Thanks for the feedback! I've added unit tests + some adjustments as separate commit.

Could you please review it?

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8539435 [details] [review]
GitHub pull request URL

Added comments on github.

This is really looking good. I compared both with master and with my v2.0 phone, and it's now faster than both versions for new, share and notification.

For share, we can still feel that displaying the image struggles because we started rendering the thread list. I think we can do better in the future, like appending the image in beforeEnter and resolving the returned promise only once the image is loaded. It will be for a separate bug but it can be a nice improvement, and this will look slicker because the panel will be nearly ready when it will be displayed. Anyway, this is for a separate bug, let's finish this bug first!

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8539435 [details] [review]
GitHub pull request URL

Hey Julien,

Fixed per your comments + rebased patch on the latest master as it had some conflicts.

Could you please take a look again?

Thanks!

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8539435 [details] [review]
GitHub pull request URL

r=me with a last nit in test

please land with green tests !

Thanks a lot, this is great :)

Comment 10

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (PTO until 1/5) from comment #9)
> Comment on attachment 8539435 [details] [review]
> GitHub pull request URL
> 
> r=me with a last nit in test
> 
> please land with green tests !
> 
> Thanks a lot, this is great :)

Thanks a lot for review! Treeherder is green.

Master: https://github.com/mozilla-b2g/gaia/commit/fce979e88240aba9f106c41b299151d3676bed99