1008990 - Standalone version of UI for link clickers should allow users to install and/or open the FxOS Loop client

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Fernando Jiménez Moreno [:ferjm]]

The hosted Loop client should check for the FxOS Loop client and allow the user to install it if it is not already installed in the device or to open it to handle the call if it is already installed.

Comment 1

[Maria Angeles Oteo (:oteo)]

HI Shell, to verify US 1010168, we need to have this implemented, so Target Milestone: mozilla34 could be a bit late for us, can you please prioritize it so we can test it sooner?

Comment 2

[Mark Banner (:standard8)]

Does anyone have a reference to technical details on how to detect from the web if we're on FFOS, and if the app is installed or not?

Comment 3

[Borja Salguero [:borjasalguero]]

In this case we would like to check if the App is installed or not in the device. For doing it, we can use the method 'checkInstalled' [1]. 

As window.navigator.mozApps is just for our Browser, you can filter using this in order to know if we are in a Mozilla Firefox, and then with the 'checkInstalled' we will know if Loop is installed.

Loop Desktop is not an App from the Marketplace, so there should be no collision (asking in Desktop if the Loop app is installed should be negative, as it's part of the browser, it's not an App with a Manifest file).

I hope it helps!


[1] https://developer.mozilla.org/en-US/docs/Web/API/Apps.checkInstalled

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Borja Salguero (this week part-time in Gaia) [:borjasalguero] from comment #3)
> In this case we would like to check if the App is installed or not in the
> device. For doing it, we can use the method 'checkInstalled' [1].

Note that in order to use .checkInstalled the standalone UI needs to have the same origin as the Loop app in FxOS. Otherwise, the check won't be allowed.

Do we know what's going to be the production URL for the standalone UI? We need to use that URL as the value of https://github.com/mozilla-b2g/firefoxos-loop-client/blob/master/manifest.webapp#L25

Comment 5

[Romain Testard [:RT]]

> Do we know what's going to be the production URL for the standalone UI? We
> need to use that URL as the value of
> https://github.com/mozilla-b2g/firefoxos-loop-client/blob/master/manifest.
> webapp#L25

That's pending naming decision, hopefully next week.

Comment 6

[Mark Banner (:standard8)]

