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

RESOLVED FIXED in FxOS-S5 (21Aug)

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

unspecified
FxOS-S5 (21Aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sms-sprint-FxOS-S5])

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
julienw
: review+
julienw
: feedback+
Details | Review | Splinter Review
(Assignee)

Description

3 years ago
If we move the Threads into service, requesting the active thread would be redundant and inefficient. We can simply cache the thread in local.
(Assignee)

Comment 1

3 years ago
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?
Flags: needinfo?(felash)
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 !
Flags: needinfo?(felash)
(Assignee)

Comment 3

3 years ago
Created attachment 8628750 [details] [review]
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!
Attachment #8628750 - Flags: feedback?(felash)
Comment on attachment 8628750 [details] [review]
Link to github

left some comments :)

but yeah this looks good
Attachment #8628750 - Flags: feedback?(felash) → feedback+
(Assignee)

Comment 5

3 years ago
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.
Attachment #8628750 - Flags: review?(felash)
Comment on attachment 8628750 [details] [review]
Link to github

One oversight and some comments :)

Thanks !
Attachment #8628750 - Flags: review?(felash)
(Assignee)

Comment 7

3 years ago
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.
Attachment #8628750 - Flags: review?(felash)
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 :)
Attachment #8628750 - Flags: review?(felash)
(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
(Assignee)

Comment 10

3 years ago
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.
Attachment #8628750 - Flags: review?(felash)
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.
Attachment #8628750 - Flags: review?(felash)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8628750 [details] [review]
Link to github

All the active/currentId are removed. Enjoy it!
Attachment #8628750 - Flags: review?(felash)
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 !!
Attachment #8628750 - Flags: review?(felash) → review+
(Assignee)

Comment 14

3 years ago
Thanks!

In master: https://github.com/mozilla-b2g/gaia/commit/a19cb0d33f2f5dc3b18104e9b071d0bc11d24f35
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
Whiteboard: [sms-sprint-FxOS-S5]
You need to log in before you can comment on or make changes to this bug.