If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Determine if a user needs AITC and disable until then

VERIFIED FIXED

Status

Web Apps
AppsInTheCloud
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: anant, Assigned: anant)

Tracking

(Depends on: 1 bug)

Details

(Whiteboard: [blocking-aitc+], [qa!])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 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.
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.

Updated

5 years ago
Whiteboard: [blocking-aitc]

Updated

5 years ago
Whiteboard: [blocking-aitc] → [blocking-aitc+]
(Assignee)

Comment 2

5 years ago
Created attachment 633689 [details] [diff] [review]
Set apps.enabled on DOM API access

Two part patch. This one sets apps.enabled to true when the DOM API is accessed.
Assignee: nobody → anant
Status: NEW → ASSIGNED
Attachment #633689 - Flags: review?(mounir)
(Assignee)

Comment 3

5 years ago
Created attachment 633691 [details] [diff] [review]
Enable AITC only when apps.enabled is true

This part checks for apps.enabled and starts AITC if true. If false, we'll watch for a change.
Attachment #633691 - Flags: review?(gps)

Comment 4

5 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.
Attachment #633691 - Flags: review?(gps)
(Assignee)

Comment 5

5 years ago
Created attachment 633745 [details] [diff] [review]
Set apps.enabled on DOM API access

Since nsIPrefBranch throws when a preference isn't present, it's probably more efficient to simply set the apps.enabled pref on service startup.
Attachment #633689 - Attachment is obsolete: true
Attachment #633689 - Flags: review?(mounir)
Attachment #633745 - Flags: review?(mounir)
(Assignee)

Comment 6

5 years ago
Created attachment 633747 [details] [diff] [review]
Enable AITC only when apps.enabled is true

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.
Attachment #633691 - Attachment is obsolete: true
Attachment #633747 - Flags: review?(gps)

Comment 7

5 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?
Attachment #633747 - Flags: review?(gps) → review+
(Assignee)

Comment 8

5 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.
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.
Attachment #633745 - Flags: review?(mounir)
(Assignee)

Comment 10

5 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.
Attachment #633745 - Flags: review?(mounir)
Attachment #633745 - Flags: review?(mounir) → review+
Depends on: 766057
(Assignee)

Comment 11

5 years ago
Created attachment 634719 [details] [diff] [review]
Set apps.enabled on DOM API access

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.
Attachment #633745 - Attachment is obsolete: true
Attachment #634719 - Flags: review?(fabrice)
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.
(Assignee)

Comment 13

5 years ago
Created attachment 636354 [details] [diff] [review]
Set mozApps.used on DOM API access

Change pref to mozApps.used
Attachment #634719 - Attachment is obsolete: true
Attachment #634719 - Flags: review?(fabrice)
Attachment #636354 - Flags: review?(fabrice)
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|
Attachment #636354 - Flags: review?(fabrice) → review+
(Assignee)

Comment 15

5 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.
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/services/services-central/rev/0388e4acf5c8
https://hg.mozilla.org/services/services-central/rev/86ae249ed098
Whiteboard: [blocking-aitc+] → [blocking-aitc+], [fixed in services]

Updated

5 years ago
Whiteboard: [blocking-aitc+], [fixed in services] → [blocking-aitc+], [fixed in services], [qa+]
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/0388e4acf5c8
https://hg.mozilla.org/mozilla-central/rev/86ae249ed098
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [blocking-aitc+], [fixed in services], [qa+] → [blocking-aitc+][qa+]

Updated

5 years ago
QA Contact: jsmith
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?
(Assignee)

Comment 19

5 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.
Verified on Nightly.
Status: RESOLVED → VERIFIED
Whiteboard: [blocking-aitc+][qa+] → [blocking-aitc+], [qa!]
Depends on: 775369
No longer depends on: 766057
You need to log in before you can comment on or make changes to this bug.