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.

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.

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

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

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.

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

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.

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?

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.

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.

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.

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.

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.

Change pref to mozApps.used

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|

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.

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

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

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?

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.

Verified on Nightly.