1050823 - [Messages][Refactoring] Revise "ThreadUI.updateHeaderData" call while retrieving threads

Status: RESOLVED FIXED

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

Description

[Oleg Zasypkin [:azasypkin]]

*** Follow-up from bug 1041124 ***

Looks like we don't need this call [1] anymore, but let's use this bug to check it to be 100% sure.

[1] https://github.com/mozilla-b2g/gaia/blob/8aff4c9d819544ddf8f1fa0928e5bb532aeab617/apps/sms/js/message_manager.js#L149

Comment 1

[Oleg Zasypkin [:azasypkin]]

Will take a look if ThreadListUI(InboxView) still needs ThreadUI(ConversationView).updateHeaderData call since it's direct dependency between two views and blocks bug 1155509.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1050823-remove-update-header > mozilla-b2g:master

Comment 3

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8598219 [details] [review]
[gaia] azasypkin:bug-1050823-remove-update-header > mozilla-b2g:master

Hey Julien,

Looks like we don't need this call anymore, I found only two cases when can call "updateHeaderData" when ThreadListUI is not rendered yet:

* When we open application via "new" activity - in this case "updateHeaderData" should work fine since Thread.active is set via [1];

* When we open application via notification - in this case "updateHeaderData" should work fine as well since Thread.active is set via [2];

I also added some integration tests to cover these non-really-obvious cases.

[1] https://github.com/mozilla-b2g/gaia/blob/42493f924fb1842976b1195689d9c755b539db31/apps/sms/js/message_manager.js#L515

[2] https://github.com/mozilla-b2g/gaia/blob/42493f924fb1842976b1195689d9c755b539db31/apps/sms/js/activity_handler.js#L241

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8598219 [details] [review]
[gaia] azasypkin:bug-1050823-remove-update-header > mozilla-b2g:master

I left some comments for the marionette tests.

If you want to land the code change + unit test change now and follow-up to get the marionette tests right it would work for me too.

Comment 5

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8598219 [details] [review]
[gaia] azasypkin:bug-1050823-remove-update-header > mozilla-b2g:master

Hey Julien,

I've fixed nits + added one more integration test we discussed in PR. I tend to leave both tests, but tell me what you think.

Thanks!

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8598219 [details] [review]
[gaia] azasypkin:bug-1050823-remove-update-header > mozilla-b2g:master

r=me with some comments:

* I think we should set the storage before actually launching the app -- unless I miss something that makes this not desirable.
* file a bug to share the code for "launching with system message"
* some various nits.

Comment 7

[Oleg Zasypkin [:azasypkin]]

Thanks for review!

(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #6)
> Comment on attachment 8598219 [details] [review]
> [gaia] azasypkin:bug-1050823-remove-update-header > mozilla-b2g:master
> * I think we should set the storage before actually launching the app --
> unless I miss something that makes this not desirable.

As we discussed it's impossible since there is no TestStorage until we run the app and inject content script to app frame.

> * file a bug to share the code for "launching with system message"

Yep, filed bug 1162165.

> * some various nits.

Nits fixed!

Comment 8

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/ce69e822270eb5e2d0ae3943cd09864d02af7e89