Anant Narayanan [:anant] Assignee
	Description • 12 years ago

The AITC service should not run if the user isn't doing anything apps related. The current code looks for "appiness" by seeing if the user has visited the mozilla marketplace or dashboard. We need a better approach.

This is especially tricky because when the user first uses a new computer/device, their history is empty, and we have no information to go on; and the user may still expect to see their apps. This bug will likely need some Ux input as well.

	Ian Bicking (:ianbicking)
	Comment 1 • 12 years ago

Presumably any call to navigator.mozApps would imply that somebody (dashboard, marketplace, other) is interested in the apps.  If, in a session, no one touches mozApps then there's no possible affect AITC could have, and so no reason for it to run.  Perhaps DOMApplicationRegistry could start up AITC in that case.

	Anant Narayanan [:anant] Assignee
	Comment 2 • 12 years ago

Two part patch. This one sets apps.enabled to true when the DOM API is accessed.

	Anant Narayanan [:anant] Assignee
	Comment 3 • 12 years ago

This part checks for apps.enabled and starts AITC if true. If false, we'll watch for a change.

	Gregory Szorc [:gps]
	Comment 4 • 12 years ago

Comment on attachment 633691 [details] [diff] [review]
Enable AITC only when apps.enabled is true

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

I love when a patch overall deletes code \o/

r+ is conditional on comments being addressed and on a unit test ensuring this works.

::: services/aitc/Aitc.js
@@ +53,2 @@
>          // Wait and see if the user wants anything apps related.
> +        Preferences.observe("apps.enabled", function() {

Nit: Name function.

@@ +53,3 @@
>          // Wait and see if the user wants anything apps related.
> +        Preferences.observe("apps.enabled", function() {
> +          if (Preferences.get("apps.enabled"), false) {

Your parenthesis are messed up there. if (x, false) ?

Also, where does Preferences come from? You are using the API exported from services-common/preferences.js. However, I don't see it imported anywhere. How is the code in this file even working? Am I blind?

@@ +53,5 @@
>          // Wait and see if the user wants anything apps related.
> +        Preferences.observe("apps.enabled", function() {
> +          if (Preferences.get("apps.enabled"), false) {
> +            this.start();
> +          }

You may want to consider removing the observer once the service is started. Or, you could unload the service if the pref changes that way. That's a little more dangerous, IMO, since you don't know what state the service could be in. Your call.

	Anant Narayanan [:anant] Assignee
	Comment 5 • 12 years ago

Since nsIPrefBranch throws when a preference isn't present, it's probably more efficient to simply set the apps.enabled pref on service startup.

	Anant Narayanan [:anant] Assignee
	Comment 6 • 12 years ago

That's what I get for uploading a patch just after watching Rambo III.

This one works, but I couldn't figure out how to write a test for it. We don't have any singletons and the only interfaces AITCService implements are nsISupports and nsIObserver, so we can't peek into it from a unit test.

	Gregory Szorc [:gps]
	Comment 7 • 12 years ago

Comment on attachment 633747 [details] [diff] [review]
Enable AITC only when apps.enabled is true

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

Yeah, I watched the first 3 Rambo films on a weekend a few months back and my patches for the next few days were highly suspect. Rambo 3 is especially mind numbing.

The code looks good. I'd still really like to see a unit test. Perhaps you could check things by changing the pref, waiting a few hundred milliseconds then verifying a service instance was created?

	Anant Narayanan [:anant] Assignee
	Comment 8 • 12 years ago

CC: Ben. One of the goals with this patch was to make sure we limit traffic to BrowserID. Our strategy is to disable AITC until a call to navigator.mozApps.* is made. Hopefully this will be sufficient to ensure all >400mil FF users don't hit the BrowserID server, but more eyes on this will be useful.

	Mounir Lamouri (:mounir)
	Comment 9 • 12 years ago

Comment on attachment 633745 [details] [diff] [review]
Set apps.enabled on DOM API access

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

I'm not sure what is the point of using a preference instead of an XPCOM observer here.

Cancelling the review request.

	Anant Narayanan [:anant] Assignee
	Comment 10 • 12 years ago

Comment on attachment 633745 [details] [diff] [review]
Set apps.enabled on DOM API access

Per discussion on IRC, even though it's ugly, looks like using a pref may be our best option here. Re-asking for review.

	Anant Narayanan [:anant] Assignee
	Comment 11 • 12 years ago

Ok, turns out nsBrowserGlue imports Webapps.jsm and calls init() even without anyone accessing the mozApps API so the pref was being set in the wrong place. Moved the setter to receiveMessage instead according to Fabrice's advice - works as expected.

	[:fabrice] Fabrice Desré
	Comment 12 • 12 years ago

I think the name of the pref you use is misleading. You're not checking if mozApps support is enabled, but if a DOM call has been made. Please use something like dom.mozApps.used instead, or something aitc specific.

	Anant Narayanan [:anant] Assignee
	Comment 13 • 12 years ago

Change pref to mozApps.used

	[:fabrice] Fabrice Desré
	Comment 14 • 12 years ago

Comment on attachment 636354 [details] [diff] [review]
Set mozApps.used on DOM API access

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

r=me with comment addressed

::: dom/apps/src/Webapps.jsm
@@ +106,5 @@
>  
>    receiveMessage: function(aMessage) {
> +    // nsIPrefBranch throws if pref does not exist, faster to simply write
> +    // the pref instead of first checking if it is false.
> +    Services.prefs.setBoolPref("mozApps.used", true);

Please use |dom.mozApps.used|

	Anant Narayanan [:anant] Assignee
	Comment 15 • 12 years ago

I tried a test that looked something like: http://pastie.org/4156250 but the observer was never firing. I'm not entirely sure how to do this in xpcshell, so I'm going to check-in this code and followup for a test in bug 760902.

	Anant Narayanan [:anant] Assignee
	Comment 16 • 12 years ago

https://hg.mozilla.org/services/services-central/rev/0388e4acf5c8
https://hg.mozilla.org/services/services-central/rev/86ae249ed098

	Gregory Szorc [:gps]
	Comment 17 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/0388e4acf5c8
https://hg.mozilla.org/mozilla-central/rev/86ae249ed098

	Jason Smith [:jsmith]
	Comment 18 • 12 years ago

Trying to understand how to verify this. Basically, when dom.mozApps.used is set to true (a mozapps action has been made), then I should see AITC-related actions. Otherwise if there has been no mozapps actions, then there should be no AITC-related actions being seen (looking at the logs). Is that right? Or are there more details to this?

	Anant Narayanan [:anant] Assignee
	Comment 19 • 12 years ago

You don't have to mess around with prefs to test this. Start a fresh profile, enable logging and ensure there is no AITC activity. Then visit the marketplace and install an app or visit the dashboard, and AITC should start up automatically.

	Jason Smith [:jsmith]
	Comment 20 • 12 years ago

Verified on Nightly.