(In reply to Romain Testard [:RT] from comment #5)
> > Do we know what's going to be the production URL for the standalone UI? We
> > need to use that URL as the value of
> > https://github.com/mozilla-b2g/firefoxos-loop-client/blob/master/manifest.
> > webapp#L25
> 
> That's pending naming decision, hopefully next week.

I thought the desire was to keep the name out of the url that we're sharing, which is why we're currently using https://call.services.mozilla.com/

Comment 7

[Romain Testard [:RT]]

The URL will be a branding decision that may depend on the name, I just have not been confirmed yet.
https://call.services.mozilla.com seems really long to be used for a sharing URL.

Comment 8

[Rémy Hubscher (:natim)]

We will have https://call.mozilla.com shortly. Is that short enough?

Comment 9

[Romain Testard [:RT]]

Arcadio confirmed that once they'll decide on the naming (hoping to be done this week) they'll also confirm the URL as the URL is part of the branding. This means the URL exposed to the end users for sharing URLs and the landing page on the URL clicker UI probably will change when the name is confirmed.

Comment 10

[Adam Roach [:abr]]

The question I'm needinfo'd for here is dependent on the outcome of the branding conversation. I'm going to lateral it over to Arcadio under the assumption that the final URL will be of the form https://<product-name>.mozilla.com

Comment 11

[Maire Reavy [:mreavy]]

Hi Darrin -- Now that we have a name, we need UX (part of the Standalone UI) for this.  The mobile Loop app needs this to complete Bug 1010168, so it's a high priority.  In particular, I want the team to verify that we can do the work without any additional mods to FxOS itself.

Comment 12

[Maire Reavy [:mreavy]]

Maria, Fernando -- Can I ask someone from your team to put up code in this bug that checks for the app being installed and then either prompts the user to install if it's not installed or opens the app if it is installed?   Your team is closer to that part of FxOS than my Loop Desktop team is at this point.  We can then take that code and incorporate it into the Standalone app along with the UI work that we need to do for Darrin's UX design. Does this make sense?

In particular, please let me know ASAP if this work requires any FxOS mods.  I don't believe this work will need FxOS mods, but until we get it working, we don't know for sure.  Thanks!

Comment 13

[Maria Angeles Oteo (:oteo)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #12)
> Maria, Fernando -- Can I ask someone from your team to put up code in this
> bug that checks for the app being installed and then either prompts the user
> to install if it's not installed or opens the app if it is installed?   Your
> team is closer to that part of FxOS than my Loop Desktop team is at this
> point.  We can then take that code and incorporate it into the Standalone
> app along with the UI work that we need to do for Darrin's UX design. Does
> this make sense?
Yes, it does :)
Fernando will do that, thanks!!

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Sure, I'll take a look as soon as possible. I believe that as long as the standalone client has the same origin as the FxOS Loop client, we shouldn't need any platform change. But as you said, until it is working we won't know for sure :|.

One note regarding the UX design. We will be using Web Activities to open the FxOS Loop app if it is installed in the device, but Web Activities by design require user interaction to be triggered. That means that we cannot automatically go to the browser, load the standalone client, check that the Loop client is installed and open the Loop client to handle the call. We need an intermediate step where the user explicitly triggers the Loop client by clicking a button for example.

Comment 15

[Romain Testard [:RT]]

Only FFOS users will see this screen so I think it makes sense to match the mobile client UI rather than the link clicker UI guidelines (both should match whenever possible obviously but it feels this should be as close to the mobile app as possible). Therefore it makes sense for TEF to provide the UX also here?
 
I needInfo Vicky for comments and I'll let Darrin and Vicky agree on the most appropriate solution.

Comment 16

[Maire Reavy [:mreavy]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #14)
> Sure, I'll take a look as soon as possible. I believe that as long as the
> standalone client has the same origin as the FxOS Loop client, we shouldn't
> need any platform change. But as you said, until it is working we won't know
> for sure :|.

Fernando -- Why do they need to have the same origin?  And what happens if they it don't?

Comment 17

[Daniel Coloma:dcoloma]

(In reply to Romain Testard [:RT] from comment #15)
> Only FFOS users will see this screen so I think it makes sense to match the
> mobile client UI rather than the link clicker UI guidelines (both should
> match whenever possible obviously but it feels this should be as close to
> the mobile app as possible). Therefore it makes sense for TEF to provide the
> UX also here?
>  
> I needInfo Vicky for comments and I'll let Darrin and Vicky agree on the
> most appropriate solution.

We agree some UX work needs to be done to align experiences, but that is up to Darrin and Vicky. However, that should not stop us to land a basic implementation of this to test this flow end-to-end. Can we do the UX alignment as a follow-up?

Comment 18

[Darrin Henein [:darrin]]

Attachment: fxos-loop-install.png

Comment 19

[Darrin Henein [:darrin]]

Attachment: fxos-loop-open.png

I've provided 2 images to show what the intended experience is... while we may refine colors/text etc., I think this should serve to communicate what the UX should be like. Let me know if there are any questions.

Comment 20

[Maire Reavy [:mreavy]]

Thanks, Darrin.  I believe with Darrin's 2 screens (attached), we have enough UX to unblock this bug and verify functionality.

Daniel, Fernando, Maria -- If anyone from your team wants to modify the standalone app based on Darrin's UX and submit it for review to my team, that's totally fine by me/us.  The key is to test functionality (as you say above).  Otherwise, I'll see how quickly I can get this UI work on our plates.  And thanks to Fernando for doing the other part (re: Comment 14).

Comment 21

[alainez]

Hi 

Just one quick comment on the wordmark: please remove the exclamation point.

Should read: Firefox Hello
Should not read: Firefox Hello!

Thank you.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #16)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #14)
> > Sure, I'll take a look as soon as possible. I believe that as long as the
> > standalone client has the same origin as the FxOS Loop client, we shouldn't
> > need any platform change. But as you said, until it is working we won't know
> > for sure :|.
> 
> Fernando -- Why do they need to have the same origin?  And what happens if
> they it don't?

We talked about this on IRC, but I'll also reply here.

The reason is because we will be using mozApps.checkInstalled, but it only works with same origin requests [1]. We can set our own origin in the FxOS client, but it will be an app:// origin while the standalone UI is an https:// origin, and I have to check if that passes the cross origin check or not.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#191

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Darrin Henein [:darrin] from comment #19)
> Created attachment 8456901 [details]
> fxos-loop-open.png
> 
> I've provided 2 images to show what the intended experience is... while we
> may refine colors/text etc., I think this should serve to communicate what
> the UX should be like. Let me know if there are any questions.

Thanks Darrin.

Do we want to allow the user to make the call within the Browser app? Or do we want to "force" him to install/use the FxOS Loop app? Maybe this is more a question for the product team.

In the last case, I think the UX could be simplified to a single button with two states:

* "Install Loop app": should take you to the Marketplace to install Loop. Once the app is installed reloading the standalone app should show the "Call" state.
* "Call": should open the Loop app and make the call.

Note that making the call within the Browser app may have unexpected results. We've been preparing the FxOS Loop client to handle situations like incoming cellular calls or alarms while the user is on a Loop call or routing the audio from the earpiece to the speaker and vice versa that the Browser app won't be able to handle. So I think it makes sense to forbid the call within the Browser.

In any case, since the critical issue right now is verifying the feasibility of this feature, I'd leave this bug to add only the basic pieces. We can update it with the final UX/UI in a follow-up.

Comment 24

[Romain Testard [:RT]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #23)
> (In reply to Darrin Henein [:darrin] from comment #19)
> > Created attachment 8456901 [details]
> > fxos-loop-open.png
> > 
> > I've provided 2 images to show what the intended experience is... while we
> > may refine colors/text etc., I think this should serve to communicate what
> > the UX should be like. Let me know if there are any questions.
> 
> Thanks Darrin.
> 
> Do we want to allow the user to make the call within the Browser app? Or do
> we want to "force" him to install/use the FxOS Loop app? Maybe this is more
> a question for the product team.
> 
> In the last case, I think the UX could be simplified to a single button with
> two states:
> 
> * "Install Loop app": should take you to the Marketplace to install Loop.
> Once the app is installed reloading the standalone app should show the
> "Call" state.
> * "Call": should open the Loop app and make the call.
> 
> Note that making the call within the Browser app may have unexpected
> results. We've been preparing the FxOS Loop client to handle situations like
> incoming cellular calls or alarms while the user is on a Loop call or
> routing the audio from the earpiece to the speaker and vice versa that the
> Browser app won't be able to handle. So I think it makes sense to forbid the
> call within the Browser.
> 
> In any case, since the critical issue right now is verifying the feasibility
> of this feature, I'd leave this bug to add only the basic pieces. We can
> update it with the final UX/UI in a follow-up.

We've just had the same discussion with Darrin, with the same outcome. The page should not allow the user to initiate a call but rather only download the application or launch it.

Comment 25

[jmunuera]

(In reply to Romain Testard [:RT] from comment #24)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #23)
> > (In reply to Darrin Henein [:darrin] from comment #19)
> > > Created attachment 8456901 [details]
> > > fxos-loop-open.png
> > > 
> > > I've provided 2 images to show what the intended experience is... while we
> > > may refine colors/text etc., I think this should serve to communicate what
> > > the UX should be like. Let me know if there are any questions.
> > 
> > Thanks Darrin.
> > 
> > Do we want to allow the user to make the call within the Browser app? Or do
> > we want to "force" him to install/use the FxOS Loop app? Maybe this is more
> > a question for the product team.
> > 
> > In the last case, I think the UX could be simplified to a single button with
> > two states:
> > 
> > * "Install Loop app": should take you to the Marketplace to install Loop.
> > Once the app is installed reloading the standalone app should show the
> > "Call" state.
> > * "Call": should open the Loop app and make the call.
> > 
> > Note that making the call within the Browser app may have unexpected
> > results. We've been preparing the FxOS Loop client to handle situations like
> > incoming cellular calls or alarms while the user is on a Loop call or
> > routing the audio from the earpiece to the speaker and vice versa that the
> > Browser app won't be able to handle. So I think it makes sense to forbid the
> > call within the Browser.
> > 
> > In any case, since the critical issue right now is verifying the feasibility
> > of this feature, I'd leave this bug to add only the basic pieces. We can
> > update it with the final UX/UI in a follow-up.
> 
> We've just had the same discussion with Darrin, with the same outcome. The
> page should not allow the user to initiate a call but rather only download
> the application or launch it.

Yes, all the calls should be launched with the Loop Client (the browser will redirect to the client or the marketplace).

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

I am afraid that my concerns about the cross origin check that I mentioned in comment 22 are becoming real and so far I couldn't make a mozApps.checkInstalled(app://originA) done from http://originA work.
Fabrice, any idea about how could we workaround this? Thanks!

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #26)
> I am afraid that my concerns about the cross origin check that I mentioned
> in comment 22 are becoming real and so far I couldn't make a
> mozApps.checkInstalled(app://originA) done from http://originA work.
> Fabrice, any idea about how could we workaround this? Thanks!

So this is the same need we had to show the loop icons in the contacts app. We solved this one by letting certified apps do "dry runs" of activity calls with the 'getFilterResults' option. Unfortunately we can't open that to web pages... And changing the same origin rules is not an option either unfortunately since we have no good way to verify origin ownership in general. I'm gonna think a bit more about that to see how we could solve this.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

I've been thinking about a workaround for this issue. It's quite a terrible hack, but I think it might work, at least for now.

Loop is going to be installed from the Marketplace and the Marketplace can tell us via mozApps.getInstalled() which apps where installed from there and so in this case it can tell if Loop is installed in the device. So we would just need to load the Loop Marketplace page, let the Marketplace do the check and probably sniff the content of the install/launch button.

If this works, it means that anyone could see which apps you've installed from the Marketplace this way, which exposes the privacy issue that the cross origin restriction for mozApps.checkInstalled() tries to avoid. So I am restricting access to this bug and adding a few folks from the security and marketplace teams in CC.

Comment 29

[[:fabrice] Fabrice Desré]

I think we can do slightly better:
Instead of loading the full Loop page from the marketplace and do ugly scrapping things, the marketplace could offer an embeddable iframe that would display just an install/launch button based on the manifest url passed as a parameter to the iframe source. Iguess the marketplace could protect itself by whitelisting the window.opener that is allowed to see which app is installed to limit security issues.

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fabrice Desré [:fabrice] from comment #29)
> I think we can do slightly better:
> Instead of loading the full Loop page from the marketplace and do ugly
> scrapping things, the marketplace could offer an embeddable iframe that
> would display just an install/launch button based on the manifest url passed
> as a parameter to the iframe source. Iguess the marketplace could protect
> itself by whitelisting the window.opener that is allowed to see which app is
> installed to limit security issues.

Yes :) It would be great if the Marketplace could provide such a thing. In this case, I don't think we need the buttons but a small postMessage based API wrapping mozApps.checkInstalled and mozApps.installPackage.

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #28) 
> If this works, it means that anyone could see which apps you've installed
> from the Marketplace this way, which exposes the privacy issue that the
> cross origin restriction for mozApps.checkInstalled() tries to avoid. So I
> am restricting access to this bug and adding a few folks from the security
> and marketplace teams in CC.

Actually, I just checked with Antonio and this doesn't seem to be possible. We cannot see the DOM tree of a web page loaded inside an iframe if its content origin is different from the embedder's origin. So we might depend entirely on the Marketplace exposing a dedicated postMessage based API as mentioned above.

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

Forget about the above solution. I found another way to detect that Loop is installed. But we still need some changes in the Marketplace for something else.

Having an unique "Call" button we can have the following flow:

1. FxOS Loop will implement a "loop-call" (or whatever) activity.

2. When the user clicks on a Loop url, the standalone Loop client will be loaded in the Browser and will show the UI with one "Call" button (even if we don't know that the FxOS Loop app is installed yet).

3. When the user clicks on the "Call" button, we will be triggering the "loop-call" activity, which will open the FxOS Loop client or fail with a "NO_PROVIDER" error.

3.1 If the app is not installed and we get the "NO_PROVIDER" error, we trigger a Marketplace activity [2] that will open the Marketplace app and will allow the user to install Loop.

Unfortunately, the Marketplace activity will not return to the standalone Loop client when the FxOS Loop app installation is completed. It will just show the "Launch" button, which will allow the user to launch the FxOS Loop app, but without any call information.

Ideally, what should happen is that the Marketplace exposes a new inline web activity that returns the control to the caller when the app installation request is completed. I filed bug 1040029 for this.

This way the standalone Loop client will get the control back and will be able to follow continue with the Loop call flow:

3.2. After the user installs the FxOS Loop app from the Marketplace, the Marketplace closes itself returning the control to the standalone Loop client which can trigger the "loop-call" activity again, with the URL token.

4. The FxOS Loop app opens and starts the call.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/activities/src/ActivitiesService.jsm#232
[2] https://github.com/mozilla/fireplace/wiki/Web-Activities#marketplace-app

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

This is not a Security-Sensitive Core Bug anymore but I can't reset the flag, so if anyone can, please do

Comment 34

[[:fabrice] Fabrice Desré]

We discussed a bit more on irc, and decided to follow this plan:

1) Short term, do what Fernando proposes in comment 32.
2) As soon as possible, implement a postMessage based api to let the marketplace tell the hosted Loop site if the app is installed or not, and do the right thing afterward.

The solution 1) is still vulnerable to "activity hijacking" by any app that would want to redirect the user to eg. a competing solution - whether it's actually desirable or not is left as an exercise to the reader ;)

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

I am in the process of uploading a mock application to the marketplace so we can test the current flow with the current limitations.

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Wil, do you think that it is possible to implement in the Marketplace this postMessage based API mentioned above? We can workaround this temporarily with the webactivities alternative, but the "activity hijacking" issue is certainly there, so it would be great to have a more robust and secure solution.

Comment 38

[Frederik Braun [:freddy]]

Removing security flag on Fernando's request.

Comment 39

[Wil Clouser [:clouserw]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #37)
> Wil, do you think that it is possible to implement in the Marketplace this
> postMessage based API mentioned above? We can workaround this temporarily
> with the webactivities alternative, but the "activity hijacking" issue is
> certainly there, so it would be great to have a more robust and secure
> solution.

Yes.  Can you file a bug for that also?  Do you want to do that instead of bug 1040029?  It would likely be the same person and if they could just work on the "real" goal it would be best.

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

Thanks Wil. I filed bug 1040278 for this. I believe we can relay entirely on this postMessage based API if it allows to handle the entire check and install process from the 3rd party app and so we won't need bug 1040029. If we need to jump to the Marketplace app at some point via activities, we will need bug 1040029. But it seems that it is not the case having bug 1040278 done.

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

I think this depends on how we end up serving the static content pages in our regular deployment process (eg do we continue to use two separate servers and a redirect?).  My assumption was that this hadn't really been decided, and was actually likely to come up at the meeting on Monday to discuss bug 1042528.  Adding ckarloff for his thoughts here...

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

Whoops, I put that in the wrong bug.  Plz ignore!

Comment 43

[Maire Reavy [:mreavy]]

Shell -- Is there a way we could tag this to indicate that this is a requirement for the FxOS loop app (not desktop)?  I put something in the whiteboard, but that may not be sufficient.

Comment 44

[:shell escalante]

Changing to P4 NOT as a reflection of priority - but P4's are being implemented by folks not on the Loop Desktop client developers.

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Since bug 1040278 is taking more than expected, I've created a fake marketplace with the same iframe-install.html that the real marketplace is hosting but without the whitelist limitation, so we can test this patch. The code for this fake marketplace is at https://github.com/ferjm/fake-firefox-marketplace and it is hosted at http://fake-market.herokuapp.com (and http://fake-market.herokuapp.com/iframe-install.html).

Dan, this patch checks if we are on a FxOS device and in that case tries to launch the call in the FxOS loop client if it is installed and available. It does it via WebActivities [1]. If the app is not installed, the standalone UI embeds the content of http://fake-market.herokuapp.com/iframe-install.html (or the market iframe specified via config) which contains a wrapper of the mozApps API. Via postMessage we ask this iframe to trigger the installation of the FxOS Loop client. Once this is completed, the control is returned to the standalone UI, that can continue the call process by trigger the WebActivity that is handled by the FxOS Loop client ("loop-call").

In this case, the app that is installed and triggered to handle the call is not the real FxOS Loop client cause we cannot install privileged apps from this fake marketplace. Instead I created a mock app that simply exposes the same web activity as the FxOS Loop client. This is also configurable.

I'd love to hear your feedback about this approach. If it is positive, I'll write the tests for it and ask for your review. I think we can make progress landing this patch if the review is positive even if bug 1040278 is not fixed yet as the market URL selection can be done via configuration later. At least we would unblock our QA team to test this feature.

Thanks.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Web_Activities

Comment 46

[Dan Mosedale (:dmosedale, :dmose)]

Just reading the description makes me concerned that it's likely to be fragile, but, that said, I haven't entirely wrapped my head around the code flow here, and unfortunately, I won't have time to today (Weds).  I'll leave the feedback? and stare at this some more Thurs morning Pacific time.  If you need the feedback sooner, Standard8 could be a good candidate, as he's on European time...

Comment 47

[Romain Testard [:RT]]

Darrin, can you amend the UX so that a call button is not displayed?
https://people.mozilla.org/~dhenein/labs/loop-link-spec/#mobile-banner

As a FFOS browser user you should NOT be able to initiate a call from within the browser.

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

Actually, I believe there should be a call button in the standalone UI to initiate the call *within* the FxOS app. We use the standalone UI to show the details of the call. The user can the decide if she wants to initiate the call (after installing the FxOS app if it is not already there) by clicking in the standalone UI call button or not.

The button that is not needed is the "Install" button.

Comment 49

[Romain Testard [:RT]]

OK, makes sense - my bad :)

Comment 50

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8464837 [details] [diff] [review]
v1

So the standalone code has a big overhaul in progress that's about to land in bug 10000127, where a lot of the UI code you're modifying is becoming reactified.  In order to avoid a lot of merge pain, I'd strongly suggesting waiting until that lands (next day or two) before touching this more.

My suspicion is that initiateFxOSCall wants to really be in it's own React (sub)-view, perhaps called FxOSHiddenMarketplaceView.

I kinda suspect that initiateFxOSCall wants to be pushed down into a model, likely the conversation model, though I could imagine it also being a utility function or its own model.  But maybe I'm over-engineering things a bit.  Once the standalone code lands, I suspect you could pair with Niko or me to figure out a reasonable way forward, and I wouldn't expect it to be very hard, though the automated tests could be a little interesting.  :-)

Comment 51

[Dan Mosedale (:dmosedale, :dmose)]

One thing that came up when chatting with ferjm is that it's not clear how we would want to handle the error condition of the marketplace being unreachable.  Darrin, your thoughts here?

Comment 52

[Darrin Henein [:darrin]]

Just spoke with Dan, and I think we firstly need to see whats available to the embedding iframe. If we can detect an unavailability, I propose we hide the bar. We should follow up on web app quality/feasibility and whether we allow the user to progress using the standalone UI.

Comment 53

[Wil Clouser [:clouserw]]

https://github.com/mozilla/zamboni/commit/5cf541f7e2617e4bcbaf497c2a09dc15cf1da794 is going live around noon today on the Marketplace so you can start using the iframe.

Comment 54

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Thanks for the previous feedback Dan.

I rebased the patch on top of the latest m-i and updated it with the React bits trying to follow your suggestions from comment 50.

Regarding the error scenarios, we should not allow the call within the browser (check comment 24), so I am just showing an error message explaining to the user the need to manually install the FxOS app from the Firefox Marketplace. Note that this should never happen anyway.

I still need to do some cleaning of the patch and add the tests, but I'd like to know if the overall approach and the React bits are on the right direction.

Thanks!

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8467921 [details] [diff] [review]
v2

I meant to ask for feedback, not review

Comment 56

[Dan Mosedale (:dmosedale, :dmose)]

Sorry for not getting to this yet, will try to hit it tomorrow (Thurs).

Comment 57

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8467921 [details] [diff] [review]
v2

Review of attachment 8467921 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in getting back to you.

I like that this factors apart the FxOS-specific code.  That said, having the main conversation form functionality itself be a mixin feels inside-out (and harder to reason about, because it moves a whole bunch of methods that are typically in a component elsewhere).  Since there's really only one method difference between the two shells for that mixin, one thing I could imagine would be to make that method (or an object containing it) be the thing that's passed into the ConversationFormView.  Without trying it, it's not obvious to me whether that's a good idea or not.  But in general, it feels like we want to a use a different mechanism (composition?) for this factoring...

I suspect it would be easier for us to discuss this using Hangouts so that we can both talk synchronously and point at code.  I'm happy to make some time for that if you'd like.

::: browser/components/loop/standalone/Makefile
@@ +5,5 @@
>  LOOP_SERVER_URL := $(shell echo $${LOOP_SERVER_URL-http://localhost:5000})
>  LOOP_PENDING_CALL_TIMEOUT := $(shell echo $${LOOP_PENDING_CALL_TIMEOUT-20000})
> +LOOP_MARKET_URL := $(shell echo $${LOOP_MARKET_URL-http://fake-market.herokuapp.com/iframe-install.html})
> +LOOP_FXOS_APP_NAME := $(shell echo $${LOOP_FXOS_APP_NAME-loop})
> +LOOP_FXOS_APP_MANIFEST_URL := $(shell echo $${LOOP_FXOS_APP_MANIFEST_URL-http://fake-market.herokuapp.com/apps/packagedApp/manifest.webapp})

Note that for local development, config.js is now being generated dynamically by server.js, so that will need some changes as well.  (Sorry about this, it's now looking like we're going to need to single-source this data sooner than I expected).

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +329,5 @@
> +      });
> +
> +      _model.initiate((function needInstall() {
> +        this.setState({
> +          marketplaceSrc: loop.config.marketplaceUrl

This doesn't feel like the sort of thing that wants to be state.  I.e. it's not going to vary during one invocation of the webapp, is it?

Comment 58

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

Dan, as I mentioned yesterday, this patch has a new approach where we use different conversation models depending on the platform. I added a specific model for FxOS that has a setupOutgoingCall function just like the conversation model for the rest of the platforms. This method is called within the handler for the click event on the "Call" button.

I also removed the polling within the hack that I mentioned yesterday to workaround the absence of iframe onload events in React cause the embedded Marketplace content is already sending a "loaded" message that let us know when to request the installation of the FxOS Loop client. We just need to start listening for messages coming from the iframe when we update its src.

Finally, I added some tests. Unfortunately they are not covering the whole activities + installation process as I can't really see an easy way to test that.

Thanks in advance for the review.

Comment 59

[Dan Mosedale (:dmosedale, :dmose)]

Sorry for the delay.  I hope to get to this Monday or Tuesday.

Comment 60

[Fernando Jiménez Moreno [:ferjm]]

Thanks Dan. No rush, I'll be on PTO for the next two weeks :).

Comment 61

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8472579 [details] [diff] [review]
v3

Review of attachment 8472579 [details] [diff] [review]:
-----------------------------------------------------------------

I'm happy to review this when I get back (Weds, Sept 3rd), and I don't think it will be a problem for it to land then.  That said, Mark, if you want to try and get this in before the uplift, I wanted to put this on your radar.  If not, no worries, put the review back to me, and I'll look when I get back.

Comment 62

[Maria Angeles Oteo (:oteo)]

Hi Maire,
we need someone in your team to review this, could you help us with it?
Thanks a lot!

Comment 63

[Maire Reavy [:mreavy]]

Comment on attachment 8472579 [details] [diff] [review]
v3

Review of attachment 8472579 [details] [diff] [review]:
-----------------------------------------------------------------

Dan -- Can you look at this?  It's toward the end of Mark's day today, and he thinks you have more context to review this.  Thanks!

Comment 64

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Standalone link clicker UI support for FxOS client, v4

Comment 65

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8483957 [details] [diff] [review]
Standalone link clicker UI support for FxOS client, v4

Rebased against today's fx-team repo.  I haven't really gotten into the reveiew yet, as I had to spend a fair amount of time debugging and fixing a failing unit test.  In the future, it would be very helpful if you could ensure that the unit tests pass before requesting review.  <http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/README.txt#16> has info on how to run these test quickly.

If I've misunderstood the code and these tests were passing before, and the failure was the result of bit-rot or merge problems, I apologize.  

In any case, we've got something that passes now, and I'll resume the review on Thursday (Pacific time).

Comment 66

[Fernando Jiménez Moreno [:ferjm]]

Thanks Dan. I believe the tests were passing when I asked for review, otherwise I wouldn't have asked for it. Sorry that you encountered a failure after rebasing.

Comment 67

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Standalone link clicker UI support for FxOS client, v5

Comment 68

[Dan Mosedale (:dmosedale, :dmose)]

No worries, Fernando.  Here's a new version, rebased on top of this morning's fx-team landing of the second part of the visual uplift.

Comment 69

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Standalone link clicker UI support for FxOS client, v6

Rebase to latest fx-team to cope with l10n changes.

Comment 70

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8485158 [details] [diff] [review]
Standalone link clicker UI support for FxOS client, v6

Review of attachment 8485158 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking really good.  The structure of the code looks great, and does an excellent job of reflecting the React idiom.  There are some testing issues, mostly detailed in the review.  

One more general one that's not is that in general, we try to mostly write and structure our tests around units of code and their specific behaviors.  That said, I can see why a bunch of the FirefoxOS stuff requires common setup.  I suspect that in the future, the way to address this will be to factor our some utility functions, and possibly use the structures suggested at <https://github.com/visionmedia/mocha/wiki/Shared-Behaviours>.  That said, I don't think it makes sense to dig into this as part of this bug.

I don't have an FxOS device set up for easy testing of dev stuff just yet, but I don't want that to delay this review any further.  That said, the current version of the patch seems pass all unit tests and the end-to-end call test on desktop.

I'd like to take another look after the changes are addressed, so r- for now.

Thanks for both the hard work and your patience!

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +95,5 @@
> +    },
> +
> +    componentDidUpdate: function() {
> +      if (this.props.onMarketplaceMessage) {
> +        window.addEventListener("message", this.props.onMarketplaceMessage);

Could this eventListener be added to the iframe window object rather than main window object?

As written, it would appear that we'll be adding a new listener on every update.  If this is the right behavior, it would be helpful to add a comment explaining why this makes sense.

Also, don't we need to remove the listener somewhere to avoid a leak?

@@ +100,5 @@
> +      }
> +    }
> +  });
> +
> +  var FxOSConversationModel = Backbone.Model.extend({

This wants unit testing, at least so that we can ensure that external APIs (eg MozActivity, postMessage, various incoming messages) are being driven/handled correctly.

@@ +112,5 @@
> +          type: "loop/token",
> +          token: this.get("loopToken"),
> +          callerId: this.get("callerId"),
> +          // XXX For now, we assume both audio and video as there is no
> +          // other option to select (bug 1048333)

This is resolved now, so I'd suggest spinning out a new bug to handle the audio-only case.

@@ +272,5 @@
> +      });
> +      this.setState({
> +        onMarketplaceMessage: this.props.model.onMarketplaceMessage.bind(
> +          this.props.model
> +        )

I suspect the bind is not necessary because React will take care of that for you.  If I'm correct, you'll see a warning to that effect in the FxOS console when this code gets executed.

@@ +332,5 @@
>            "https://www.mozilla.org/privacy/'>" + privacy_notice_name + "</a>"
>        });
>  
> +      var btnClassStartCall = "btn btn-large btn-accept " +
> +                              loop.shared.utils.getTargetPlatform();

I _think_ this is just a remnant of one of the merges, unless you added the getTargetPlatform call in here.  btnClassStartCall is no longer used, so we should get rid of this line, and if we need the target platform in the class here, it would be helpful to understand how that's used.

@@ +664,5 @@
> +      // So far WebActivities are exposed only in FxOS, but they may be
> +      // exposed in Firefox Desktop soon, so we check for its existence
> +      // and also check if the UA belongs to a mobile platform.
> +      return !!window.MozActivity && /mobi/i.test(platform);
> +    },

WebActivities are also exposed in WebRT apps on Firefox for Android.  It feels like these tests are really proxy tests for what we actually want to know, which might be "Does this machine support web activities _and_ have a loop-call activity registered with the system by someone other than us".  Would making the code check for that be possible/better?

In the interest of landing sooner, I could live with an XXX comment and a spinoff bug for handling that case.

::: browser/components/loop/standalone/content/l10n/data.ini
@@ -1,2 @@
>  @import url(loop.en-US.properties)
> -

This was my mistake in merging. Feel free to remove this change in the next iteration.  Or don't bother, since it's not a big deal.  :-)

::: browser/components/loop/standalone/content/l10n/loop.en-US.properties
@@ +45,5 @@
>  ## LOCALIZATION NOTE (call_url_creation_date_label): Example output: (from May 26, 2014)
>  call_url_creation_date_label=(from {{call_url_creation_date}})
>  call_progress_connecting_description=Connecting…
>  call_progress_ringing_description=Ringing…
> +fxos_app_needed=Please, install the Firefox Hello app from the Firefox Marketplace.

No comma necessary after please.  

It's probably worth adding a screenshot or two and requesting ux-review from :darrin.

Also, instead of Firefox Hello, don't we want to pass in loop.config.fxosApp.name from JS?

::: browser/components/loop/test/standalone/webapp_test.js
@@ +678,5 @@
> +        sinon.assert.calledTwice(conversation.listenTo);
> +        sinon.assert.calledWith(conversation.listenTo, conversation,
> +                                "session:error", sinon.match.func);
> +        sinon.assert.calledWith(conversation.listenTo, conversation,
> +                                "fxos:app-needed", sinon.match.func);

We try to focus our tests around the APIs and their behaviors rather than the implementations, as this is most robust when refactoring time comes around.

Part of this is to say that I think the original test that you extended should just go away.  It's not testing anything that would be meaningful to an API client.

As far as testing fxos:app-needed, that should be unit-testable by triggering that event and then checking for the state change.

@@ +751,5 @@
> +        sinon.assert.calledOnce(notifier.errorL10n);
> +        sinon.assert.calledWithExactly(notifier.errorL10n,
> +                                       "custom error");
> +      });
> +

I think this belongs next to the other session:error test in the Events block of StartConversationView, so it's easier to see what's being tested at a glance.

@@ +808,5 @@
>          expect(helper.isFirefox("Opera")).eql(false);
>        });
>      });
> +
> +    describe("#isFirefoxOS no mozActivities", function() {

From a style standpoint, this wants to be two describe blocks:

describe("#isFirefoxOS, function () {
  describe("with mozActivities") ...

  describe("without mozActivities") ...
}

Assuming we stick to mozActivities as the detector, which I'm not sure we want to, as mentioned elsewhere in the review.

@@ +866,5 @@
> +      });
> +    });
> +
> +    it("Should load the StartConversationView with FxOSConversationModel",
> +      function() {

The code inside the function wants to be indented two spaces more for readability.

@@ +871,5 @@
> +      router.initiate("fakeToken");
> +
> +      sinon.assert.calledOnce(router.loadReactComponent);
> +      var model = router.loadReactComponent.getCall(0).args[0]._store.props
> +                        .model;

This feels fragile.  Perhaps the code under test needs to be factored slightly to be more testable?

@@ +872,5 @@
> +
> +      sinon.assert.calledOnce(router.loadReactComponent);
> +      var model = router.loadReactComponent.getCall(0).args[0]._store.props
> +                        .model;
> +      expect(model instanceof loop.webapp.FxOSConversationModel).to.eql(true);

expect(model).to.be.an.instanceOf(FxOSConversationModel) is a bit more readable and is local style for this and other tests like it.

@@ +883,5 @@
> +      beforeEach(function() {
> +        conversation = new loop.webapp.FxOSConversationModel({
> +          loopToken: "fakeToken"
> +        });
> +        fakeSubmitEvent = {preventDefault: sinon.spy()};

This doesn't appear to be used, so can presumably the entire var can be removed.

@@ +884,5 @@
> +        conversation = new loop.webapp.FxOSConversationModel({
> +          loopToken: "fakeToken"
> +        });
> +        fakeSubmitEvent = {preventDefault: sinon.spy()};
> +        setupOutgoingCall = sinon.stub(conversation, "setupOutgoingCall");

Thought it's not strictly necessary, local convention is to just use sandbox.stub() for everything so that we end up with fewer cleanup errors.

Comment 71

[Maire Reavy [:mreavy]]

Thanks for the review, Dan!

Comment 72

[Maria Angeles Oteo (:oteo)]

Hi Shell, the milestone change does not mean that the patch can not be uplifted to mozilla34, doesn't it?
We need to land this in mozilla34 for Loop FxOS.

Comment 73

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the awesome review Dan! Sorry I couldn't get to this before.

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #70)
> 
> ::: browser/components/loop/standalone/content/js/webapp.jsx
> @@ +95,5 @@
> > +    },
> > +
> > +    componentDidUpdate: function() {
> > +      if (this.props.onMarketplaceMessage) {
> > +        window.addEventListener("message", this.props.onMarketplaceMessage);
> 
> Could this eventListener be added to the iframe window object rather than
> main window object?

The Marketplace iframe is doing a window.top.postMessage where if I am not wrong window.top is the embedder window and in this case the global window of the standalone UI.

> 
> As written, it would appear that we'll be adding a new listener on every
> update.  If this is the right behavior, it would be helpful to add a comment
> explaining why this makes sense.
> 

This happens only once when we change the src property of the iframe. I'll add a comment about it.

> Also, don't we need to remove the listener somewhere to avoid a leak?
> 

Yes, we do. I'll remove it as soon as we get the install-package message response from the iframe.

> @@ +100,5 @@
> > +      }
> > +    }
> > +  });
> > +
> > +  var FxOSConversationModel = Backbone.Model.extend({
> 
> This wants unit testing, at least so that we can ensure that external APIs
> (eg MozActivity, postMessage, various incoming messages) are being
> driven/handled correctly.
> 

Ok, I'm trying to add some tests for this.

> @@ +112,5 @@
> > +          type: "loop/token",
> > +          token: this.get("loopToken"),
> > +          callerId: this.get("callerId"),
> > +          // XXX For now, we assume both audio and video as there is no
> > +          // other option to select (bug 1048333)
> 
> This is resolved now, so I'd suggest spinning out a new bug to handle the
> audio-only case.
> 

Great! I am just adding the fix here as it is quite simple. We have that info on the conversation model.

> @@ +272,5 @@
> > +      });
> > +      this.setState({
> > +        onMarketplaceMessage: this.props.model.onMarketplaceMessage.bind(
> > +          this.props.model
> > +        )
> 
> I suspect the bind is not necessary because React will take care of that for
> you.  If I'm correct, you'll see a warning to that effect in the FxOS
> console when this code gets executed.
> 

Removing the bind throws an error about this.setupOutgoingCall being undefined.

> @@ +332,5 @@
> >            "https://www.mozilla.org/privacy/'>" + privacy_notice_name + "</a>"
> >        });
> >  
> > +      var btnClassStartCall = "btn btn-large btn-accept " +
> > +                              loop.shared.utils.getTargetPlatform();
> 
> I _think_ this is just a remnant of one of the merges, unless you added the
> getTargetPlatform call in here.  btnClassStartCall is no longer used, so we
> should get rid of this line, and if we need the target platform in the class
> here, it would be helpful to understand how that's used.
> 

I don't recall adding that :D.

> @@ +664,5 @@
> > +      // So far WebActivities are exposed only in FxOS, but they may be
> > +      // exposed in Firefox Desktop soon, so we check for its existence
> > +      // and also check if the UA belongs to a mobile platform.
> > +      return !!window.MozActivity && /mobi/i.test(platform);
> > +    },
> 
> WebActivities are also exposed in WebRT apps on Firefox for Android.  It
> feels like these tests are really proxy tests for what we actually want to
> know, which might be "Does this machine support web activities _and_ have a
> loop-call activity registered with the system by someone other than us". 
> Would making the code check for that be possible/better?
> 

