1169576 - [Messages][NG] Implement Conversation service: method for streaming joined threads and drafts list

Status: RESOLVED FIXED

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

Description

[Oleg Zasypkin [:azasypkin]]

In this bug we'd like to implement "getThreads" (may have better name) defined in bug 1169573.

Comment 2

[Oleg Zasypkin [:azasypkin]]

Stealing while Julien is busy with navigation stuff.

Comment 3

[Oleg Zasypkin [:azasypkin]]

Quick update: was blocked by https://github.com/gaia-components/threads/pull/52, now back to work on this :)

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Comment 5

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Hey guys,

What do you think about this PR? Second commit is for demo purposes only if you want to run it locally in Fx Desktop (http://sms.gaiamobile.org:8080/views/inbox).

I still see some weird issue on Desktop sometimes - when I refresh page with ctrl-f5 bridge doesn't work as expected and it's fine when I refresh with just f5, I'm looking into this.

Thanks!

Comment 6

[Oleg Zasypkin [:azasypkin]]

Added some performance numbers to PR [1].

[1] https://github.com/mozilla-b2g/gaia/pull/31051#issuecomment-124576360

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Looks good and I think we're in the right direction despite the possible performance regression and we might need to fix/tune it in both bridge and app, great job!

Comment 8

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #5)
> 
> I still see some weird issue on Desktop sometimes - when I refresh page with
> ctrl-f5 bridge doesn't work as expected and it's fine when I refresh with
> just f5, I'm looking into this.

Sounds like a race in the startup code (stating the obvious) related to caches (ctrl-f5 bypass the cache and force-reloads everything).

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

left comments and thoughts on github (I haven't actually tried it TBH)

If we do batching I think it's better to do it in a separate bug; I'm fine landing this now and iterating on the performance.

great work !

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Hey guys,

I think it should be ready for the first round of review. I tried to keep PR as small as I could and split integration work in separate bugs, it's not used by any code yet, but you can try it on device or in Fx Desktop using my WIP branch [1]. WIP branch is based on PR + some glue changes.

As you know performance numbers are quite bad, so we can't use this service in master as is. At the moment I'm measuring and experimenting with different setups (use window/iframe instead of SharedWorker, get rid of BroadcastChannel and etc.). I'll let you know the results, I hope we can find trade off approach to start using conversation service in master as soon as possible.

Thanks!

[1] https://github.com/azasypkin/gaia/tree/bug-xxx-use-conversation-service

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

I don't have much to say; I left some comments but they're not big issues.

We mostly have to find out how we can cut the performance issue...

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Patch looks good and I only left some minor nits. Let's see the result after rebased. Thanks!

Comment 13

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Hey guys,

* PR is rebased on the Steve's MessagingClient PR;
* I've handled your comments in a separate commit;
* I've posted all measurements and analysis I had to GitHub.

I'm going to handle performance problems in the follow-up to keep this work more maintainable and reviewable :)

So, please tell me what you think!

Thanks

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Just left some nits but this looks good to me.

So r=me with nits fixed and a green try (latest try didn't run properly).

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8636623 [details] [review]
[gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Overall looks good, let's move on for the profiling part. Thanks!

Comment 16

[Oleg Zasypkin [:azasypkin]]

Thanks for review guys!

Treeherder is green (except for the one failing GijTV test that seems unrelated).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Oleg Zasypkin [:azasypkin]]

Backed out because of regression (see bug 1199612): https://github.com/mozilla-b2g/gaia/commit/e9c57d7d952c9466548ab75f67e54f9ccf028360.

Should be relanded together with PR for bug 1199612.

Comment 19

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1169576-conversation-service-get-threads > mozilla-b2g:master

Comment 20

[Oleg Zasypkin [:azasypkin]]

Patch for bug 1199612 is r+'ed, Treeherder is green, so re-landed.

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