1169573 - [Messages][NG] Lay out Messaging service structure

Status: RESOLVED FIXED

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

Description

[Oleg Zasypkin [:azasypkin]]

Based on MessagingService spec from bug 1169543, we'd like to create basic MessagingService interface here.

Patch for this bug should contain service/client definition without real method implementation (just stub methods throwing "Not Implemented" exception for now). As the first step we may need only methods required by InboxView.

MessagingService will look like a wrapper service around mozMobileMessage API and will live in main context or separate iframe. This service will be used by other services that need access mozMobileMessage API.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:new-message-messaging-service-layout > mozilla-b2g:master

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8613473 [details] [review]
[gaia] steveck-chung:new-message-messaging-service-layout > mozilla-b2g:master

Hey,

It's the first version of the messaging service layout with service/client js and html markup(if we need it for iframe thread). It's almost the same methods export from message manager and I know you might want to split it further. I can find a good reason why we need to separate these methods into two services but we can discuss in the bug, thanks!

Comment 3

[Julien Wajsberg [:julienw]]

I'll defer my feedback until you give your feedback in bug 1169543 so that you understand the main logic I have in mind :)

Comment 4

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8613473 [details] [review]
[gaia] steveck-chung:new-message-messaging-service-layout > mozilla-b2g:master

Hey Steve,

Since we agreed on MobileMessagingShim in bug 1169543, would you mind create separate bug and PR for the shim only?

Thanks!

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8613473 [details] [review]
[gaia] steveck-chung:new-message-messaging-service-layout > mozilla-b2g:master

Let's wait for bug 1171865 before moving forward here :)

Comment 6

[Steve Chung [:steveck]]

Revisit the messaging service part again with some thoughts, because it's related to the settings(multi sim part):

- Do we really need to make messaging methods (sending related functionality) into service, or it could be simply a controller per views(conversation and new message in the future). If it's a service, that means we need to make settings into a service as well.

- Following by the 1st question, do we want to move settings into service? The current settings maintains some data that need to be shared between views, but mostly are fixed value that shouldn't be changed after initialize. The only part that might be chargeable is serviceId, but we can query the these data in runtime instead of caching in settings. So actually we don't have to set and data into service that need to be shared between views. Once we can split the readAhead part from settings, settings could only be utilized in conversation/new message view(I don't think we need most of information from settings in inbox view and try to remove the dependency, but maybe I'm wrong).

Anyway just want to know your POV toward the settings and messaging since the controller not really need to be service, and both way seems work for messaging and settings(but if we decide to make messaging as service, seem we can only make settings into service as well, unless we split serviceId part into another service).

Comment 7

[Julien Wajsberg [:julienw]]

Some rough thoughts:

* Settings: remember we do a lot of things at startup here, especially querying the settings database; it seems to be it would be a good thing to have it in a Shared Worker instead, so that it's queried only once. So independently from the choice for the messaging methods, it seems it's better to have it in a shared worker anyway.
  Can we access mozSettings from workers now? I don't think so, actually, so we'll also need a shim for mozSettings.

* now about your first question, I think it's better if the messaging service is a service in a worker. Remember it will also handle the events from the API and update the local database in the future. Now we could do that later if you think it's too much work for now.

Comment 8

[Oleg Zasypkin [:azasypkin]]

My two cents:

1. I tend to messaging service that will allow us to hide backend details from view controller, and make migration to our own DB easier in the future;

2. I was thinking about two ways of handling mozSettings:
 2.1 Wrap mozSettings into shim that can be consumed by other services as Julien says or;
 2.2 Wrap mozSettings into lib that is used by view controller and result is passed to service method as a additional parameter, but I don't have all cases in mind to be sure that it's feasible at all.

Comment 9

[Steve Chung [:steveck]]

Ok, just want to confirm that we will go with settings shim because we didn't discuss more detail how we deal with mozSettings. I thought you might have other idea for the settings so it's not set in the service readme first. But since settings API could still only live in main thread, I'm afraid it might not be able to improve the loading much as shim, unless it could be accessed in worker.

> Remember it will also handle the events from the API and update the local database in the future...
you mean the event broadcast from message shim here? yeah I think we could have a separate issue for storing/updating local DB when receiving event from shim in messaging service, but didn't we plan to wait for the the getPendingOperration API for building local DB, or you plan to store in local first and still use gecko DB before API ready?

Comment 10

[Steve Chung [:steveck]]

Attachment: Link to github

It's a quick prototype for the messaging service with 3 methods related to message sending. mozSettings and local DB issues are not addressed in this patch, just want to know your thought about the messaging service and what we should return from service.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8628163 [details] [review]
Link to github

