1180592 - [Messages][NG] mozMobileConnections shim Implementation

Status: RESOLVED FIXED

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

Description

[Steve Chung [:steveck]]

Based on the discussion in Bug 1179628, we decide to move settings into service. Considering the mozMobileConnections could not work in worker, having a shim for mozMobileConnections API is necessary in order to make settings service runnable.

Comment 1

[Steve Chung [:steveck]]

I'm a little bit confused that how many methods we should exposed for mozMobileConnections shim. For the messaging/Settings service, the necessary part is only the connection number that could classify whether the device is multi-sim or not. Another part is for switchMmsSimHandler that need specific connection data and connection event listener. It's still doable that we design API for this case such as getData with serviceId and startEventLister for mozMobileConnections shim, but it seems not real make sense for me to add much more complicate logic in shim layer. I'm thinking of moving most of switchMmsSimHandler in SettingsClient and only the setMmsSimServiceId will access settings service. What do you think if we only have connection number in the shim and leave other connection API in settings client side?

Comment 2

[Julien Wajsberg [:julienw]]

So from what I see, for mozMobileConnections, we need to:

* know whether the device is dual sim, and we know this with the service ids. Currently we use mozMobileConnections but maybe we should use mozIcc instead as this could be simpler.
* switch the service id, for this we need to know the current data state, etc. I know the initial design was that we were just forwarding the mozAPI from the shim, without any change to the API, but as far as the service ID switching is concerned, maybe it's easier to have all the switch behavior in the shim. I'm especially afraid of races, and I think it makes things more complicated.
  After all this is a fairly low-level operation and I don't think it would be bad to do this in the shim.

What do you think?

Comment 3

[Steve Chung [:steveck]]

It's good that we can rethink about how we utilize shim. I was thinking about wrapping everything inside the SettingsClient.switchMmsSimHandler, and inside the switchMmsSimHandler would be:

switchMmsSimHandler(iccId) {
  SettingsService.getServiceIdByIccId(iccId).then((id) => {
    var conn = window.navigator.mozMobileConnections[id];
    return new Promise(function(resolve, reject) {
      if (conn) {
        if (conn.data.state === 'registered') {
          resolve();
        } else {
          conn.addEventListener('datachange', function onDataChange() {
            if (conn.data.state === 'registered') {
              conn.removeEventListener('datachange', onDataChange);
              resolve();
            }
          });
          SettingsService.setMmsSimServiceId(id);
        }
      } else {
        reject('Invalid connection');
      }
    }.bind(this));
  })
}

But it still need window->service(get service id)->window->service(set service id)->shim(set service id to settings) and wait for the data change in window context.

If we accept to move all the switching logic inside the shim, the call path would be much simpler. Will create a WIP to see if it's what you expected.

Comment 4

[Steve Chung [:steveck]]

