1179628 - [Messages][NG] Lay out Settings service structure

Status: RESOLVED WONTFIX

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

Description

[Steve Chung [:steveck]]

Since Settings might be used in certain service like messaging service, we'd need to have a service for settings and create basic SettingsService interface here.

It's likely that we will need to treat this SettingsService as a shim because of mozSettings API and even mozMobileConnections, so maybe a settings service with mozSettings shim and mozMobileConnections shim should be enough, but in fact there's still other APIs in the Settings like mozL10n/mozIccManager in shared MobileOperator. So I think it's time to revisit how we deal with the Settings service. I might keep the original idea that create settings service and mozSettings/mozMobileConnections shim, and split getSimNameByIccId/getOperatorByIccId to other controllers/views. Another thought is we simply move the entire settings module into shim, and we don't have to worry about the API since everything is still running on iframe context, but you might not prefer this idea. Anyway any idea is welcome!

Comment 1

[Oleg Zasypkin [:azasypkin]]

Just some ideas:

> I might keep the original idea that create settings service and
> mozSettings/mozMobileConnections shim, and split
> getSimNameByIccId/getOperatorByIccId to other controllers/views.

Yeah, that sounds like a good mid-term plan for me :) IMO "mozL10n" should not be in a service, and luckily it's only used by Settings.getSimNameByIccId that is only used in one place in ActivityHandler - so I'd rather move this method to ActivityHandler for now. Same 1-to-1 usage for getOperatorByIccId (that uses MobileOperator->mozIccManager that is out of our control right now).

> Another thought is we simply move the entire settings module into shim, 
> and we don't have to worry about the API since everything is still running
> on iframe context, but you might not prefer this idea. Anyway any idea is welcome!

I think this also acceptable as a short-term plan, so that we move faster with making up our NGA-Inbox. And then split SettingsShim to SettingService, mozSettingsShim, mozMobileConnectionShim and etc... Not sure what do you guys think.

Looks like Julien mistakenly removed ni?, so returning it back :)

Comment 2

[Julien Wajsberg [:julienw]]

aaah I knew I had something else to answer :D

Given Settings needs to access to a lot of API, I'm fine if this is a shim living in a frame... but I admit I don't know how this really works with the new threads.js...

Now, we can talk about the purpose of our Settings object:

1. synchronous access to some settings (because we cache the value) (and this was before promises existed)
2. caching some values in the Settings object because access was slow..

About 1: if settings moves to a service we don't have this refinement anymore.

About 2: maybe we don't need this anymore now? I think the first access to an API could be slow but that the subsequent calls are fast... but then we'd have the same issue with our Settings object anyway.

Therefore here is a suggestion: we could make a new Settings object that would make it easy to access some settings using promises. But this is really useful if we use it in views only -- I think we need it in Services too, right?

We could also split the current settings.js in a part suitable for services and a part suitable for views.


So, sorry, not really answers, more questions...

Comment 3

[Steve Chung [:steveck]]

It might be a good time to think whether we should put everything into settings service. We can definitely move entire settings into service and access via promise-base method, but considering we access settings property frequently in everywhere. It might be painful to rewrite every synchronous access into promise, especially most of the properties should not be changeable in app runtime. So I'm fine to split settings for view only and something for service. I assume:
- serviceId/sim related method for settings service, since the active service id is changeable during runtime and we might need to cache whole service ID array in service. It could be simUtilsService with mozSettingsShim and mozMobileConnectionShim(or simply simUtilsService running with iframe)
- Settings controller for view with init function to query and cache the settings value. We can access the settings API directly or by mozSettingsShim.

But firstly I would prefer to split the getSimNameByIccId/getOperatorByIccId into other view because they are not suitable to be in service, wdyt?

Comment 4

[Steve Chung [:steveck]]

Create:
- Bug 1180593 for removing getSimNameByIccId/getOperatorByIccId from settings
- Bug 1180591 for mozSettings shim Implementation 
- Bug 1180592 for mozMobileConnections shim Implementation

So removing 1 point form sms-sprint-FxOS-S3

Comment 5

[Steve Chung [:steveck]]

Update some more thought about the settings service: I might not create a settings service :p, and use settings client to connect with shim directly. Since there's not much data really need to be share via service.

Here are the methods that we might have in settings client:
- hasSeveralSim, and it might be replaced with navigator.mozIccManager.iccIds.length directly and I thinks it's meaningless to create a mozIccManager only for this purpose. I might move the sim number checking from messaging service to messaging client and handle the send option in messaging client side.

- mmsSizeLimitation: will call this method for mms size limit in conversation init and cache in the view. For compose it will set as a parameter in compose.init(), so compose don't need to have any dependency with settings. Activity handler will need it but we can access the settings DB instead of cache it in somewhere.

- maxConcatenatedMessages: same like mmsSizeLimitation, but it even doesn't need to cache in conversation view because there's no other place will need it.

- supportEmailRecipient: should be able to be removed safely?

- setReadAheadThreadRetrieval: it's only need for inbox view

The rest of part that not mentioned seems not needed in new settings client.

Comment 6

[Julien Wajsberg [:julienw]]

Why not, simpler is better. Let's try this and we'll see if we need a service later on.

Having a service that simply proxifies the shim is not useful.

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8662915 [details] [review]
[gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

It's a very early version of the settings client layout. I finally remove the readAhead because I don't think it's really necessary to have init settings client in inbox only for this method. The settings client for conversation view and compose is working but the mms limitation in activity handler is not ready yet.

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 8662915 [details] [review]
[gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

Patch updated with activity handler using settings client, any early feedback is welcome!

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8662915 [details] [review]
[gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

I think it's good!

I only left one question/concern about the new way of handling activity data.

Thanks!

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8662915 [details] [review]
[gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

looks good, just left one simple comment about the shim API.

also you'll need to rebase of course :)

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8662915 [details] [review]
[gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

Hi Oleg, I added the unit test part for settings client. There's additional changes that moves hasSeveralSim method to Utils because it's not related to settings actually. Please let me know if you have different thought on this change, thanks!

Comment 13

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8662915 [details] [review]
[gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

Hey Steve, PR looks really good! There is one small issue with settings client unit tests + I've left several nits/questions, most of them are optional though.

Please, r? whenever you're ready.

Thanks!

Comment 14

[Steve Chung [:steveck]]

Comment on attachment 8662915 [details] [review]
[gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

Thanks for the suggestions! I applied most of them but I can't figure out unit test for the changes in startup, maybe you have some brilliant idea for that :)
BTW I'll create another follow up for removing Settings.hasSeveralSim method in messaging service and we can remove the entire settings after that.

Comment 15

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8662915 [details] [review]
[gaia] steveck-chung:new-message-settings-client > mozilla-b2g:master

Looks good and seems to work well on the device, but I left few comments at GitHub about broken split view mode and some racy exception from logcat.

Also could you please rebase your PR on the latest master and try to run integration tests locally so that even tests disabled on Treeherder are executed (mostly deferring r+ because of this to make sure we are safe :))?

Thanks!

Comment 16

[Julien Wajsberg [:julienw]]

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

Comment 17

[Julien Wajsberg [:julienw]]

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