Closed Bug 1193587 Opened 5 years ago Closed 5 years ago

Implement late customization (In tree)

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.5+)

RESOLVED FIXED
FxOS-S9 (16Oct)
feature-b2g 2.5+

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file)

Implementation bug for the system app pieces of bug 1172922. 
This should be a self-contained script that facilitates late customization (fetch and install apps from marketplace on first-time use), to either be added to system app directly or possibly via an add-on.
No longer blocks: 1172922
Blocks 2.5 meta for this feature.
feature-b2g: --- → 2.5+
Assignee: nobody → sfoster
Status update: I've push a WIP branch: https://github.com/sfoster/gaia/tree/late-customization-system
This is a simple proof-of-concept. Add a setting (e.g. to your build/config/custom-settings.json) like so: 
{
  "latecustomization.manifestUrl": "{manifestUrl}"
}
..where {manifestUrl} is a valid url that points to a json file like so: 

{
  "apps": [
    { "manifestUrl": "https://www.bzlite.com/manifest.webapp", "name": "bugzillalite" }
  ]
}

.. and flash (make reset-gaia). There's no error handling in there yet, and lots of potential error conditions. Like, what if the wifi is a captive portal, or if the hostname doesnt resolve? What if the user enables data - that will produce an 'online' event and trigger the install sequence - should we wait and see if they also connect to a wifi network first? And if so, wait how long? Currently we abort when the FTU concludes (ftudone event) 

As it stands, this could likely be fleshed out converted to an add-on, but it breaks cardinal rules by not informing the user what's going on. To implement the UX spec with the add-on approach, we'll need FTU changes to allow injection of new steps. 

If we bake in Late Customization and follow the UX spec, I think we could fetch manifestUrl and populate a list of apps in a screen in FTU provided we were careful to abort the XHR if it was still in-flight when closing the FTU. 

Is there a bug open for the prompt-less install we want here?
I updated and made a PR for this approach to make it easier to see what's going on. If you have an entry like so in your custom-settings.json:  
{ "latecustomization.manifestUrl": "https://marketplace-dev.allizom.org/api/v2/feed/shelves/310/" }

.. it will attempt to install the 6 apps listed there. Apps are now installed serially (you get a prompt for each.) I'm not yet handling the various error conditions. 

:fabrice: Do we have a bug open for the prompt-less install? I'm guessing we want another condition at/near https://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2516 to special-case the late-customization case somehow?
Flags: needinfo?(fabrice)
No I'm not aware of a bug for prompt-less install. How do you plan to let gecko know which manifestURL we should not prompt for? and how to distinguish the "late customization" installs from regular installs for the same manifest url? An other option is to have gaia to not prompt for installs that are triggered from by the ftu panel.
Flags: needinfo?(fabrice)
Depends on: 1208599
(In reply to [:fabrice] Fabrice Desré from comment #5)
> No I'm not aware of a bug for prompt-less install. How do you plan to let
> gecko know which manifestURL we should not prompt for? and how to
> distinguish the "late customization" installs from regular installs for the
> same manifest url? An other option is to have gaia to not prompt for
> installs that are triggered from by the ftu panel.

Update on this: If I register for mozChromeEvents soon enough in the system app, I can intercept the webapps-ask-install and manually dispatch the webapps-install-granted approving the install. I'm going down this path for now, but: 

1) We'll rely on listener registration order. I've not yet confirmed this will continue to work with the new add-on API. Either way this definitely needs to be done in the system app vs. the FTU. 

2) We'll need to be careful to close this hole back up and completely disable this listener and the late customization mechanism after first run - so we're not hitting this code path after FTU has been and gone.
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

Tim, Alberto, as potential reviewers could you take a quick look over this WIP patch and give me some high-level feedback? 

If you apply the patch and reset-gaia with a custom-settings.json something like: 
{
 "latecustomization.manifestUrl": "https://marketplace-dev.allizom.org/api/v2/feed/shelves/3/"
}

.. you should get a new "Free Apps" panel in the FTU with 3 apps listed if you made a network connection (and a message if not). The approach is: 
* attempt to get the late-customization manifest URL from settings in FTU - if we get one we're "enabled"
* wait for navigator.onLine and fetch the manifest - which is something like the existing operator shelf end-point and lists a small (I think we capped at 5?) number of apps the operator would like installed
* In system app, we add an early event listener for mozChromeEvent to allow us to intercept the install requests. 
* The FTU sends over the list of app manifest URLs we'll be installing. 
* The FTU triggers install(), the event is caught in System app and a approve event manually dispatched if the manifestURL is on our list.  
* In the FTU, the panel lists out the apps and toggles a pending state once install is complete. 

That's it. In principal, I believe this could be packages as a pre-installed add-on, as part of a customization. We need to patch to enable that though. Furthermore, I've not written unit or integration tests yet - if we land in Gaia I'll do that as per normal. If we're going down the add-on route, I need to make a different plan - clearly we need tests of some kind either way.
Attachment #8649983 - Flags: feedback?(timdream)
Attachment #8649983 - Flags: feedback?(apastor)
Summary: Implement late customization in system app → Implement late customization (In tree)
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

