1167144 - [Messages] Reduce the use of Threads.active and Threads.currentId in conversation view

Status: RESOLVED FIXED

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

Description

[Steve Chung [:steveck]]

If we move the Threads into service, requesting the active thread would be redundant and inefficient. We can simply cache the thread in local.

Comment 1

[Steve Chung [:steveck]]

Hi Julien, for some place like activity-handler and closeNotificationsForThread we might need this for checking current thread directly. Maybe We can remove it once the navigation is ready and we can check current thread id by hash, or you have better idea for caching this information?

Comment 2

[Julien Wajsberg [:julienw]]

I'm not sure I get your question :)

I think we'll have a service to get thread information; in the Conversation view, we can cache locally the thread information because we can use it in this view, but if we need the information elsewhere we can occasionaly use the service too.

Tell me if I misunderstood something !

Comment 3

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, this patch is the simple idea about reducing the Conversation service dependency. I add some comments for the part that we might will need to service method to get thread or change certain thread property. Please let me know if you have any other thought about caching thread/threadId in conversation view, thanks!

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8628750 [details] [review]
Link to github

left some comments :)

but yeah this looks good

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8628750 [details] [review]
Link to github

Polish the patch that removing the use of current thread id in Threads object in Activity_handler and Utils. I didn't remove the thread id entirely from Threads because it might conflict with your ongoing Navigation patch. But you can hold the review if you still found any conflicting with your navigation patch.

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8628750 [details] [review]
Link to github

One oversight and some comments :)

Thanks !

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8628750 [details] [review]
Link to github

I found the problems that cause the test failed. Addressed for next review.
BTW I didn't found any document about jsdoc @returns for Promise value, not really sure if we can use {Promise}<void> here.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8628750 [details] [review]
Link to github

Some more "Threads.active" to replace, a nit, and an integration test still failing :)

Can you please add your changes in a separate commit ? This makes it easier to review again :)

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #7)
> Comment on attachment 8628750 [details] [review]
> Link to github
> 
> I found the problems that cause the test failed. Addressed for next review.
> BTW I didn't found any document about jsdoc @returns for Promise value, not
> really sure if we can use {Promise}<void> here.

There is a whole thread in [1] about documenting promises. I think for such promises we could use {Promise.<undefined>}, at least it's understandable. What do you think?

[1] https://github.com/jsdoc3/jsdoc/issues/509#issuecomment-124581305

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8628750 [details] [review]
Link to github

Fixed some other issues and test, and also the the jsdoc part to be more clear about the returned promise result.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8628750 [details] [review]
Link to github

We need a rebase because of bug 1162030.

I also left a suggestion at https://github.com/mozilla-b2g/gaia/pull/30798/files#r36626890. I think this could be a good bug to do it, and it should be quite easy, but I'm fine if you prefer a separate bug for this.

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8628750 [details] [review]
Link to github

All the active/currentId are removed. Enjoy it!

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8628750 [details] [review]
Link to github

r=me with 3 nits to fix before landing:
* 1 jshint failure
* 1 style nit
* mock_threads' mTeardown still references Threads.currentId and Threads.active

Please wait for a green try before merging !

good job !!

Comment 14

[Steve Chung [:steveck]]

Thanks!

In master: https://github.com/mozilla-b2g/gaia/commit/a19cb0d33f2f5dc3b18104e9b071d0bc11d24f35