Closed Bug 1169576 Opened 5 years ago Closed 4 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-master fixed)

RESOLVED FIXED
FxOS-S6 (04Sep)
Tracking Status
b2g-master --- fixed

People

(Reporter: azasypkin, Assigned: azasypkin)

References

Details

(Whiteboard: [p(FxOS-S5)=3][p(FxOS-S4)=3][p(FxOS-S3)=3])

Attachments

(2 files)

In this bug we'd like to implement "getThreads" (may have better name) defined in bug 1169573.
Depends on: 1171865
No longer blocks: messages-nga
Depends on: 1179628
No longer depends on: 1179628
No longer blocks: ng-messaging-service
No longer depends on: 1169573
Summary: [Messages][NG] Implement Messaging service: method for retrieving of all threads → [Messages][NG] Implement Conversation service: method for retrieving of all threads
Whiteboard: [p=1]
Target Milestone: 2.2 S14 (12june) → ---
Summary: [Messages][NG] Implement Conversation service: method for retrieving of all threads → [Messages][NG] Implement Conversation service: method for streaming joined threads and drafts list
No longer blocks: 1169541
Duplicate of this bug: 1169541
Whiteboard: [p=3]
Assignee: nobody → felash
Stealing while Julien is busy with navigation stuff.
Flags: needinfo?(azasypkin)
Quick update: was blocked by https://github.com/gaia-components/threads/pull/52, now back to work on this :)
Assignee: felash → azasypkin
Status: NEW → ASSIGNED
Flags: needinfo?(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!
Attachment #8636623 - Flags: feedback?(schung)
Attachment #8636623 - Flags: feedback?(felash)
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!
Attachment #8636623 - Flags: feedback?(schung) → feedback+
(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 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 !
Attachment #8636623 - Flags: feedback?(felash) → feedback+
Blocks: 1188765
Whiteboard: [p=3] → [p(FxOS-S4)=3][p(FxOS-S3)=3]
No longer blocks: 1188765
Whiteboard: [p(FxOS-S4)=3][p(FxOS-S3)=3] → [p(FxOS-S5)=3][p(FxOS-S4)=3][p(FxOS-S3)=3]
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
Attachment #8636623 - Flags: review?(schung)
Attachment #8636623 - Flags: review?(felash)
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...
Attachment #8636623 - Flags: review?(felash)
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 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
Attachment #8636623 - Flags: review?(felash)
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).
Attachment #8636623 - Flags: review?(felash) → review+
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!
Attachment #8636623 - Flags: review?(schung) → review+
Thanks for review guys!

Treeherder is green (except for the one failing GijTV test that seems unrelated).
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/664bed482e0f33f307e6f21e1341a0320ac81f13
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S6 (04Sep)
Blocks: 1199612
No longer blocks: 1199612
Depends on: 1199612
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Patch for bug 1199612 is r+'ed, Treeherder is green, so re-landed.

Master: https://github.com/mozilla-b2g/gaia/commit/26579a3ee39c5ecd1f35a8970ad1827be402d2e1
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.