I am afraid that such a check is not possible. We can check for web activities support but we cannot know without triggering the activity if there is any handler for it. There is a way to check that, but it is allowed only to certified apps (bug 1017420).

> In the interest of landing sooner, I could live with an XXX comment and a
> spinoff bug for handling that case.
> 

Ok, will do that. Bug 1065403.

> Also, instead of Firefox Hello, don't we want to pass in
> loop.config.fxosApp.name from JS?
> 

Yes, that seems better.

> ::: browser/components/loop/test/standalone/webapp_test.js
> @@ +678,5 @@
> > +        sinon.assert.calledTwice(conversation.listenTo);
> > +        sinon.assert.calledWith(conversation.listenTo, conversation,
> > +                                "session:error", sinon.match.func);
> > +        sinon.assert.calledWith(conversation.listenTo, conversation,
> > +                                "fxos:app-needed", sinon.match.func);
> 
> We try to focus our tests around the APIs and their behaviors rather than
> the implementations, as this is most robust when refactoring time comes
> around.
> 
> Part of this is to say that I think the original test that you extended
> should just go away.  It's not testing anything that would be meaningful to
> an API client.
> 
> As far as testing fxos:app-needed, that should be unit-testable by
> triggering that event and then checking for the state change.
> 

Makes sense. Done.

