1201016 - [Messages][NG] Migrate the current Message manager event handling to NGA

Status: RESOLVED WONTFIX

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

Description

[Steve Chung [:steveck]]

After message shim layout landed and connection is established, we can start to migrate the function in message manager to messaging/shim client. So there will be a window - iframe connection for each view. For the previous design, we put the client script in the service folder, but it seems not really fit this requirement for inbox/conversation view, because the method set would be completely different in different view, and we already have mozMobileShim client in shim folder for messaging service.

Here is some thought about the shim client for view: Create separate shim script under view/js, not sure if you prefer the duplicated name "moz_mobile_message_shim_client.js" as well. Thee we'll have duplicate event handling in script(or you want to extract the event handling logic into another script). I think the message shim might be the only exception that the views both require service/shim but the method set is different(maybe conversation service as well because we may need it in conversation view to update conversationSummaryInfo). It might be more reasonable to have separated client than single client besides the service/shim. Any thought?

Comment 1

[Steve Chung [:steveck]]

Per some discussion with Oleg, we decided to implement a layout for message event handling in this sprint. Here is the brief idea:
 - In inbox view: The conversation client should handle the 'thread' event from conversation service instead of message event from message shim. Since conversation summary is not ready yet, we might connect the conversation client with message shim directly with message event to replace the message manager first.

 - In conversation view: We reuse the current messaging client that could connect to shim.

In this patch we also need to clone the sms/mms properly for avoiding the data clone issue, but I wonder the necessity if we use sync dispatchEvent method in bridge since it's not using postMessage for sending the message and no need to serialize the message.

Comment 2

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #1)
> In this patch we also need to clone the sms/mms properly for avoiding the
> data clone issue, but I wonder the necessity if we use sync dispatchEvent
> method in bridge since it's not using postMessage for sending the message
> and no need to serialize the message.

Yeah, it will be true for MozMobileMessageShim(iframe) -> ConversationClient(window), but I think we'll still need it for MozMobileMessageShim(iframe) -> MessagingService(SharedWorker) -> MessagingClient(window)?

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:new-message-event-handling > mozilla-b2g:master

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8659839 [details] [review]
[gaia] steveck-chung:new-message-event-handling > mozilla-b2g:master

First draft about using the messaging client for event listening(and event handler removal). It's not polished yet but workable for real device and browser. Please let me know if you have better idea about the event handing pattern, thanks!

Comment 5

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8659839 [details] [review]
[gaia] steveck-chung:new-message-event-handling > mozilla-b2g:master

In general, idea with registering MessagingClient in MessagingService once it's initialized looks good to me. I've just left a few concerns at GitHub.

Thanks!

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8659839 [details] [review]
[gaia] steveck-chung:new-message-event-handling > mozilla-b2g:master

left some questions and remarks

Comment 7

[Steve Chung [:steveck]]

Add a commit for inbox view event handling. Since it requires to have bridge and other related scripts in start up call path, I also did some profiling for it. The time for visuallyLoaded will increase about 50~60ms and memory will increase around 1 MB. The increasing of the load time is expected by not sure why the memory increased as well... Oleg, did you also have the similar tendency from your profiling in bug 1198266, and do you think maybe it's better to wait for your patch since this patch will have lots of conflicts with patch in bug 1198266?

Comment 8

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #7)
> The time for visuallyLoaded will increase about 50~60ms and memory
> will increase around 1 MB. The increasing of the load time is expected by
> not sure why the memory increased as well... Oleg, did you also have the
> similar tendency from your profiling in bug 1198266.

I haven't looked into memory consumption closely, but yeah I think I've seen memory increase... Maybe now that "Record Allocations" option is available in WebIDE (and just checked it doesn't crash app anymore like in bug 1197257), we can check what exactly occupies our memory.

> and do you think maybe it's better to wait for your patch since this patch
> will have lots of conflicts with patch in bug 1198266?

It depends on how much time it will take for bug 1198266, maybe we'll never land it :D So up to you, I'm okay if I have to handle conflicts with your patch ;)

(Removing likely wrong ni? to azaaza :D)

Thanks!

Comment 9

[Julien Wajsberg [:julienw]]

p=1 to give feedback.

Comment 10

[Steve Chung [:steveck]]

Hey Julien,
Since you cancelled the feedback few weeks ago before we giving the 1 point for feedback, do you have any other comments about the patch, or I should just continue the patch based on current status?

Comment 11

[Julien Wajsberg [:julienw]]

My only concern is that we need to understand properly which events we need in the view. Maybe the ones you defined are too precise for ConversationService.

Comment 12

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 13

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(