Closed Bug 1179628 Opened 4 years ago Closed 3 years ago

[Messages][NG] Lay out Settings service structure

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: steveck, Unassigned)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file)

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!
No longer blocks: sms-sprint-2.2S14, 1169576
Flags: needinfo?(felash)
Flags: needinfo?(azasypkin)
Flags: needinfo?(felash)
Blocks: 1169573
No longer depends on: 1169573, 1169543, 1171865
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 :)
Flags: needinfo?(azasypkin) → needinfo?(felash)
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...
Flags: needinfo?(felash)
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?
Depends on: 1180593
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
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.
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 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 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!
Attachment #8662915 - Flags: feedback?(felash)
Attachment #8662915 - Flags: feedback?(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!
Attachment #8662915 - Flags: feedback?(azasypkin) → feedback+
Assignee: nobody → schung
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 :)
Attachment #8662915 - Flags: feedback?(felash) → feedback+
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!
Attachment #8662915 - Flags: review?(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!
Attachment #8662915 - Flags: review?(azasypkin)
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.
Attachment #8662915 - Flags: review?(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!
Attachment #8662915 - Flags: review?(azasypkin)
Assignee: schung → nobody
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.