> @@ +751,5 @@
> > +        sinon.assert.calledOnce(notifier.errorL10n);
> > +        sinon.assert.calledWithExactly(notifier.errorL10n,
> > +                                       "custom error");
> > +      });
> > +
> 
> I think this belongs next to the other session:error test in the Events
> block of StartConversationView, so it's easier to see what's being tested at
> a glance.
> 

Done.

> @@ +808,5 @@
> >          expect(helper.isFirefox("Opera")).eql(false);
> >        });
> >      });
> > +
> > +    describe("#isFirefoxOS no mozActivities", function() {
> 
> From a style standpoint, this wants to be two describe blocks:
> 
> describe("#isFirefoxOS, function () {
>   describe("with mozActivities") ...
> 
>   describe("without mozActivities") ...
> }
> 
> Assuming we stick to mozActivities as the detector, which I'm not sure we
> want to, as mentioned elsewhere in the review.
> 

Done.

> @@ +866,5 @@
> > +      });
> > +    });
> > +
> > +    it("Should load the StartConversationView with FxOSConversationModel",
> > +      function() {
> 
> The code inside the function wants to be indented two spaces more for
> readability.
> 

Done.

> @@ +871,5 @@
> > +      router.initiate("fakeToken");
> > +
> > +      sinon.assert.calledOnce(router.loadReactComponent);
> > +      var model = router.loadReactComponent.getCall(0).args[0]._store.props
> > +                        .model;
> 
> This feels fragile.  Perhaps the code under test needs to be factored
> slightly to be more testable?
> 

Sorry, but I really can't think about an alternative approach :(. Any suggestion?

> @@ +872,5 @@
> > +
> > +      sinon.assert.calledOnce(router.loadReactComponent);
> > +      var model = router.loadReactComponent.getCall(0).args[0]._store.props
> > +                        .model;
> > +      expect(model instanceof loop.webapp.FxOSConversationModel).to.eql(true);
> 
> expect(model).to.be.an.instanceOf(FxOSConversationModel) is a bit more
> readable and is local style for this and other tests like it.
> 

Done.

> @@ +883,5 @@
> > +      beforeEach(function() {
> > +        conversation = new loop.webapp.FxOSConversationModel({
> > +          loopToken: "fakeToken"
> > +        });
> > +        fakeSubmitEvent = {preventDefault: sinon.spy()};
> 
> This doesn't appear to be used, so can presumably the entire var can be
> removed.
>

Done.
 
> @@ +884,5 @@
> > +        conversation = new loop.webapp.FxOSConversationModel({
> > +          loopToken: "fakeToken"
> > +        });
> > +        fakeSubmitEvent = {preventDefault: sinon.spy()};
> > +        setupOutgoingCall = sinon.stub(conversation, "setupOutgoingCall");
> 
> Thought it's not strictly necessary, local convention is to just use
> sandbox.stub() for everything so that we end up with fewer cleanup errors.

Done.

Comment 74

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v7

This patch addresses most part of Dan's feedback. I just need to add the unit tests for FxOSConversationModel. I'll try to do it tomorrow.

Comment 75

[:shell escalante]

just tagged this bug with [loop-uplift], since this is targeted to uplift into Fx34 (so we can track the uplift bugs).  moved bugs off target milestone fx34 to fx35 as QA was validly asking if they should be looking for/testing a lot of bugs (some that were moved to [loop-uplift] and some postponed to Fx35 or later).

Comment 76

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #73)
> Thanks for the awesome review Dan! Sorry I couldn't get to this before.

No worries, I totally sympathize.  :-)

> > Could this eventListener be added to the iframe window object rather than
> > main window object?
> 
> The Marketplace iframe is doing a window.top.postMessage where if I am not
> wrong window.top is the embedder window and in this case the global window
> of the standalone UI.

That makes sense.  Maybe add a brief comment explaining?

> > 
> > As written, it would appear that we'll be adding a new listener on every
> > update.  If this is the right behavior, it would be helpful to add a comment
> > explaining why this makes sense.
> > 
> 
> This happens only once when we change the src property of the iframe. I'll
> add a comment about it.

A comment about this sounds like it would add to maintainability as well.

> > This is resolved now, so I'd suggest spinning out a new bug to handle the
> > audio-only case.
> > 
> 
> Great! I am just adding the fix here as it is quite simple. We have that
> info on the conversation model.

Excellent!

> 
> Removing the bind throws an error about this.setupOutgoingCall being
> undefined.

OK, my mistake.  :-)

> > @@ +332,5 @@
> > >            "https://www.mozilla.org/privacy/'>" + privacy_notice_name + "</a>"
> > >        });
> > >  
> > > +      var btnClassStartCall = "btn btn-large btn-accept " +
> > > +                              loop.shared.utils.getTargetPlatform();
> > 
> > I _think_ this is just a remnant of one of the merges, unless you added the
> > getTargetPlatform call in here.  btnClassStartCall is no longer used, so we
> > should get rid of this line, and if we need the target platform in the class
> > here, it would be helpful to understand how that's used.
> > 
> 
> I don't recall adding that :D.

OK, if you could then remove it in the next iteration, I'd very much appreaciate that.

> > @@ +664,5 @@
> > > +      // So far WebActivities are exposed only in FxOS, but they may be
> > > +      // exposed in Firefox Desktop soon, so we check for its existence
> > > +      // and also check if the UA belongs to a mobile platform.
> > > +      return !!window.MozActivity && /mobi/i.test(platform);
> > > +    },
> > 
> > WebActivities are also exposed in WebRT apps on Firefox for Android.  It
> > feels like these tests are really proxy tests for what we actually want to
> > know, which might be "Does this machine support web activities _and_ have a
> > loop-call activity registered with the system by someone other than us". 
> > Would making the code check for that be possible/better?
> > 
> 
> I am afraid that such a check is not possible. We can check for web
> activities support but we cannot know without triggering the activity if
> there is any handler for it. There is a way to check that, but it is allowed
> only to certified apps (bug 1017420).

So, presumably, that's only available to certified apps because there are probably security/privacy fingerprinting issues with unprivileged apps knowing the list of things available.  However, I suspect it would be reasonsable to create a more restricted API that could be made available to privileged apps or even web apps.  Specifically, all it would need to know is "will this activity be handled, or will it fail?" without exactly what's available.  If you agree, could file a spin off bug to create this API for the future?  If you disagree, I'd be interested in understanding why.

> > In the interest of landing sooner, I could live with an XXX comment and a
> > spinoff bug for handling that case.
> > 
> 
> Ok, will do that. Bug 1065403.

Thanks!

> > Part of this is to say that I think the original test that you extended
> > should just go away.  It's not testing anything that would be meaningful to
> > an API client.
> > 
> > As far as testing fxos:app-needed, that should be unit-testable by
> > triggering that event and then checking for the state change.
> > 
> 
> Makes sense. Done.

Again; thanks!
 
> > @@ +871,5 @@
> > > +      router.initiate("fakeToken");
> > > +
> > > +      sinon.assert.calledOnce(router.loadReactComponent);
> > > +      var model = router.loadReactComponent.getCall(0).args[0]._store.props
> > > +                        .model;
> > 
> > This feels fragile.  Perhaps the code under test needs to be factored
> > slightly to be more testable?
> > 
> 
> Sorry, but I really can't think about an alternative approach :(. Any
> suggestion?

Let me ponder this; I'll see what I can think of tomorrow.

Comment 77

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v7

Now with tests for FxOSConversationModel

Comment 78

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #76)
> 
> > > @@ +664,5 @@
> > > > +      // So far WebActivities are exposed only in FxOS, but they may be
> > > > +      // exposed in Firefox Desktop soon, so we check for its existence
> > > > +      // and also check if the UA belongs to a mobile platform.
> > > > +      return !!window.MozActivity && /mobi/i.test(platform);
> > > > +    },
> > > 
> > > WebActivities are also exposed in WebRT apps on Firefox for Android.  It
> > > feels like these tests are really proxy tests for what we actually want to
> > > know, which might be "Does this machine support web activities _and_ have a
> > > loop-call activity registered with the system by someone other than us". 
> > > Would making the code check for that be possible/better?
> > > 
> > 
> > I am afraid that such a check is not possible. We can check for web
> > activities support but we cannot know without triggering the activity if
> > there is any handler for it. There is a way to check that, but it is allowed
> > only to certified apps (bug 1017420).
> 
> So, presumably, that's only available to certified apps because there are
> probably security/privacy fingerprinting issues with unprivileged apps
> knowing the list of things available.  However, I suspect it would be
> reasonsable to create a more restricted API that could be made available to
> privileged apps or even web apps.  Specifically, all it would need to know
> is "will this activity be handled, or will it fail?" without exactly what's
> available.  If you agree, could file a spin off bug to create this API for
> the future?  If you disagree, I'd be interested in understanding why.
> 

I didn't really participate on the design of this API but I can also imagine that the reason for allowing it to certified apps only is because of security/privacy fingerprinting issues as you mentioned. Fabrice and/or Jonas would know in more detail.

Comment 79

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Error message

This is how the screen looks like after the Loop app installation fails (which should be quite a rare case). As you can see we show the error message and the call button is disabled.

Comment 80

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't know how to create an API which can answer the question "will this activity be handled" without also enabling that API to be used to answer the question "has facebook been installed" or "has twitter been installed".

Given how flexible WebActivities filtering system is, we can count on each app which handles activities to have slightly different filters. And given any difference in filtering, it will always be possible to write an activity which can be handled by one but not the other. So if we have an API for "will this activity be handled" you can always provide an activity which checks tells the install state of two apps apart.

Comment 81

[Darrin Henein [:darrin]]

Comment on attachment 8488049 [details]
Error message

Can we make the 'arrow' part of the call button look disabled as well?

Comment 82

[Dan Mosedale (:dmosedale, :dmose)]

OK, today I'm swamped, but I'll try and have a look at this patch tomorrow.

Comment 83

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Darrin Henein [:darrin] from comment #81)
> Comment on attachment 8488049 [details]
> Error message
> 
> Can we make the 'arrow' part of the call button look disabled as well?

Sure

Comment 84

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Loop standlone UI should hand off to FxOS Loop client, v8 (WIP)

Comment 85

[Dan Mosedale (:dmosedale, :dmose)]

Unfortunately, due to the delay in reviewing (sorry!), there's been substantial bit-rot.  I've rebased this patch against today's fx-team (the integration branch where the front-end folks do most of our work).

The tests are passing again, with the exception of a couple of l10n tests, which I haven't yet had time to figure out how best to fix both the tests and code itself.  I've ported the tests and code around the construction of StartConversationView from the (now-obsolete) WebappRouter to the new WebappRoot.

Besides the l10n cleanup mentioned above (which you're more than welcome to poke at), it still needs to be actually tried on FxOS, as well as comment 83 addressed.  Feel free to dig into those, or, if you don't it pick it up again tomorrow morning and keep driving it forward.

Comment 86

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Loop standlone UI should hand off to FxOS Loop client, v9

Comment 87

[Dan Mosedale (:dmosedale, :dmose)]

This latest patch is rebased against this morning's fx-team, fixes the unit tests, and implements the chevron disabling Darrin requested.  After sinking a couple of hours into configuring my phone configured so that I can test this there (and getting a lot closer!), I've decided to set that aside for a while and keep going on the code/review.

Comment 88

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8492394 [details] [diff] [review]
Loop standlone UI should hand off to FxOS Loop client, v9

Review of attachment 8492394 [details] [diff] [review]:
-----------------------------------------------------------------

More Monday...

::: browser/components/loop/test/standalone/webapp_test.js
@@ +753,1 @@
>        });

I refactored the above tests to only test API behaviors rather than implementation details.  Unfortunately, I forgot to remove the .skip, so that still needs to happen. :-)