You should be able to leverage core.js and app.js etc to ensure your script starts before app_install_manager.js. <script> in HTML delays the launch time of System app.

My preference, instead, is not to rely on stopImmediatePropagation the event and the order of addEventListener calls. We will very likely change that in the future, so (thinking aloud here):

1. maybe have your script loaded by app_install_manager itself? and
2. maybe don't add another event listener but just put an early return block in the existing event handling function?

IMHO it's safer than rely on event listener order and stopImmediatePropagation.

PS I did not look at the script itself. I have no opinion on that since we already have IAC in place so its not going to be worse than that anyway.
Attachment #8649983 - Flags: feedback?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (OOO Oct 9-18; please ni? to queue) from comment #8)
> Comment on attachment 8649983 [details] [review]
> [gaia] sfoster:late-customization-system > mozilla-b2g:master
> 
> You should be able to leverage core.js and app.js etc to ensure your script
> starts before app_install_manager.js. <script> in HTML delays the launch
> time of System app.

yeah I should clarify, the current <script> and really loose coupling with system app is there with a mind towards making this an add-on. If we add this capability to the system app, then there would be no index.html changes and I'd use the bootstrap mechanism(s) to load and initialize the system app part.

> My preference, instead, is not to rely on stopImmediatePropagation the event
> and the order of addEventListener calls. We will very likely change that in
> the future, so (thinking aloud here):
> 
> 1. maybe have your script loaded by app_install_manager itself? and
> 2. maybe don't add another event listener but just put an early return block
> in the existing event handling function?

I've been sitting on the fence with this - integrate with the system (and FTU) app, or keep loosely coupled and ship as an add-on. I guess I'm starting to form an opinion: 

I'm wary of shipping a 2.5 feature as an add-on at this stage. ISTM that add-ons are still in their infancy in FxOS and this feature would get to blaze the trail in some ways - for how to ship meaningful and useful test coverage for a FxOS add-on, for localization, for enabling add-ons as part of a customization and maintaining a committed feature as an add-on. 

Going forward, I'm not keen on all the isFtuRunning checks we have dotted around already, and putting another check in this path for what will be a uncommon case isn't ideal. I'm sure there are alternatives - I'll have a think.
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

I think this is about ready for a first review. I gather Tim is on PTO, maybe Alberto and Kevin can fight about the system pieces. Fernando, if you could look at the FTU part. Both obviously need to be seen in context of the whole flow. 
To test it out, you'll need to include the following in your build/config/custom-settings.json

{
 "latecustomization.manifestURL": "https://marketplace-dev.allizom.org/api/v2/late-customization/?carrier=telenor&region=ca"
}

Sadly there's really no substitute for doing a full reset-gaia each time, as we need to not have these apps already installed, reset connections and FTU state, and pick up the new manifest datastore change in FTU. 

Following Tim's comments, I've gone ahead and added handling for the late/ftu customization case in AppInstallManager. I've got unit tests in there for the changes to FtuLauncher and AppInstallManager. Tests for both system and FTU side of Late Customization are underway.

Let me know if you want me to walk through it.
Attachment #8649983 - Flags: review?(kevingrandon)
Attachment #8649983 - Flags: review?(fernando.campo)
Attachment #8649983 - Flags: review?(apastor)
Attachment #8649983 - Flags: feedback?(apastor)
No longer depends on: 1208599
A couple clarifications on this as its up for review and feature-complete AFAIK. 

Wilfred: I'm using a navigator.mozSetting value (latecustomization.manifestURL) to tell me where to get the list of apps to install. This is a marketplace URL. If its not there, late customization is not enabled and skipped entirely, even if there is an active mobile connection and operator info. The assumption is that partner/operator will populate this setting in their build. Is that correct? We don't have any way to know from the device if this will be zero-rated, so the presence or absence of the setting seems like a simple way to determine if late customization is enabled. 

I'm talking with Marketplace folks about sending mcc & mnc in the request, as the short operator name and region name are surprisingly difficult to produce. 

In the PR I've got a 2nd setting value for latecustomization.operatorInfo. This is intended for testing either with a mock server, or to get a valid response from the Marketplace with a random SIM. But its not fully baked yet; I'm working on tests today and will see how that plays out.
Doh. Wilfred, see #comment 11
Flags: needinfo?(wmathanaraj)
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

Tiff, this is ready to get UX eyes on it. Let me know if I can help.
Attachment #8649983 - Flags: ui-review?(tshakespeare)
Updated the PR with some minor changes to improve disconnect/reconnection handling. Also, the 'latecustomization.operatorInfo' setting lets you enable late customization in the FTU by providing a dummy operator instead of your SIM's (or if you have no SIM). Your custom-settings.json should look something like:
{
 "latecustomization.manifestURL": "https://marketplace-dev.allizom.org/api/v2/late-customization/?carrier=telenor&region=ca",
 "latecustomization.operatorInfo": { "operator": "acme", "region": "ca" }
}

