Closed
Bug 1226555
Opened 9 years ago
Closed 9 years ago
[Messages] Split views are not rendered anymore
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
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
Assignee | ||
Comment 1•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Flags: needinfo?(azasypkin)
Assignee | ||
Updated•9 years ago
|
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8690851 [details]
[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)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8690851 [details]
[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)
Assignee | ||
Comment 5•9 years ago
|
||
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
Reporter | ||
Comment 6•9 years ago
|
||
OK ! (and sorry, grrr)
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8690851 [details]
[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]
[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+
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #8)
> Comment on attachment 8690851 [details]
> [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.
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8690851 [details]
[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+
Assignee | ||
Comment 11•9 years ago
|
||
Thanks! Nits are fixed, Treeherder is green.
Master: https://github.com/mozilla-b2g/gaia/commit/d5a26d135b9d5d2ff8994e3fca854d5a1a78c526
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•