@@ +753,4 @@
>        });
> +
> +
> +      it("should listen for fxos:app-needed events", function() {

The description here doesn't match what's being tested, which seems to be something like "should change internal state when fxos:app-needed is triggered on the conversation".  Reading http://facebook.github.io/react/docs/component-api.html#setstate also makes me suspect that this is racy.  Perhaps something along of firing the event, calling forceUpdate, and then checking the DOM Element to see if the iframe attributes are set correctly?

Comment 89

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Loop standlone UI should hand off to FxOS Loop client, v10

Thanks Dan, I've updated the tests with your latest comments.

Comment 90

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8493068 [details] [diff] [review]
Loop standlone UI should hand off to FxOS Loop client, v10

Review of attachment 8493068 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good -- almost there, I think!  I'll make the changes I can and then upload a new patch for you to review my changes and try them on out FxOS.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +174,5 @@
> +        if (event.target.error.name !== "NO_PROVIDER") {
> +          console.error ("Unexpected " + event.target.error.name);
> +          this.trigger("session:error", "fxos_app_needed", {
> +            fxosAppName: loop.config.fxosApp.name
> +          });

I don't think the notifications code supports parameter passing yet, but I chatted with Niko, and that should be easy to fix.  Since all the tests are passing and _onSessionError doesn't currently try to pass a parameter, I assume we're not testing this.  So we'll want to test this code as well as to test/fix the notification code.

@@ +379,5 @@
> +    _onFxOSAppNeeded: function() {
> +      this.setState({
> +        // XXX Update with the real marketplace url once the FxOS Loop app is
> +        //     uploaded to the marketplace bug 1053424
> +        marketplaceSrc: "http://fake-market.herokuapp.com/iframe-install.html"

Should this URL either be or be derived from something set as a loop.config variable?

@@ +520,5 @@
>            <ConversationFooter />
> +
> +          <FxOSHiddenMarketplace
> +            marketplaceSrc={this.state.marketplaceSrc}
> +            onMarketplaceMessage= {this.state.onMarketplaceMessage} />

Nit: extra space character after the =

::: browser/components/loop/standalone/server.js
@@ +18,5 @@
>      "loop.config.serverUrl = 'http://localhost:" + loopServerPort + "';" +
> +    "loop.config.pendingCallTimeout = 20000;" +
> +    "loop.config.fxosApp = loop.config.fxosApp || {};" +
> +    "loop.config.fxosApp.name = 'Loop';" +
> +    "loop.config.fxosApp.manifestUrl = 'http://fake-market.herokuapp.com/apps/packagedApp/manifest.webapp'"

These also need to be added to the Makefile for the non-dynamic configuration.  (Yeah, yuck, bug 1069872 will fix this).

::: browser/components/loop/test/standalone/webapp_test.js
@@ +61,5 @@
> +
> +        sinon.assert.calledOnce(React.renderComponent);
> +        sinon.assert.calledWith(React.renderComponent,
> +          sinon.match(function(value) {
> +            console.error("convo: ", value._store.props.conversation);

Whoops, I must have left this in here.  It can go.  :-)

@@ +408,5 @@
>  
>                sinon.assert.notCalled(ocView._websocket.mediaUp);
>              });
>  
> +          it("should notify tloadhe websocket that media is up if both streams" +

This changes looks inadvertent; let's ditch it.

@@ +821,5 @@
>          tos = view.getDOMNode().querySelector(".terms-service");
>  
>          expect(tos.classList.contains("hide")).to.equal(true);
>        });
> +

Inadvertent, presumably.

@@ +923,5 @@
> +      sandbox.stub(client, "requestCallInfo");
> +      conversation = new sharedModels.ConversationModel({}, {
> +        sdk: {},
> +        pendingCallTimeout: 1000
> +      });

pendingCallTimeout is no longer a client-side thing, as of this morning, so we can get rid of that.  Moreover, though, I suspect we can get rid of the stub conversation entirely here, since we shouldn't be using the sharedModel.ConversationModel thing in this code at all.

@@ +956,5 @@
> +        var button = view.getDOMNode().querySelector("button");
> +        React.addons.TestUtils.Simulate.click(button);
> +
> +        sinon.assert.calledOnce(setupOutgoingCall);
> +      });

This seems like an integration test, not a unit test.  I suspect this wants to be hoisted to the StartConversationView unit tests and changed to test something slightly different (ie that it does whatever is required for the FxConversationModel to make that call later).

@@ +1014,5 @@
> +          trigger = sandbox.stub(model, "trigger");
> +        });
> +
> +        afterEach(function() {
> +          trigger.restore();

This shouldn't be necessary, I don't think, because you've used sandbox.stub, and at the top of the file, sandbox.restore() is called after every ttest.

@@ +1018,5 @@
> +          trigger.restore();
> +        });
> +
> +        it("Activity properties", function() {
> +          expect(_activityProps).to.not.exist;

Our local convention, when practical to separate the "setup", "execution", "verification" steps of our unit tests with blank lines to make the tests a bit more readable.

@@ +1021,5 @@
> +        it("Activity properties", function() {
> +          expect(_activityProps).to.not.exist;
> +          model.setupOutgoingCall();
> +          expect(_activityProps).to.exist;
> +          expect(_activityProps).eql({

I suspect that this really wants to read .to.deep.equal so that the whole JS object gets checked.

@@ +1042,5 @@
> +            sinon.assert.calledWithExactly(trigger, "fxos:app-needed");
> +          }
> +        );
> +
> +        it("Other activity error should trigger session:error",

"An unrecognized activity error" would make it a bit more expicit that it's not referring to an error named "Other".

@@ +1074,5 @@
> +        });
> +
> +        afterEach(function() {
> +          setupOutgoingCall.restore();
> +          trigger.restore();

Both of these should be already taken care of by the sandbox.restore.

@@ +1078,5 @@
> +          trigger.restore();
> +        });
> +
> +        it("We should call trigger a FxOS outgoing call if we get " +
> +           "install-package message without error", function() {

The "it" here refers to onMarketplaceMessage, the unit of code under test.  So a clearer description could read "should call setupOutgoingCall when given an install-package message without error".

@@ +1089,5 @@
> +          sinon.assert.calledOnce(setupOutgoingCall);
> +        });
> +
> +        it("We should trigger a session:error event if we get " +
> +           "install-package message with an error", function() {

Similar test naming suggestion to the previous test.

Comment 91

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Loop standalone UI should handoff to FxOS client, v11

Comment 92

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8493068 [details] [diff] [review]
Loop standlone UI should hand off to FxOS Loop client, v10

Review of attachment 8493068 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/standalone/server.js
@@ +18,5 @@
>      "loop.config.serverUrl = 'http://localhost:" + loopServerPort + "';" +
> +    "loop.config.pendingCallTimeout = 20000;" +
> +    "loop.config.fxosApp = loop.config.fxosApp || {};" +
> +    "loop.config.fxosApp.name = 'Loop';" +
> +    "loop.config.fxosApp.manifestUrl = 'http://fake-market.herokuapp.com/apps/packagedApp/manifest.webapp'"

Done.  When we land this, we'll need to notify ops of these changes too so that the production instances get this stuff as well. (Yes, this triple-sourcing of the same data is nuts, we need to fix it :-).

::: browser/components/loop/test/standalone/webapp_test.js
@@ +956,5 @@
> +        var button = view.getDOMNode().querySelector("button");
> +        React.addons.TestUtils.Simulate.click(button);
> +
> +        sinon.assert.calledOnce(setupOutgoingCall);
> +      });

There are already a bunch of StartConversationView unit tests that verify that the model's setupOutgoingCall is called when the button is pressed under a variety of circumstances, so I think this test is probably a duplicate that we don't want to have to maintain, so I'm going remove it.

@@ +1014,5 @@
> +          trigger = sandbox.stub(model, "trigger");
> +        });
> +
> +        afterEach(function() {
> +          trigger.restore();

This has turned out to be true, so I've also gone and removed a bunch of the initial condition assertions that stubs haven't previously been called, and use of the sandbox takes care of this.

@@ +1021,5 @@
> +        it("Activity properties", function() {
> +          expect(_activityProps).to.not.exist;
> +          model.setupOutgoingCall();
> +          expect(_activityProps).to.exist;
> +          expect(_activityProps).eql({

Interestingly, the code did seem to be detecting the right stuff without the deep.equal bits, but I've switched it anyway so that it's more explicit

Comment 93

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8493279 [details] [diff] [review]
Loop standalone UI should handoff to FxOS client, v11

># HG changeset patch
># User Dan Mosedale <[e-mail]>
>
>Bug 1008990 - Loop standalone UI should handoff to FxOS client, r=dmose
>
>diff --git a/browser/components/loop/standalone/Makefile b/browser/components/loop/standalone/Makefile
>index 4992a91..ceabf93 100644
>--- a/browser/components/loop/standalone/Makefile
>+++ b/browser/components/loop/standalone/Makefile
>@@ -68,8 +68,11 @@ remove_old_config:
> # working with that deployment.
> .PHONY: config
> config:
> 	@echo "var loop = loop || {};" > content/config.js
> 	@echo "loop.config = loop.config || {};" >> content/config.js
> 	@echo "loop.config.serverUrl = '`echo $(LOOP_SERVER_URL)`';" >> content/config.js
> 	@echo "loop.config.feedbackApiUrl = '`echo $(LOOP_FEEDBACK_API_URL)`';" >> content/config.js
> 	@echo "loop.config.feedbackProductName = '`echo $(LOOP_FEEDBACK_PRODUCT_NAME)`';" >> content/config.js
>+	@echo "loop.config.fxosApp = loop.config.fxosApp || {};",
>+	@echo "loop.config.fxosApp.name = 'Loop';",
>+	@echo "loop.config.fxosApp.manifestUrl = 'http://fake-market.herokuapp.com/apps/packagedApp/manifest.webapp'"
>diff --git a/browser/components/loop/standalone/content/js/webapp.js b/browser/components/loop/standalone/content/js/webapp.js
>index fffb829..584dad7 100644
>--- a/browser/components/loop/standalone/content/js/webapp.js
>+++ b/browser/components/loop/standalone/content/js/webapp.js
>@@ -1,15 +1,15 @@
> /** @jsx React.DOM */
> 
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>-/* global loop:true, React */
>+/* global loop:true, React, MozActivity */
> /* jshint newcap:false, maxlen:false */
> 
> var loop = loop || {};
> loop.webapp = (function($, _, OT, mozL10n) {
>   "use strict";
> 
>   loop.config = loop.config || {};
>   loop.config.serverUrl = loop.config.serverUrl || "http://localhost:5000";
>@@ -116,16 +116,105 @@ loop.webapp = (function($, _, OT, mozL10n) {
>       return (
>         React.DOM.h1({className: "standalone-header-title"}, 
>           React.DOM.strong(null, mozL10n.get("brandShortname")), " ", mozL10n.get("clientShortname")
>         )
>       );
>     }
>   });
> 
>+  /**
>+   * The Firefox Marketplace exposes a web page that contains a postMesssage
>+   * based API that wraps a small set of functionality from the WebApps API
>+   * that allow us to request the installation of apps given their manifest
>+   * URL. We will be embedding the content of this web page within an hidden
>+   * iframe in case that we need to request the installation of the FxOS Loop
>+   * client.
>+   */
>+  var FxOSHiddenMarketplace = React.createClass({displayName: 'FxOSHiddenMarketplace',
>+    render: function() {
>+      return React.DOM.iframe({id: "marketplace", src: this.props.marketplaceSrc, hidden: true});
>+    },
>+
>+    componentDidUpdate: function() {
>+      // This happens only once when we change the 'src' property of the iframe.
>+      if (this.props.onMarketplaceMessage) {
>+        // The reason for listening on the global window instead of on the
>+        // iframe content window is because the Marketplace is doing a
>+        // window.top.postMessage.
>+        window.addEventListener("message", this.props.onMarketplaceMessage);
>+      }
>+    }
>+  });
>+
>+  var FxOSConversationModel = Backbone.Model.extend({
>+    setupOutgoingCall: function() {
>+      // The FxOS Loop client exposes a "loop-call" activity. If we get the
>+      // activity onerror callback it means that there is no "loop-call"
>+      // activity handler available and so no FxOS Loop client installed.
>+      var request = new MozActivity({
>+        name: "loop-call",
>+        data: {
>+          type: "loop/token",
>+          token: this.get("loopToken"),
>+          callerId: this.get("callerId"),
>+          callType: this.get("callType")
>+        }
>+      });
>+
>+      request.onsuccess = function() {};
>+
>+      request.onerror = (function(event) {
>+        if (event.target.error.name !== "NO_PROVIDER") {
>+          console.error ("Unexpected " + event.target.error.name);
>+          this.trigger("session:error", "fxos_app_needed", {
>+            fxosAppName: loop.config.fxosApp.name
>+          });
>+          return;
>+        }
>+        this.trigger("fxos:app-needed");
>+      }).bind(this);
>+    },
>+
>+    onMarketplaceMessage: function(event) {
>+      var message = event.data;
>+      switch (message.name) {
>+        case "loaded":
>+          var marketplace = window.document.getElementById("marketplace");
>+          // Once we have it loaded, we request the installation of the FxOS
>+          // Loop client app. We will be receiving the result of this action
>+          // via postMessage from the child iframe.
>+          marketplace.contentWindow.postMessage({
>+            "name": "install-package",
>+            "data": {
>+              "product": {
>+                "name": loop.config.fxosApp.name,
>+                "manifest_url": loop.config.fxosApp.manifestUrl,
>+                "is_packaged": true
>+              }
>+            }
>+          }, "*");
>+          break;
>+        case "install-package":
>+          window.removeEventListener("message", this.onMarketplaceMessage);
>+          if (message.error) {
>+            console.error(message.error.error);
>+            this.trigger("session:error", "fxos_app_needed", {
>+              fxosAppName: loop.config.fxosApp.name
>+            });
>+            return;
>+          }
>+          // We installed the FxOS app \o/, so we can continue with the call
>+          // process.
>+          this.setupOutgoingCall();
>+          break;
>+      }
>+    }
>+  });
>+
>   var ConversationHeader = React.createClass({displayName: 'ConversationHeader',
>     render: function() {
>       var cx = React.addons.classSet;
>       var conversationUrl = location.href;
> 
>       var urlCreationDateClasses = cx({
>         "light-color-font": true,
>         "call-url-date": true, /* Used as a handler in the tests */
>@@ -227,18 +316,20 @@ loop.webapp = (function($, _, OT, mozL10n) {
>    * as a `model` property.
>    *
>    * Required properties:
>    * - {loop.shared.models.ConversationModel}    model    Conversation model.
>    * - {loop.shared.models.NotificationCollection} notifications
>    */
>   var StartConversationView = React.createClass({displayName: 'StartConversationView',
>     propTypes: {
>-      model: React.PropTypes.instanceOf(sharedModels.ConversationModel)
>-                                       .isRequired,
>+      model: React.PropTypes.oneOfType([
>+               React.PropTypes.instanceOf(sharedModels.ConversationModel),
>+               React.PropTypes.instanceOf(FxOSConversationModel)
>+             ]).isRequired,
>       // XXX Check more tightly here when we start injecting window.loop.*
>       notifications: React.PropTypes.object.isRequired,
>       client: React.PropTypes.object.isRequired
>     },
> 
>     getDefaultProps: function() {
>       return {showCallOptionsMenu: false};
>     },
>@@ -251,22 +342,39 @@ loop.webapp = (function($, _, OT, mozL10n) {
>       };
>     },
> 
>     componentDidMount: function() {
>       // Listen for events & hide dropdown menu if user clicks away
>       window.addEventListener("click", this.clickHandler);
>       this.props.model.listenTo(this.props.model, "session:error",
>                                 this._onSessionError);
>+      this.props.model.listenTo(this.props.model, "fxos:app-needed",
>+                                this._onFxOSAppNeeded);
>       this.props.client.requestCallUrlInfo(this.props.model.get("loopToken"),
>                                            this._setConversationTimestamp);
>     },
> 
>     _onSessionError: function(error) {
>-      console.error(error);
>+      var errorL10n = error || "unable_retrieve_call_info";
>+      this.props.notifications.errorL10n(errorL10n);
>+      console.error(errorL10n);
>+    },
>+
>+    _onFxOSAppNeeded: function() {
>+      this.setState({
>+        // XXX Update with the real marketplace url once the FxOS Loop app is
>+        //     uploaded to the marketplace bug 1053424
>+        marketplaceSrc: "http://fake-market.herokuapp.com/iframe-install.html"
>+      });
>+      this.setState({
>+        onMarketplaceMessage: this.props.model.onMarketplaceMessage.bind(
>+          this.props.model
>+        )
>+      });
>       this.props.notifications.errorL10n("unable_retrieve_call_info");
>     },
> 
>     /**
>      * Initiates the call.
>      * Takes in a call type parameter "audio" or "audio-video" and returns
>      * a function that initiates the call. React click handler requires a function
>      * to be called when that event happenes.
>@@ -324,16 +432,20 @@ loop.webapp = (function($, _, OT, mozL10n) {
>         "native-dropdown-large-parent": true,
>         "standalone-dropdown-menu": true,
>         "visually-hidden": !this.state.showCallOptionsMenu
>       });
>       var tosClasses = React.addons.classSet({
>         "terms-service": true,
>         hide: (localStorage.getItem("has-seen-tos") === "true")
>       });
>+      var chevronClasses = React.addons.classSet({
>+        "btn-chevron": true,
>+        "disabled": this.state.disableCallButton
>+      });
> 
>       return (
>         React.DOM.div({className: "container"}, 
>           React.DOM.div({className: "container-box"}, 
> 
>             ConversationHeader({
>               urlCreationDateString: this.state.urlCreationDateString}), 
> 
>@@ -354,17 +466,17 @@ loop.webapp = (function($, _, OT, mozL10n) {
>                             disabled: this.state.disableCallButton, 
>                             title: mozL10n.get("initiate_audio_video_call_tooltip2")}, 
>                       React.DOM.span({className: "standalone-call-btn-text"}, 
>                         mozL10n.get("initiate_audio_video_call_button2")
>                       ), 
>                       React.DOM.span({className: "standalone-call-btn-video-icon"})
>                     ), 
> 
>-                    React.DOM.div({className: "btn-chevron", 
>+                    React.DOM.div({className: chevronClasses, 
>                          onClick: this._toggleCallOptionsMenu}
>                     )
> 
>                   ), 
> 
>                   React.DOM.ul({className: dropdownMenuClasses}, 
>                     React.DOM.li(null, 
>                       /*
>@@ -382,17 +494,21 @@ loop.webapp = (function($, _, OT, mozL10n) {
>               ), 
>               React.DOM.div({className: "flex-padding-1"})
>             ), 
> 
>             React.DOM.p({className: tosClasses, 
>                dangerouslySetInnerHTML: {__html: tosHTML}})
>           ), 
> 
>-          ConversationFooter(null)
>+          ConversationFooter(null), 
>+
>+          FxOSHiddenMarketplace({
>+            marketplaceSrc: this.state.marketplaceSrc, 
>+            onMarketplaceMessage: this.state.onMarketplaceMessage})
>         )
>       );
>     }
>   });
> 
>   /**
>    * Ended conversation view.
>    */
>@@ -733,16 +849,25 @@ loop.webapp = (function($, _, OT, mozL10n) {
>     this._iOSRegex = /^(iPad|iPhone|iPod)/;
>   }
> 
>   WebappHelper.prototype = {
>     isFirefox: function(platform) {
>       return platform.indexOf("Firefox") !== -1;
>     },
> 
>+    isFirefoxOS: function(platform) {
>+      // So far WebActivities are exposed only in FxOS, but they may be
>+      // exposed in Firefox Desktop soon, so we check for its existence
>+      // and also check if the UA belongs to a mobile platform.
>+      // XXX WebActivities are also exposed in WebRT on Firefox for Android,
>+      //     so we need a better check. Bug 1065403.
>+      return !!window.MozActivity && /mobi/i.test(platform);
>+    },
>+
>     isIOS: function(platform) {
>       return this._iOSRegex.test(platform);
>     },
> 
>     locationHash: function() {
>       return window.location.hash;
>     }
>   };
>@@ -751,19 +876,24 @@ loop.webapp = (function($, _, OT, mozL10n) {
>    * App initialization.
>    */
>   function init() {
>     var helper = new WebappHelper();
>     var client = new loop.StandaloneClient({
>       baseServerUrl: loop.config.serverUrl
>     });
>     var notifications = new sharedModels.NotificationCollection();
>-    var conversation = new sharedModels.ConversationModel({}, {
>-      sdk: OT
>-    });
>+    var conversation
>+    if (helper.isFirefoxOS()) {
>+      conversation = new FxOSConversationModel();
>+    } else {
>+      conversation = new sharedModels.ConversationModel({}, {
>+        sdk: OT
>+      });
>+    }
>     var feedbackApiClient = new loop.FeedbackAPIClient(
>       loop.config.feedbackApiUrl, {
>         product: loop.config.feedbackProductName,
>         user_agent: navigator.userAgent,
>         url: document.location.origin
>       });
> 
>     // Obtain the loopToken and pass it to the conversation
>@@ -793,11 +923,12 @@ loop.webapp = (function($, _, OT, mozL10n) {
>     OutgoingConversationView: OutgoingConversationView,
>     EndedConversationView: EndedConversationView,
>     HomeView: HomeView,
>     UnsupportedBrowserView: UnsupportedBrowserView,
>     UnsupportedDeviceView: UnsupportedDeviceView,
>     init: init,
>     PromoteFirefoxView: PromoteFirefoxView,
>     WebappHelper: WebappHelper,
>+    FxOSConversationModel: FxOSConversationModel,
>     WebappRootView: WebappRootView
>   };
> })(jQuery, _, window.OT, navigator.mozL10n);
>diff --git a/browser/components/loop/standalone/content/js/webapp.jsx b/browser/components/loop/standalone/content/js/webapp.jsx
>index 0630e82..77bfa8c 100644
>--- a/browser/components/loop/standalone/content/js/webapp.jsx
>+++ b/browser/components/loop/standalone/content/js/webapp.jsx
>@@ -1,15 +1,15 @@
> /** @jsx React.DOM */
> 
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>-/* global loop:true, React */
>+/* global loop:true, React, MozActivity */
> /* jshint newcap:false, maxlen:false */
> 
> var loop = loop || {};
> loop.webapp = (function($, _, OT, mozL10n) {
>   "use strict";
> 
>   loop.config = loop.config || {};
>   loop.config.serverUrl = loop.config.serverUrl || "http://localhost:5000";
>@@ -116,16 +116,105 @@ loop.webapp = (function($, _, OT, mozL10n) {
>       return (
>         <h1 className="standalone-header-title">
>           <strong>{mozL10n.get("brandShortname")}</strong> {mozL10n.get("clientShortname")}
>         </h1>
>       );
>     }
>   });
> 
>+  /**
>+   * The Firefox Marketplace exposes a web page that contains a postMesssage
>+   * based API that wraps a small set of functionality from the WebApps API
>+   * that allow us to request the installation of apps given their manifest
>+   * URL. We will be embedding the content of this web page within an hidden
>+   * iframe in case that we need to request the installation of the FxOS Loop
>+   * client.
>+   */
>+  var FxOSHiddenMarketplace = React.createClass({
>+    render: function() {
>+      return <iframe id="marketplace" src={this.props.marketplaceSrc} hidden/>;
>+    },
>+
>+    componentDidUpdate: function() {
>+      // This happens only once when we change the 'src' property of the iframe.
>+      if (this.props.onMarketplaceMessage) {
>+        // The reason for listening on the global window instead of on the
>+        // iframe content window is because the Marketplace is doing a
>+        // window.top.postMessage.
>+        window.addEventListener("message", this.props.onMarketplaceMessage);
>+      }
>+    }
>+  });
>+
>+  var FxOSConversationModel = Backbone.Model.extend({
>+    setupOutgoingCall: function() {
>+      // The FxOS Loop client exposes a "loop-call" activity. If we get the
>+      // activity onerror callback it means that there is no "loop-call"
>+      // activity handler available and so no FxOS Loop client installed.
>+      var request = new MozActivity({
>+        name: "loop-call",
>+        data: {
>+          type: "loop/token",
>+          token: this.get("loopToken"),
>+          callerId: this.get("callerId"),
>+          callType: this.get("callType")
>+        }
>+      });
>+
>+      request.onsuccess = function() {};
>+
>+      request.onerror = (function(event) {
>+        if (event.target.error.name !== "NO_PROVIDER") {
>+          console.error ("Unexpected " + event.target.error.name);
>+          this.trigger("session:error", "fxos_app_needed", {
>+            fxosAppName: loop.config.fxosApp.name
>+          });
>+          return;
>+        }
>+        this.trigger("fxos:app-needed");
>+      }).bind(this);
>+    },
>+
>+    onMarketplaceMessage: function(event) {
>+      var message = event.data;
>+      switch (message.name) {
>+        case "loaded":
>+          var marketplace = window.document.getElementById("marketplace");
>+          // Once we have it loaded, we request the installation of the FxOS
>+          // Loop client app. We will be receiving the result of this action
>+          // via postMessage from the child iframe.
>+          marketplace.contentWindow.postMessage({
>+            "name": "install-package",
>+            "data": {
>+              "product": {
>+                "name": loop.config.fxosApp.name,
>+                "manifest_url": loop.config.fxosApp.manifestUrl,
>+                "is_packaged": true
>+              }
>+            }
>+          }, "*");
>+          break;
>+        case "install-package":
>+          window.removeEventListener("message", this.onMarketplaceMessage);
>+          if (message.error) {
>+            console.error(message.error.error);
>+            this.trigger("session:error", "fxos_app_needed", {
>+              fxosAppName: loop.config.fxosApp.name
>+            });
>+            return;
>+          }
>+          // We installed the FxOS app \o/, so we can continue with the call
>+          // process.
>+          this.setupOutgoingCall();
>+          break;
>+      }
>+    }
>+  });
>+
>   var ConversationHeader = React.createClass({
>     render: function() {
>       var cx = React.addons.classSet;
>       var conversationUrl = location.href;
> 
>       var urlCreationDateClasses = cx({
>         "light-color-font": true,
>         "call-url-date": true, /* Used as a handler in the tests */
>@@ -227,18 +316,20 @@ loop.webapp = (function($, _, OT, mozL10n) {
>    * as a `model` property.
>    *
>    * Required properties:
>    * - {loop.shared.models.ConversationModel}    model    Conversation model.
>    * - {loop.shared.models.NotificationCollection} notifications
>    */
>   var StartConversationView = React.createClass({
>     propTypes: {
>-      model: React.PropTypes.instanceOf(sharedModels.ConversationModel)
>-                                       .isRequired,
>+      model: React.PropTypes.oneOfType([
>+               React.PropTypes.instanceOf(sharedModels.ConversationModel),
>+               React.PropTypes.instanceOf(FxOSConversationModel)
>+             ]).isRequired,
>       // XXX Check more tightly here when we start injecting window.loop.*
>       notifications: React.PropTypes.object.isRequired,
>       client: React.PropTypes.object.isRequired
>     },
> 
>     getDefaultProps: function() {
>       return {showCallOptionsMenu: false};
>     },
>@@ -251,22 +342,39 @@ loop.webapp = (function($, _, OT, mozL10n) {
>       };
>     },
> 
>     componentDidMount: function() {
>       // Listen for events & hide dropdown menu if user clicks away
>       window.addEventListener("click", this.clickHandler);
>       this.props.model.listenTo(this.props.model, "session:error",
>                                 this._onSessionError);
>+      this.props.model.listenTo(this.props.model, "fxos:app-needed",
>+                                this._onFxOSAppNeeded);
>       this.props.client.requestCallUrlInfo(this.props.model.get("loopToken"),
>                                            this._setConversationTimestamp);
>     },
> 
>     _onSessionError: function(error) {
>-      console.error(error);
>+      var errorL10n = error || "unable_retrieve_call_info";
>+      this.props.notifications.errorL10n(errorL10n);
>+      console.error(errorL10n);
>+    },
>+
>+    _onFxOSAppNeeded: function() {
>+      this.setState({
>+        // XXX Update with the real marketplace url once the FxOS Loop app is
>+        //     uploaded to the marketplace bug 1053424
>+        marketplaceSrc: "http://fake-market.herokuapp.com/iframe-install.html"
>+      });
>+      this.setState({
>+        onMarketplaceMessage: this.props.model.onMarketplaceMessage.bind(
>+          this.props.model
>+        )
>+      });
>       this.props.notifications.errorL10n("unable_retrieve_call_info");
>     },
> 
>     /**
>      * Initiates the call.
>      * Takes in a call type parameter "audio" or "audio-video" and returns
>      * a function that initiates the call. React click handler requires a function
>      * to be called when that event happenes.
>@@ -324,16 +432,20 @@ loop.webapp = (function($, _, OT, mozL10n) {
>         "native-dropdown-large-parent": true,
>         "standalone-dropdown-menu": true,
>         "visually-hidden": !this.state.showCallOptionsMenu
>       });
>       var tosClasses = React.addons.classSet({
>         "terms-service": true,
>         hide: (localStorage.getItem("has-seen-tos") === "true")
>       });
>+      var chevronClasses = React.addons.classSet({
>+        "btn-chevron": true,
>+        "disabled": this.state.disableCallButton
>+      });
> 
>       return (
>         <div className="container">
>           <div className="container-box">
> 
>             <ConversationHeader
>               urlCreationDateString={this.state.urlCreationDateString} />
> 
>@@ -354,17 +466,17 @@ loop.webapp = (function($, _, OT, mozL10n) {
>                             disabled={this.state.disableCallButton}
>                             title={mozL10n.get("initiate_audio_video_call_tooltip2")} >
>                       <span className="standalone-call-btn-text">
>                         {mozL10n.get("initiate_audio_video_call_button2")}
>                       </span>
>                       <span className="standalone-call-btn-video-icon"></span>
>                     </button>
> 
>-                    <div className="btn-chevron"
>+                    <div className={chevronClasses}
>                          onClick={this._toggleCallOptionsMenu}>
>                     </div>
> 
>                   </div>
> 
>                   <ul className={dropdownMenuClasses}>
>                     <li>
>                       {/*
>@@ -383,16 +495,20 @@ loop.webapp = (function($, _, OT, mozL10n) {
>               <div className="flex-padding-1"></div>
>             </div>
> 
>             <p className={tosClasses}
>                dangerouslySetInnerHTML={{__html: tosHTML}}></p>
>           </div>
> 
>           <ConversationFooter />
>+
>+          <FxOSHiddenMarketplace
>+            marketplaceSrc={this.state.marketplaceSrc}
>+            onMarketplaceMessage={this.state.onMarketplaceMessage} />
>         </div>
>       );
>     }
>   });
> 
>   /**
>    * Ended conversation view.
>    */
>@@ -733,16 +849,25 @@ loop.webapp = (function($, _, OT, mozL10n) {
>     this._iOSRegex = /^(iPad|iPhone|iPod)/;
>   }
> 
>   WebappHelper.prototype = {
>     isFirefox: function(platform) {
>       return platform.indexOf("Firefox") !== -1;
>     },
> 
>+    isFirefoxOS: function(platform) {
>+      // So far WebActivities are exposed only in FxOS, but they may be
>+      // exposed in Firefox Desktop soon, so we check for its existence
>+      // and also check if the UA belongs to a mobile platform.
>+      // XXX WebActivities are also exposed in WebRT on Firefox for Android,
>+      //     so we need a better check. Bug 1065403.
>+      return !!window.MozActivity && /mobi/i.test(platform);
>+    },
>+
>     isIOS: function(platform) {
>       return this._iOSRegex.test(platform);
>     },
> 
>     locationHash: function() {
>       return window.location.hash;
>     }
>   };
>@@ -751,19 +876,24 @@ loop.webapp = (function($, _, OT, mozL10n) {
>    * App initialization.
>    */
>   function init() {
>     var helper = new WebappHelper();
>     var client = new loop.StandaloneClient({
>       baseServerUrl: loop.config.serverUrl
>     });
>     var notifications = new sharedModels.NotificationCollection();
>-    var conversation = new sharedModels.ConversationModel({}, {
>-      sdk: OT
>-    });
>+    var conversation
>+    if (helper.isFirefoxOS()) {
>+      conversation = new FxOSConversationModel();
>+    } else {
>+      conversation = new sharedModels.ConversationModel({}, {
>+        sdk: OT
>+      });
>+    }
>     var feedbackApiClient = new loop.FeedbackAPIClient(
>       loop.config.feedbackApiUrl, {
>         product: loop.config.feedbackProductName,
>         user_agent: navigator.userAgent,
>         url: document.location.origin
>       });
> 
>     // Obtain the loopToken and pass it to the conversation
>@@ -793,11 +923,12 @@ loop.webapp = (function($, _, OT, mozL10n) {
>     OutgoingConversationView: OutgoingConversationView,
>     EndedConversationView: EndedConversationView,
>     HomeView: HomeView,
>     UnsupportedBrowserView: UnsupportedBrowserView,
>     UnsupportedDeviceView: UnsupportedDeviceView,
>     init: init,
>     PromoteFirefoxView: PromoteFirefoxView,
>     WebappHelper: WebappHelper,
>+    FxOSConversationModel: FxOSConversationModel,
>     WebappRootView: WebappRootView
>   };
> })(jQuery, _, window.OT, navigator.mozL10n);
>diff --git a/browser/components/loop/standalone/content/l10n/loop.en-US.properties b/browser/components/loop/standalone/content/l10n/loop.en-US.properties
>index 64c326c..ca9d34d 100644
>--- a/browser/components/loop/standalone/content/l10n/loop.en-US.properties
>+++ b/browser/components/loop/standalone/content/l10n/loop.en-US.properties
>@@ -41,16 +41,17 @@ legal_text_and_links=By using this product you agree to the {{terms_of_use_url}}
> terms_of_use_link_text=Terms of use
> privacy_notice_link_text=Privacy notice
> brandShortname=Firefox
> clientShortname=WebRTC!
> ## LOCALIZATION NOTE (call_url_creation_date_label): Example output: (from May 26, 2014)
> call_url_creation_date_label=(from {{call_url_creation_date}})
> call_progress_connecting_description=Connectingâ¦
> call_progress_ringing_description=Ringingâ¦
>+fxos_app_needed=Please install the {{fxosAppName}} app from the Firefox Marketplace.
> 
> feedback_call_experience_heading2=How was your conversation?
> feedback_what_makes_you_sad=What makes you sad?
> feedback_thank_you_heading=Thank you for your feedback!
> feedback_category_audio_quality=Audio quality
> feedback_category_video_quality=Video quality
> feedback_category_was_disconnected=Was disconnected
> feedback_category_confusing=Confusing
>diff --git a/browser/components/loop/standalone/server.js b/browser/components/loop/standalone/server.js
>index d3bf4c0..d80d3e8 100644
>--- a/browser/components/loop/standalone/server.js
>+++ b/browser/components/loop/standalone/server.js
>@@ -16,16 +16,19 @@ function getConfigFile(req, res) {
> 
>   res.set('Content-Type', 'text/javascript');
>   res.send([
>     "var loop = loop || {};",
>     "loop.config = loop.config || {};",
>     "loop.config.serverUrl = 'http://localhost:" + loopServerPort + "';",
>     "loop.config.feedbackApiUrl = '" + feedbackApiUrl + "';",
>     "loop.config.feedbackProductName = '" + feedbackProductName + "';",
>+    "loop.config.fxosApp = loop.config.fxosApp || {};",
>+    "loop.config.fxosApp.name = 'Loop';",
>+    "loop.config.fxosApp.manifestUrl = 'http://fake-market.herokuapp.com/apps/packagedApp/manifest.webapp'"
>   ].join("\n"));
> }
> 
> app.get('/content/config.js', getConfigFile);
> 
> // This lets /test/ be mapped to the right place for running tests
> app.use('/', express.static(__dirname + '/../'));
> 
>diff --git a/browser/components/loop/test/standalone/webapp_test.js b/browser/components/loop/test/standalone/webapp_test.js
>index 52bc57e..590afa3 100644
>--- a/browser/components/loop/test/standalone/webapp_test.js
>+++ b/browser/components/loop/test/standalone/webapp_test.js
>@@ -52,16 +52,47 @@ describe("loop.webapp", function() {
>     });
> 
>     it("should set the loopToken on the conversation", function() {
>       loop.webapp.init();
> 
>        sinon.assert.called(conversationSetStub);
>        sinon.assert.calledWithExactly(conversationSetStub, "loopToken", "fake-Token");
>     });
>+
>+    it("should init the WebappRootView with a ConversationModel on non-FxOS",
>+      function () {
>+        sandbox.stub(loop.webapp.WebappHelper.prototype, "isFirefoxOS").returns(false);
>+
>+        loop.webapp.init();
>+
>+        sinon.assert.calledOnce(React.renderComponent);
>+        sinon.assert.calledWith(React.renderComponent,
>+          sinon.match(function(value) {
>+            return value._store.props.conversation instanceof
>+              sharedModels.ConversationModel;
>+          })
>+        );
>+      });
>+
>+    it("should create the WebappRootView with an FxOSConversationModel on FxOS",
>+      function() {
>+        sandbox.stub(loop.webapp.WebappHelper.prototype, "isFirefoxOS").returns(true);
>+
>+        loop.webapp.init();
>+
>+        sinon.assert.calledOnce(React.renderComponent);
>+        sinon.assert.calledWith(React.renderComponent,
>+          sinon.match(function(value) {
>+            return value._store.props.conversation instanceof
>+              loop.webapp.FxOSConversationModel;
>+          })
>+        );
>+      });
>+
>   });
> 
>   describe("OutgoingConversationView", function() {
>     var ocView, conversation, client;
> 
>     function mountTestComponent(props) {
>       return TestUtils.renderIntoDocument(
>         loop.webapp.OutgoingConversationView(props));
>@@ -685,17 +716,18 @@ describe("loop.webapp", function() {
> 
>       beforeEach(function() {
>         conversation = new sharedModels.ConversationModel({
>           loopToken: "fake"
>         }, {
>           sdk: {}
>         });
> 
>-        sandbox.spy(conversation, "listenTo");
>+        conversation.onMarketplaceMessage = function() {};
>+        sandbox.stub(notifications, "errorL10n");
>         requestCallUrlInfo = sandbox.stub();
> 
>         view = React.addons.TestUtils.renderIntoDocument(
>             loop.webapp.StartConversationView({
>               model: conversation,
>               notifications: notifications,
>               client: {requestCallUrlInfo: requestCallUrlInfo}
>             })
>@@ -704,31 +736,50 @@ describe("loop.webapp", function() {
> 
>       it("should call requestCallUrlInfo", function() {
>         sinon.assert.calledOnce(requestCallUrlInfo);
>         sinon.assert.calledWithExactly(requestCallUrlInfo,
>                                        sinon.match.string,
>                                        sinon.match.func);
>       });
> 
>-      it("should listen for session:error events", function() {
>-        sinon.assert.calledOnce(conversation.listenTo);
>-        sinon.assert.calledWithExactly(conversation.listenTo, conversation,
>-                                       "session:error", sinon.match.func);
>+      it("should add a notification when a session:error model event is " +
>+         " received without an argument", function() {
>+        conversation.trigger("session:error");
>+
>+        sinon.assert.calledOnce(notifications.errorL10n);
>+        sinon.assert.calledWithExactly(notifications.errorL10n,
>+          sinon.match.string);
>       });
> 
>-      it("should trigger a notication when a session:error model event is " +
>-         " received", function() {
>-        sandbox.stub(notifications, "errorL10n");
>-        conversation.trigger("session:error", "tech error");
>+      it("should add a notification with the custom message id when a " +
>+         "session:error event is fired with an argument", function() {
>+        conversation.trigger("session:error", "tech_error");
> 
>         sinon.assert.calledOnce(notifications.errorL10n);
>         sinon.assert.calledWithExactly(notifications.errorL10n,
>-                                       "unable_retrieve_call_info");
>+                                       "tech_error");
>+      });
>+
>+      it("should set marketplace hidden iframe src when fxos:app-needed is " +
>+         "triggered", function(done) {
>+        var marketplace = view.getDOMNode().querySelector("#marketplace");
>+        expect(marketplace.src).to.be.equal("");
>+
>+        conversation.trigger("fxos:app-needed");
>+
>+        view.forceUpdate(function() {
>+          // XXX Update with the real marketplace url once the FxOS Loop app
>+          //     is uploaded to the marketplace bug 1053424
>+          expect(marketplace.src).to.be.equal(
>+            "http://fake-market.herokuapp.com/iframe-install.html");
>+          done();
>+        });
>       });
>+
>     });
> 
>     describe("#render", function() {
>       var conversation, view, requestCallUrlInfo, oldLocalStorageValue;
> 
>       beforeEach(function() {
>         oldLocalStorageValue = localStorage.getItem("has-seen-tos");
>         localStorage.removeItem("has-seen-tos");
>@@ -853,10 +904,198 @@ describe("loop.webapp", function() {
>         expect(helper.isFirefox("Firefox/Gecko")).eql(true);
>         expect(helper.isFirefox("Gecko/Firefox/Chuck Norris")).eql(true);
>       });
> 
>       it("shouldn't detect Firefox with other platforms", function() {
>         expect(helper.isFirefox("Opera")).eql(false);
>       });
>     });
>+
>+    describe("#isFirefoxOS", function() {
>+      describe("without mozActivities", function() {
>+        it("shouldn't detect FirefoxOS on mobile platform", function() {
>+          expect(helper.isFirefoxOS("mobi")).eql(false);
>+        });
>+
>+        it("shouldn't detect FirefoxOS on non mobile platform", function() {
>+          expect(helper.isFirefoxOS("whatever")).eql(false);
>+        });
>+      });
>+
>+      describe("with mozActivities", function() {
>+        var realMozActivity;
>+
>+        before(function() {
>+          realMozActivity = window.MozActivity;
>+          window.MozActivity = {};
>+        });
>+
>+        after(function() {
>+          window.MozActivity = realMozActivity;
>+        });
>+
>+        it("should detect FirefoxOS on mobile platform", function() {
>+          expect(helper.isFirefoxOS("mobi")).eql(true);
>+        });
>+
>+        it("shouldn't detect FirefoxOS on non mobile platform", function() {
>+          expect(helper.isFirefoxOS("whatever")).eql(false);
>+        });
>+      });
>+    });
>+  });
>+
>+  describe("Firefox OS", function() {
>+    var client;
>+
>+    before(function() {
>+      client = new loop.StandaloneClient({
>+        baseServerUrl: "http://fake.example.com"
>+      });
>+      sandbox.stub(client, "requestCallInfo");
>+    });
>+
>+    describe("FxOSConversationModel", function() {
>+      var model, realMozActivity;
>+
>+      before(function() {
>+        model = new loop.webapp.FxOSConversationModel({
>+          loopToken: "fakeToken",
>+          callerId: "callerId",
>+          callType: "callType"
>+        });
>+
>+        realMozActivity = window.MozActivity;
>+
>+        loop.config.fxosApp = {
>+          name: "Firefox Hello"
>+        };
>+      });
>+
>+      after(function() {
>+        window.MozActivity = realMozActivity;
>+      });
>+
>+      describe("setupOutgoingCall", function() {
>+        var _activityProps, _onerror, trigger;
>+
>+        function fireError(errorName) {
>+          _onerror({
>+            target: {
>+              error: {
>+                name: errorName
>+              }
>+            }
>+          });
>+        }
>+
>+        before(function() {
>+          window.MozActivity = function(activityProps) {
>+            _activityProps = activityProps;
>+            return {
>+              set onerror(callback) {
>+                _onerror = callback;
>+              }
>+            };
>+          };
>+        });
>+
>+        after(function() {
>+          window.MozActivity = realMozActivity;
>+        });
>+
>+        beforeEach(function() {
>+          trigger = sandbox.stub(model, "trigger");
>+        });
>+
>+        it("Activity properties", function() {
>+          expect(_activityProps).to.not.exist;
>+
>+          model.setupOutgoingCall();
>+
>+          expect(_activityProps).to.exist;
>+          expect(_activityProps).to.deep.equal({
>+            name: "loop-call",
>+            data: {
>+              type: "loop/token",
>+              token: "fakeToken",
>+              callerId: "callerId",
>+              callType: "callType"
>+            }
>+          });
>+        });
>+
>+        it("should trigger fxos:app-needed if a NO_PROVIDER activity error" +
>+          " occurs ",
>+          function() {
>+
>+            model.setupOutgoingCall();
>+            fireError("NO_PROVIDER");
>+
>+            sinon.assert.calledOnce(trigger);
>+            sinon.assert.calledWithExactly(trigger, "fxos:app-needed");
>+          }
>+        );
>+
>+        it("should trigger session:error if an unrecognized activity error" +
>+          " occurs",
>+          function() {
>+
>+            model.setupOutgoingCall();
>+            fireError("whatever");
>+
>+            sinon.assert.calledOnce(trigger);
>+            sinon.assert.calledWithExactly(trigger, "session:error",
>+              "fxos_app_needed", { fxosAppName: loop.config.fxosApp.name });
>+          }
>+        );
>+      });
>+
>+      describe("onMarketplaceMessage", function() {
>+        var view, setupOutgoingCall, trigger;
>+
>+        before(function() {
>+          view = React.addons.TestUtils.renderIntoDocument(
>+            loop.webapp.StartConversationView({
>+              model: model,
>+              notifications: notifications,
>+              client: {requestCallUrlInfo: sandbox.stub()}
>+            })
>+          );
>+        });
>+
>+        beforeEach(function() {
>+          setupOutgoingCall = sandbox.stub(model, "setupOutgoingCall");
>+          trigger = sandbox.stub(model, "trigger");
>+        });
>+
>+        it("should call setupOutgoingCall when given an install-package message" +
>+           " without error", function() {
>+
>+          model.onMarketplaceMessage({
>+            data: {
>+              name: "install-package"
>+            }
>+          });
>+
>+          sinon.assert.calledOnce(setupOutgoingCall);
>+        });
>+
>+        it("should trigger a session:error event when given an" +
>+           "install-package message with an error", function() {
>+
>+          model.onMarketplaceMessage({
>+            data: {
>+              name: "install-package",
>+              error: "error"
>+            }
>+          });
>+
>+          sinon.assert.notCalled(setupOutgoingCall);
>+          sinon.assert.calledOnce(trigger);
>+          sinon.assert.calledWithExactly(trigger, "session:error",
>+            "fxos_app_needed", { fxosAppName: loop.config.fxosApp.name });
>+        });
>+      });
>+    });
>   });
> });

Comment 94

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8493279 [details] [diff] [review]
Loop standalone UI should handoff to FxOS client, v11

Review of attachment 8493279 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is rebased against this morning's fx-team and with all comments addressed but the ones mentioned below.

I think we're in good shape now.  This will get r=dmose once you've addressed/spun-off the two comments here, and also 

* manually tested it a bit on an actual device with and without the app installed
* reviewed the changes I've made since your last patch :-)

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +166,5 @@
> +        if (event.target.error.name !== "NO_PROVIDER") {
> +          console.error ("Unexpected " + event.target.error.name);
> +          this.trigger("session:error", "fxos_app_needed", {
> +            fxosAppName: loop.config.fxosApp.name
> +          });

As mentioned in the review comments; passing a param like this doesn't actually work.  Feel free to fix it, as suggested in those review comments, or simply spin that off to a separate bug and hardcode the name for now in the interest of getting this landed.

@@ +362,5 @@
> +    _onFxOSAppNeeded: function() {
> +      this.setState({
> +        // XXX Update with the real marketplace url once the FxOS Loop app is
> +        //     uploaded to the marketplace bug 1053424
> +        marketplaceSrc: "http://fake-market.herokuapp.com/iframe-install.html"

The only remaining unaddressed comment from the previous review: should this URL either be or be derived from something set as a loop.config variable? 

I don't think this should stand in the way of the landing here, so feel free to either fix this, or spin it off as a separate bug.

Comment 95

[Fernando Jiménez Moreno [:ferjm]]

Thanks Dan!

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #94)
> ::: browser/components/loop/standalone/content/js/webapp.jsx
> @@ +166,5 @@
> > +        if (event.target.error.name !== "NO_PROVIDER") {
> > +          console.error ("Unexpected " + event.target.error.name);
> > +          this.trigger("session:error", "fxos_app_needed", {
> > +            fxosAppName: loop.config.fxosApp.name
> > +          });
> 
> As mentioned in the review comments; passing a param like this doesn't
> actually work.  Feel free to fix it, as suggested in those review comments,
> or simply spin that off to a separate bug and hardcode the name for now in
> the interest of getting this landed.

Yeah, this was added on v7 patch but it seems that it got lost at some point probably while rebasing. I'll add it again.

> 
> @@ +362,5 @@
> > +    _onFxOSAppNeeded: function() {
> > +      this.setState({
> > +        // XXX Update with the real marketplace url once the FxOS Loop app is
> > +        //     uploaded to the marketplace bug 1053424
> > +        marketplaceSrc: "http://fake-market.herokuapp.com/iframe-install.html"
> 
> The only remaining unaddressed comment from the previous review: should this
> URL either be or be derived from something set as a loop.config variable? 
> 

Yes, this was initially part of loop.config but was also removed because of my misunderstanding of your feedback at comment 57. I am adding it again as well.

Comment 96

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Loop standalone UI should handoff to FxOS client, v12

Dan, this patch should address your review comments above. I also fixed a couple of issues more and successfully tested the patch on a FxOS device.

Thanks for your help!

Comment 97

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8493884 [details] [diff] [review]
Loop standalone UI should handoff to FxOS client, v12

Review of attachment 8493884 [details] [diff] [review]:
-----------------------------------------------------------------

A lot of the unit tests were failing when I applied this patch to fx-team.  <https://gist.github.com/1d816e9976ee61bf6786> has a patch that fixes it.  Please add merge this into the patch before landing on fx-team.

An easy way to run all the loop tests except the functional test is, from the top of your source tree:

browser/components/loop/run-all-loop-test.sh

Please also be sure you're using build-jsx to build your JSX files, as it insists on the One True Version of react-tools that everyone on the team is using so that we don't get weird conflicts in the generated files.

r=dmose with these comments address; feel free to land once you've addressed them.  I'd suggest landing on fx-team, but that's not strictly necessary.

::: browser/components/loop/content/shared/js/models.js
@@ +370,3 @@
>       *
>       * @param  {String} messageId L10n message id
> +     * @param  {Object} l10nProps An object with variables to be interpolated

Looking at <http://usejsdoc.org/tags-type.html>, I think this wants to be {Object} [l10nprops] to indicate that it's optional.  This also needs a unit test that the optional property gets passed through correctly.

::: browser/components/loop/standalone/server.js
@@ +17,5 @@
>    res.set('Content-Type', 'text/javascript');
>    res.send([
>      "var loop = loop || {};",
>      "loop.config = loop.config || {};",
> +    "loop.config.serverUrl = 'http://192.168.0.16:" + loopServerPort + "';",

This looks like it was probably changed for real live testing, and presumably wants to be reverted.

Comment 98

[Fernando Jiménez Moreno [:ferjm]]

Thanks Dan!

https://hg.mozilla.org/integration/fx-team/rev/3164ff8ce17c

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #97)
> Comment on attachment 8493884 [details] [diff] [review]
> Loop standalone UI should handoff to FxOS client, v12
> 
> Review of attachment 8493884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A lot of the unit tests were failing when I applied this patch to fx-team. 
> <https://gist.github.com/1d816e9976ee61bf6786> has a patch that fixes it. 
> Please add merge this into the patch before landing on fx-team.

I couldn't reproduce these failures. But I added the fix anyway as it seemed to be harmless.

Comment 99

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3164ff8ce17c

Comment 100

[Dan Mosedale (:dmosedale, :dmose)]

You're welcome -- I'm excited to see this land, and sorry it took so long!

Comment 101

[u279076]

Although this has in-testsuite coverage, I'd like to make sure we verify this before uplift. Tony can you please make sure this gets tested?

Comment 102

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #101)
> Although this has in-testsuite coverage, I'd like to make sure we verify
> this before uplift. Tony can you please make sure this gets tested?

Flagging for verification prior to Aurora uplift as well. Tony, would it be possible for you to test this using the Try-server builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 103

[u279076]

Maire informed me that this is not testable until the standalone app is updated on the Loop server. As such I am untracking this for any verification. Please needinfo me once this becomes testable by QE.

Comment 104

[Maria Angeles Oteo (:oteo)]

Maire, please, let us know when this functionality is updated in the Production server so we can start the corresponding testing with the Loop mobile application. Thanks a lot!

Comment 105

[Mark Banner (:standard8)]

(In reply to Maria Angeles Oteo (:oteo) from comment #104)
> Maire, please, let us know when this functionality is updated in the
> Production server so we can start the corresponding testing with the Loop
> mobile application. Thanks a lot!

This is being handled in bug 1077059 / bug 1077061. We currently need information as to what the new configuration values should be for staging/production before we can push this out.

Comment 106

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8493884 [details] [diff] [review]
Loop standalone UI should handoff to FxOS client, v12

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 107

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a01b2985cfe6

Comment 108

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8493884 [details] [diff] [review]
Loop standalone UI should handoff to FxOS client, v12

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.