Closed Bug 1226555 Opened 4 years ago Closed 4 years ago

[Messages] Split views are not rendered anymore

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: azasypkin)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR:
1. load app://sms.gaiamobile.org/views/conversation/index.html#?id=3 in a Firefox or Mulet in a DEBUG=1 DESKTOP=1 profile.

Issue is that we have a deadlock:
* in startup we wait for visually-loaded to load the lazy dependencies [1]
* still in startup we call Navigation.setReady() when the lazy dependencies are loaded [2]
* but we render the messages and send the "visually-loaded" event in afterEnter... [3] * and we wait for setReady to call afterEnter [4]

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/conversation/js/startup.js#L76
[2] https://github.com/mozilla-b2g/gaia/blob/74bb9b37ca84a4e970344aaddb26f1bc135d3af5/apps/sms/views/conversation/js/startup.js#L47
[3] https://github.com/mozilla-b2g/gaia/blob/74bb9b37ca84a4e970344aaddb26f1bc135d3af5/apps/sms/views/conversation/js/conversation.js#L1492
[4] https://github.com/mozilla-b2g/gaia/blob/74bb9b37ca84a4e970344aaddb26f1bc135d3af5/apps/sms/views/shared/js/navigation.js#L811


Regression from bug 1217859
Will take a look, likely we'll need to get rid of Navigation.waitForThePanelToBeReady entirely as was discussed on irc.

Also we'll see if there any way to run integration tests for the split view version.
Flags: needinfo?(azasypkin)
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Flags: needinfo?(azasypkin)
See Also: → 1227147
Comment on attachment 8690851 [details] [review]
[gaia] azasypkin:bug-1226555-conversation-split-mode > mozilla-b2g:master

Hey Julien,

How does it look? Some notes:

* I didn't notice any user facing issues while testing it, though we can get exceptions if we quickly try to do something that depends on files that are lazy loaded (like quickly tap on the phone link, but will work once LinkActionHandler is loaded). It's very edgy and not noticeable by user, and if we need something in the very beginning - we can just load it on startup;

* Integration test trick looks hacky, but it can help us to avoid some bit-rotting for split view mode. Tell me what you think!

Thanks!
Attachment #8690851 - Flags: review?(felash)
Depends on: 1227219
Comment on attachment 8690851 [details] [review]
[gaia] azasypkin:bug-1226555-conversation-split-mode > mozilla-b2g:master

Some nits, and one question.

Please put your changes in a separate commit to ease the next review :)

Thanks !
Attachment #8690851 - Flags: review?(felash)
Depends on: 1227147
Hey Julien, just rebased on the latest master and see that Inbox view doesn't load anymore as well (new integration test failed \0/) :)

This is regression from bug 1206727, the fix is trivial so I'll cover it in this PR if you don't mind.
Blocks: 1206727
Summary: [Messages] The Conversation split view is not rendered anymore → [Messages] Split views are not rendered anymore
OK ! (and sorry, grrr)
Comment on attachment 8690851 [details] [review]
[gaia] azasypkin:bug-1226555-conversation-split-mode > mozilla-b2g:master

Hey Julien,

I think it's ready for the 2nd round of review :)

Johan,

Could you please take a look at "mock_split_view_mode.js" + how it's used and say if it looks sane for you? It's temporary and hopefully will be removed in the nearest future :)

Thanks!
Attachment #8690851 - Flags: review?(felash)
Attachment #8690851 - Flags: feedback?(jlorenzo)
Comment on attachment 8690851 [details] [review]
[gaia] azasypkin:bug-1226555-conversation-split-mode > mozilla-b2g:master

(In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #7)
> Could you please take a look at "mock_split_view_mode.js" + how it's used
> and say if it looks sane for you?

Yes, it does. The hack is simple enough. A manifest injection would require more hacks in marionette apps. 

I ran the tests locally in order to spot a potential race condition due to the hack, but my eyes didn't catch anything. I guess the mock is fully synchronous, so all the waits are done correctly, right?
Attachment #8690851 - Flags: feedback?(jlorenzo) → feedback+
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #8)
> Comment on attachment 8690851 [details] [review]
> [gaia] azasypkin:bug-1226555-conversation-split-mode > mozilla-b2g:master
> 
> (In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #7)
> > Could you please take a look at "mock_split_view_mode.js" + how it's used
> > and say if it looks sane for you?
> 
> Yes, it does. The hack is simple enough. A manifest injection would require
> more hacks in marionette apps. 
> 
> I ran the tests locally in order to spot a potential race condition due to
> the hack, but my eyes didn't catch anything. I guess the mock is fully
> synchronous, so all the waits are done correctly, right?

Thanks! Yeah, it should, all observers that mocks register should fire before app is run (actually all of them will be invoked once again once we replace URL). And all this should happen before "app.launch()" returns and we do any "wait"s.
Comment on attachment 8690851 [details] [review]
[gaia] azasypkin:bug-1226555-conversation-split-mode > mozilla-b2g:master

Looks good, let's land this :) r=me with some simple nits
Attachment #8690851 - Flags: review?(felash) → review+
Thanks! Nits are fixed, Treeherder is green.

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