:mstriemer, is there a bug for accepting mcc & mnc in this request? 

Tiff, this should be working for you now.
Flags: needinfo?(mstriemer)
 Is this only set at build time or can this be set even after build time? what if at launch time this is not set and then later (eg. a month later) its set? is this possible?
Flags: needinfo?(wmathanaraj) → needinfo?(sfoster)
Depends on: 1213328
I filed bug 1213328 to update the Marketplace API and marked it as a blocker on here.
Flags: needinfo?(mstriemer)
(In reply to Wilfred Mathanaraj [:WDM] from comment #15)
>  Is this only set at build time or can this be set even after build time?
> what if at launch time this is not set and then later (eg. a month later)
> its set? is this possible?

The setting needs to be there when when the FTU runs. To avoid the install prompts we use the (system app) FTU launcher which detects the first-run. That means the running the FTU manually app (e.g. from the WebIDE or the 'launch FTE' button in settings) could trigger late customization, but you would get prompted for each (and this scenario is not tested)

I don't know how partners usually manage the builds on new devices. But you could certainly create a build from any version after this lands, then later (but before they've been turned on and the FTE has run), update the settings on those devices without needing to re-build. 

For my testing I usually just use the reset-gaia tool to create a new profile and flash gaia. But you can acually also flip the 'ftu.enabled' bit in the system app's window.asyncStorage, update any settings, stop/start b2g and the FTE will run again. I *believe* these are all things that are automatable, and we have python scripts that could accomplish this, I've not tried to script it though. 

Running through the FTE is somewhat destructive today. So running it again after a user has already run it and started using the device is probably not a good idea. In an update scenario, we would just want to point them at the Marketplace. We do run the FTU when an updated is detected - e.g from 2.1 > 2.2. Currently we only show the tutorial though and we would not be able to show the "free apps" panel. 

Hope that answers the question.
Flags: needinfo?(sfoster)
Attachment #8649983 - Flags: ui-review?(tshakespeare) → ui-review+
oops forgot my comments - sfoster is going to do a minor string update. We also noticed that the icon shown during FTE isn't being used as the app icon on the homescreen. Most likely it's because these aren't actually in Marketplace? 

Let's double check once everything is hooked up.

NI to Eric in case he was to look at it from the visual standpoint (check your email for screenshots). Thanks!
Flags: needinfo?(epang)
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

I left a comment about the location of FTU migration logic. If Alberto or perhaps Etienne leave an R+ without that change my feelings would not be hurt though.
Attachment #8649983 - Flags: review?(kevingrandon)
> I left a comment about the location of FTU migration logic. If Alberto or
> perhaps Etienne leave an R+ without that change my feelings would not be
> hurt though.

I updated the PR to have AppInstallManger just consult a Service.query('ftuCustomizationContains', manifesturl); This will return false correctly if FtuCustomization is never loaded, stopped or not started, or if the manifestUrl is not in the set of late customization apps. And only return true if it is.
Looks good to me Tif!  Thanks for flagging me.
Flags: needinfo?(epang)
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

I've updated the PR with the following changes: 
* Strings in FTU properly localization-ready. 

* The setting name has changed to better match the pattern I saw in common-settings.json. To test, you'll need something like the following values in custom-settings.json: 

"latecustomization.url": "https://marketplace-dev.allizom.org/api/v2/late-customization/",
 "latecustomization.operatorInfo": { "operator": "telenor", "region": "ca", "mcc": "238", "mnc": "02" }
}

latecustomization.operatorInfo is a test-only convenience, so you can emulate having a valid SIM. The mnc/mcc values there are bogus, when bug 1213328 this will likely need to be valid mnc/mcc values)

* The latecustomization.url setting has the prodn marketplace URL by default. The decision to have a default value (meaning FTU will always check this end-point if there's a SIM) is not yet final. 

* Service.query('ftuCustomizationContains, url) implemented. Removed all the other stuff from AppInstallManager. Also was able to simplify the flow substantially in FtuLateCustomization

* Unit tests for the FTU and System app pieces
Attachment #8649983 - Flags: review?(apastor) → review?(kevingrandon)
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

I think I'm happy with the changes here and this mostly looks good now. Thanks for the tests!
Attachment #8649983 - Flags: review?(kevingrandon) → review+
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

Have a few more unit tests for ftu/js/late_customization.js incoming. Plus console/debug cleanup.
Attachment #8649983 - Flags: review?(fernando.campo) → review?(francisco)
Comment on attachment 8649983 [details] [review]
[gaia] sfoster:late-customization-system > mozilla-b2g:master

ftu part looking good to me, left some nits but in general looking good.

r+ with a bit more of unit tests as commented above.
Attachment #8649983 - Flags: review?(francisco) → review+
Merged to master: https://github.com/mozilla-b2g/gaia/commit/3d717fa63889d26b0baa026cfb24004311b81a0e

I got the XHR -> LazyLoader.getJSON change, unit tests and I think all the other nits. Thanks Kevin and Francisco.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S9 (16Oct)
Blocks: 1214990
You need to log in before you can comment on or make changes to this bug.