Closed
Bug 1162028
Opened 10 years ago
Closed 9 years ago
[Messages][NG] Extract views/conversation/conversation.html
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
FxOS-S1 (26Jun)
People
(Reporter: julienw, Assigned: azasypkin)
References
Details
(Whiteboard: [p(2.2S13)=1][p=3])
Attachments
(1 file, 2 obsolete files)
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Hey guys, In this very WIP PR I just wanted to somehow get rid of ActivityHandler (and activity system message) dependency from ConversationView + one of ideas on how to organize services. My idea was to have activity service in top view (views/index.html that will host little-browser) that handles activity system messages and views (within iframes with their own URLs) use clients to receive activity request and post result back. Or maybe we should not do like this and wait for the solution from Gecko (either fix for system messages or for SW to be able to handle system messages). What do you think?
Attachment #8607957 -
Flags: feedback?(schung)
Attachment #8607957 -
Flags: feedback?(felash)
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8607957 [details]
GitHub Pull Request URL (WIP)
looks good
My main concern is that we'll need the service (or part of the service) to live in the main view, not in a worker or a shared worker or a separate iframe, for the reasons we know. And so the current "threads" lib won't work for this...
Attachment #8607957 -
Flags: feedback?(felash) → feedback+
Reporter | ||
Comment 3•9 years ago
|
||
Something that might work, but we need to check: maybe we can use a "window" type of service (in threads, see https://github.com/gaia-components/threads/#supplying-a-target for example -- without the "target" property). We can already try to look if something like this works: * index.html file * contains an iframe activity.html * activity.html handles the system message * manifest.webapp specifies index.html as endpoint We could use one of the branches of the testcase in bug 818000 to check for this. We need to see if everything (app is launched or not, etc) works with this setup -- if this works then it may be a win. Otherwise we can ask the Threads library team how we can overcome the issue (likely providing a in-thread service object instead of a separate worker or window).
Reporter | ||
Comment 4•9 years ago
|
||
This is the best I could do using an iframe: https://github.com/julienw/app-system-messages/tree/using-iframe I'm not really satisfied because not all system messages happen in the iframe: "notification" still happens in the main window because we want to launch the full app when we click the notification. Also because of this there are cases where the notification "click" action is handled in the iframe ("onclick"), and cases where it's handled in the main view (system message). We could probably fix this using cross-frame communication though. This is not _really_ satisfying but this kind of works for "sms-received". And I'm not sure for activities either...
Comment 5•9 years ago
|
||
Comment on attachment 8607957 [details]
GitHub Pull Request URL (WIP)
The idea is fine, but just like julien said we might have some problem if we want to migrate it to threads library because system message api should not possible to work on worker. Spawning a iframe for window thread might solve this problem but I'm not sure if all the system messages could work correctly on the thread we spawned. We might need more experiment for each case.
About the separate server/client, I understand it's reasonable to put part of system message part into server side(although I'm not sure if it's suitable to apply threads lib for system message case). But for the client side maybe we can absorb into activity handler?
Attachment #8607957 -
Flags: feedback?(schung) → feedback+
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → azasypkin
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8607957 [details] GitHub Pull Request URL (WIP) Moved to bug 1168441.
Attachment #8607957 -
Attachment is obsolete: true
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8612313 [details]
[gaia] azasypkin:bug-1162028-conversation-html > mozilla-b2g:master
Hey guys,
I'm not really confident about doing much work related to navigation stuff until we incorporate gaia-navigator, so I'm trying first very simple patch that nonetheless does the job we need.
How do you feel if just introduce small monkey patch (see 3rd commit only)?
Here are some shortcuts to verify in browser:
* app://sms.gaiamobile.org/views/conversation - New Message view;
* app://sms.gaiamobile.org/views/conversation#composer - New Message view
* app://sms.gaiamobile.org/views/conversation#thread&id=1 Conversation view
Thanks!
Attachment #8612313 -
Flags: feedback?(schung)
Attachment #8612313 -
Flags: feedback?(felash)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•9 years ago
|
||
[p=3] for 2.2S14 includes: * Extract conversation.html; * Prefetch threads/thread required by ConversationView; * Verify/fix navigation to work with non-default panel.
Blocks: sms-sprint-2.2S14
Summary: [Messages][New Gaia Architecture] extract conversation/index.html → [Messages][NG] Extract views/conversation/conversation.html
Whiteboard: [p=1] → [p(2.2S13)=1][p=3]
Assignee | ||
Updated•9 years ago
|
Target Milestone: --- → 2.2 S14 (12june)
Comment 10•9 years ago
|
||
Comment on attachment 8612313 [details]
[gaia] azasypkin:bug-1162028-conversation-html > mozilla-b2g:master
I think the extraction part looks fine, will review the activity part in another patch later.
Attachment #8612313 -
Flags: feedback?(schung) → feedback+
Assignee | ||
Comment 11•9 years ago
|
||
Hey guys, Here is one more variation of the possible solution for this bug so that you can compare and choose: Inbox and Conversation have their own startup.js files now that 1. allows to reference more required files in index file directly (especially good for separated Conversation view) instead of lazy loading them and 2. to not lazy load redundant files (especially good for separated Inbox view); I was trying to somehow reuse view-specific startup files in the main startup, but it brought even more mess there, would be happy to hear your suggestions! Thanks!
Attachment #8615131 -
Flags: feedback?(schung)
Attachment #8615131 -
Flags: feedback?(felash)
Assignee | ||
Comment 12•9 years ago
|
||
Also just in case you want to try to navigate between views or run it on the device, you can use my experimental branch with hackish navigation.js shim and updated manifest [1]. * System messages don't work there yet; * Thread less drafts aren't supported (we don't call Drafts.request in handleDraft). [1] https://github.com/azasypkin/gaia/tree/bug-xxx-nga-experiments
Reporter | ||
Updated•9 years ago
|
Blocks: messages-nga
Reporter | ||
Updated•9 years ago
|
Attachment #8612313 -
Flags: feedback?(felash)
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8615131 [details]
GitHub pull request URL (separated startup files)
I don't see much downsides, only upsides. So let's do this.
I left answers to your comments, and some additional comments from my own mind :)
Attachment #8615131 -
Flags: feedback?(felash) → feedback+
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8615131 [details]
GitHub pull request URL (separated startup files)
I've added unit tests, added fixes according to Julien's feedback, updated build script and fixed some more nits. So should be ready for the first round of review :)
There is still unresolved question regarding to string vs number parameters passed to Navigation, but we can discuss this on GitHub.
Thanks!
Attachment #8615131 -
Flags: review?(schung)
Attachment #8615131 -
Flags: review?(felash)
Attachment #8615131 -
Flags: feedback?(schung)
Reporter | ||
Comment 15•9 years ago
|
||
Comment on attachment 8615131 [details]
GitHub pull request URL (separated startup files)
Left answers and nits + an issue I see with the patch.
Please request review once you're ready for the (hopefully :) ) last review !
Attachment #8615131 -
Flags: review?(felash)
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8615131 [details]
GitHub pull request URL (separated startup files)
Thanks for review and finding the issue!
I've fixed nits (hopefully I haven't missed any :)), issue and added integration test for the issue you've discovered.
Attachment #8615131 -
Flags: review?(felash)
Reporter | ||
Comment 17•9 years ago
|
||
Comment on attachment 8615131 [details]
GitHub pull request URL (separated startup files)
I think we should fix the found issue differently; I left 2 ideas on github.
I can review again once I arrive in Vancouver tomorrow, so feel free to ask review again !
Attachment #8615131 -
Flags: review?(felash)
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8615131 [details] GitHub pull request URL (separated startup files) (In reply to Julien Wajsberg [:julienw] from comment #17) > Comment on attachment 8615131 [details] > GitHub pull request URL (separated startup files) > > I think we should fix the found issue differently; I left 2 ideas on github. > > I can review again once I arrive in Vancouver tomorrow, so feel free to ask > review again ! Updated PR! Should be fine now :)
Attachment #8615131 -
Flags: review?(felash)
Assignee | ||
Updated•9 years ago
|
Attachment #8612313 -
Attachment is obsolete: true
Reporter | ||
Comment 19•9 years ago
|
||
Comment on attachment 8615131 [details]
GitHub pull request URL (separated startup files)
r=me, let's land this !
Steve, do you want to add an extra feedback on this?
Attachment #8615131 -
Flags: review?(felash) → review+
Comment 20•9 years ago
|
||
Comment on attachment 8615131 [details]
GitHub pull request URL (separated startup files)
Looks great! Sorry about the late review.
Attachment #8615131 -
Flags: review?(schung) → review+
Assignee | ||
Comment 21•9 years ago
|
||
Thanks for review, guys! Master: https://github.com/mozilla-b2g/gaia/commit/70c9beeb7406ccd5a11367d4dca01a0bba7b3b39
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: 2.2 S14 (12june) → NGA S3 (26Jun)
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Comment 22•9 years ago
|
||
As NGA Program Manager suggested, let's replace the NGA-X milestones with FxOS-Sx ones (more generic ones), once Bug 1174794 has already landed
Target Milestone: NGA S3 (26Jun) → FxOS-S1 (26Jun)
You need to log in
before you can comment on or make changes to this bug.
Description
•