1162028 - [Messages][NG] Extract views/conversation/conversation.html

Status: RESOLVED FIXED

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

Comment 1

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub Pull Request URL (WIP)

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?

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8607957 [details] [review]
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...

Comment 3

[Julien Wajsberg [:julienw]]

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).

Comment 4

[Julien Wajsberg [:julienw]]

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

[Steve Chung [:steveck]]

Comment on attachment 8607957 [details] [review]
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?

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8607957 [details] [review]
GitHub Pull Request URL (WIP)

Moved to bug 1168441.

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1162028-conversation-html > mozilla-b2g:master

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8612313 [details] [review]
[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!

Comment 9

[Oleg Zasypkin [:azasypkin]]

[p=3] for 2.2S14 includes:

* Extract conversation.html;
* Prefetch threads/thread required by ConversationView;
* Verify/fix navigation to work with non-default panel.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8612313 [details] [review]
[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.

Comment 11

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (separated startup files)

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!

Comment 12

[Oleg Zasypkin [:azasypkin]]

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

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8615131 [details] [review]
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 :)

Comment 14

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8615131 [details] [review]
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!

Comment 15

[Julien Wajsberg [:julienw]]

Comment on attachment 8615131 [details] [review]
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 !

Comment 16

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8615131 [details] [review]
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.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8615131 [details] [review]
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 !

Comment 18

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8615131 [details] [review]
GitHub pull request URL (separated startup files)

(In reply to Julien Wajsberg [:julienw] from comment #17)
> Comment on attachment 8615131 [details] [review]
> 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 :)

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 8615131 [details] [review]
GitHub pull request URL (separated startup files)

r=me, let's land this !

Steve, do you want to add an extra feedback on this?

Comment 20

[Steve Chung [:steveck]]

Comment on attachment 8615131 [details] [review]
GitHub pull request URL (separated startup files)

Looks great! Sorry about the late review.

Comment 21

[Oleg Zasypkin [:azasypkin]]

Thanks for review, guys!

Master: https://github.com/mozilla-b2g/gaia/commit/70c9beeb7406ccd5a11367d4dca01a0bba7b3b39

Comment 22

[Maria Angeles Oteo (:oteo)]

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