WIP for conns shim: https://github.com/steveck-chung/gaia/commit/859fe2d2144c418eaba1ba4627b5164d1a71a53a
Maybe I could also add first version of the settings shim and make the overall sim switch scenario works.

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #3)
> It's good that we can rethink about how we utilize shim. I was thinking
> about wrapping everything inside the SettingsClient.switchMmsSimHandler, and
> inside the switchMmsSimHandler would be:
> 
> switchMmsSimHandler(iccId) {
>   SettingsService.getServiceIdByIccId(iccId).then((id) => {
>     var conn = window.navigator.mozMobileConnections[id];

I see you'd need access to mozMobileConnections here :)



(In reply to Steve Chung [:steveck] from comment #4)
> WIP for conns shim:
> https://github.com/steveck-chung/gaia/commit/
> 859fe2d2144c418eaba1ba4627b5164d1a71a53a

Yeah I think this looks good.

> Maybe I could also add first version of the settings shim and make the
> overall sim switch scenario works.

why not !

Comment 6

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #1)
> it seems not real make sense for me to add much more complicate logic in shim layer.

Agree.

(In reply to Julien Wajsberg [:julienw] from comment #2)
> So from what I see, for mozMobileConnections, we need to:
> 
> * know whether the device is dual sim, and we know this with the service
> ids. Currently we use mozMobileConnections but maybe we should use mozIcc
> instead as this could be simpler.

Yeah, IIRC this is what multi_sim_action_button.js uses internally, this originates second problem since this file is in shared and used by other app and we can't just switch it to our services (for mozIcc and mozSettings) and async based methods. Do we have plan for this? 

> * switch the service id, for this we need to know the current data state,
> etc. I know the initial design was that we were just forwarding the mozAPI
> from the shim, without any change to the API, but as far as the service ID
> switching is concerned, maybe it's easier to have all the switch behavior in
> the shim. I'm especially afraid of races, and I think it makes things more
> complicated.
>   After all this is a fairly low-level operation and I don't think it would
> be bad to do this in the shim.
> 
> What do you think?

(In reply to Julien Wajsberg [:julienw] from comment #5)
> 
> (In reply to Steve Chung [:steveck] from comment #4)
> > WIP for conns shim:
> > https://github.com/steveck-chung/gaia/commit/
> > 859fe2d2144c418eaba1ba4627b5164d1a71a53a
> 
> Yeah I think this looks good.

Yep, approach looks good to me as well. And I agree with Julien about mozIcc. IIRC this is what multi_sim_action_button.js uses internally, but this reminds me about one more problem: since this file is in shared and used by other app and we can't just switch it to our services (I see it uses mozIcc, mozSettings and mozTelephony) inside and async based methods.

Do we have plan for this?

Comment 7

[Oleg Zasypkin [:azasypkin]]

Sorry for weird comment - I've messed up "replies" :)

Comment 8

[Julien Wajsberg [:julienw]]

> Do we have plan for this?

Good remark !

Yet my lazy solution is: we don't need a plan.
The button lives in a window, so we'll have access to the API.

The shim is only _really_ important for the API we need to access in the Worker.

Comment 9

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> 
> The shim is only _really_ important for the API we need to access in the
> Worker.

I tend to agree with this thought: We can access the API directly while we're just in the window context. Shim is for solving the API in Worker problem, not necessary to make the original workable method in window context to become more complicate to use.

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:new-message-conns-shim > mozilla-b2g:master

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8654800 [details] [review]
[gaia] steveck-chung:new-message-conns-shim > mozilla-b2g:master

Here is the new version of the mozMobileConns shim and workable for sim switch function on device(you can tried with sending mms to the non-data-active sim case). The client side only provide the switch function to the view, and in the client will connect to shim directly. Actually there's no other service that really need to switch connection inside. The only necessary part is the getServiceIdByIccId that resendMessage in messaging service. Not sure if you think the window -> iframe connection is impractical pattern. There is some other thought:
(1)Make the sim switch function into utils or conversation view helper, so only getServiceIdByIccId will be in the shim for resend message only and no need to expose to the view. Or even we can deal the getServiceIdByIccId in messaging client side directly(and same for the sim number in mozIccManager).
(2) We still keep the window -> service -> shim pattern, so let's add a service for mozMobileConns here.

I think we might gain some performance for (1) If we can load less script in shim host iframe, if we don't really have to use API with shim pattern if it's not necessary to be called in service, wdyt?

Comment 12

[Julien Wajsberg [:julienw]]

If we think more globally, the switchSim API are part of the 2 flows "retrieveMMS" and "sendMMS". Longer term switchSim shouldn't be called directly by the view, but rather by the MessagingService, as part of these flows.

I don't mind that it's  called by the view for now, but then it should still be in the separate window. We don't need to add a new mozMobileConns service though, because longterm this won't be called by the view directly.

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8654800 [details] [review]
[gaia] steveck-chung:new-message-conns-shim > mozilla-b2g:master

I didn't try it but this looks good from a quick view. I don't have much things to add :)

Comment 14

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8654800 [details] [review]
[gaia] steveck-chung:new-message-conns-shim > mozilla-b2g:master

Shim part looks good, but couldn't try PR as it seems client is not checked in :)

So just few nits and questions at github.

Thanks!

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8654800 [details] [review]
[gaia] steveck-chung:new-message-conns-shim > mozilla-b2g:master

Test part has been added, and with some mocks for desktop mode.

Comment 16

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8654800 [details] [review]
[gaia] steveck-chung:new-message-conns-shim > mozilla-b2g:master

It looks good to me, just few nits and some minor questions.

Thanks!

Comment 17

[Steve Chung [:steveck]]

Thanks for the review!

In master: https://github.com/mozilla-b2g/gaia/commit/027d26bab9351aff533b236bfdb147f0734a660c