nothing much to say, this looks good.

I answered some question and added some more comments on github.

Comment 12

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8628163 [details] [review]
Link to github

Looks good, added some nits at GitHub.

I'm mostly worried about DOMParser-in-worker, it wasn't available in my naive preliminary test (commented at GitHub), hope that I'm wrong :)

Thanks!

Comment 13

[Steve Chung [:steveck]]

Thanks for the feedbacks!
I just updated the patch, but I can not figure out a better way to write a test for a worker. Maybe we can use real service/shim/bridge all the way for testing a real service method, but it's impossible to verify for each stage, especially in worker. Maybe you any good idea about writing test for worker individually?

Comment 14

[Julien Wajsberg [:julienw]]

Wondering why you can't unit test the file just by loading it as usual ? You'll need to import the BridgeServiceMixin as well, and then make importScripts an empty function so that it doesn't interfere.
You'll need also to stub a mock bridge library.

What do you think?

Comment 15

[Oleg Zasypkin [:azasypkin]]

Yeah, like Julien said, I also remember we had unit test for shared worker script [1] some time ago.

[1] https://github.com/mozilla-b2g/gaia/blob/f9d18900d52e2d460a949cd9b226edccd926b775/apps/sms/test/unit/inter_instance_shared_worker_test.js

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8628163 [details] [review]
Link to github

Thanks for the suggestions, I add the unit test that treat it as the main context for testing. Also I change the interface a little bit and will add some comments on github.

Comment 17

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8628163 [details] [review]
Link to github

Hey Steve,

I've left some nits and questions at GitHub, I haven't looked into tests yet, but looks like there are failing because of the fact that they don't respect brand new threads.js "listen" method. + JSHINT job is complaining as well.

Thanks!

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8628163 [details] [review]
Link to github

Oleg left a lot of comments already :) I added a few more.

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8628163 [details] [review]
Link to github

Update the patch and verifying the result about DOMrequest test, will update soon.

Comment 20

[Steve Chung [:steveck]]

Hi Oleg,

About the return value from bridge service, it will return the result directly instead of DOMrequest, so it will execute the DOMRequest's then and return final result in service method.

BTW I found one thing that the data return in resolve/reject is not the event like what we get in onsuccess/onerror callback, it would the the result or error directly in DOMRequest's then. I talked with Bevis and seems all the DOMRequest will return the result directly in promise instead of event wrapping. So I also update the patch because we can simply our return promise without digging the events for data.

Comment 21

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #20)
> Hi Oleg,
> 
> About the return value from bridge service, it will return the result
> directly instead of DOMrequest, so it will execute the DOMRequest's then and
> return final result in service method.
> 
> BTW I found one thing that the data return in resolve/reject is not the
> event like what we get in onsuccess/onerror callback, it would the the
> result or error directly in DOMRequest's then. I talked with Bevis and seems
> all the DOMRequest will return the result directly in promise instead of
> event wrapping. So I also update the patch because we can simply our return
> promise without digging the events for data.

Yeah, thanks for confirming, that is what I expected and that's actually good :)

Comment 22

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8628163 [details] [review]
Link to github

Looks good! Just few more nits left at GitHub.

Thanks!

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8628163 [details] [review]
Link to github

I'll trust Oleg's review here.

My only question is: should we try to use this code in the current application ? What do you think ?

Maybe it's not possible yet though :)

Comment 24

[Oleg Zasypkin [:azasypkin]]

JFR, as per Wilson we already can use BroadcastChannelAPI as handshake mechanism with the following snippet (haven't tested it yet though):

Declaring service inside iframe:
serivce('mozMobileMessageShim').listen(new BroadcastChannel('shim-channel'));

Declaring client inside MessagingService:
client('mozMobileMessageShim', new BroadcastChannel('shim-channel'));

Comment 25

[Steve Chung [:steveck]]

Maybe we can (or we should) tried it in bug 1169576?

Comment 26

[Steve Chung [:steveck]]

(In reply to Steve Chung [:steveck] from comment #25)
> Maybe we can (or we should) tried it in bug 1169576?

Hmm, bug 1169576 will not need messaging service(it only need message API shim at most in conversation service). Will create a bug for conversation view to migrate messaging service methods.

Comment 27

[Steve Chung [:steveck]]

Ok, create bug 1184865 to verify our thoughts in current code!

In master: https://github.com/mozilla-b2g/gaia/commit/4a4e885c5ee1574bfef42df0d6fa74069387b5f2