Closed Bug 937820 Opened 6 years ago Closed 6 years ago

Make dynamic snippets built into the browser

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
fennec 28+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 5 obsolete files)

We started developing our dynamic snippets feature as an add-on [1], but recently decided that it would be best to just include it as part of the browser, similarly to desktop Firefox.

That being said, the Home.banner API is still available for add-ons to add their own messages to this promotional banner, so third parties could still also show messages if they want.

Setting this to block 922792, which is tracking the dynamic snippets work.

[1] https://github.com/leibovic/promo-banner/blob/snippets/bootstrap.js
Depends on: home-banner
Attached patch WIP (obsolete) — Splinter Review
Some issues:
* For some reason `Services.prefs.setCharPref(SNIPPETS_LAST_UPDATE_PREF, Date.now())` isn't working, so this patch is never loading snippets from the cache.
* I hard-coded the country code to test the filtering, but we'll actually need to ping the server to get the correct value for that.
* Still not pinging the metrics server with ids of messages that were shown, but we can do that when bug 937373 is fixed.

Other than that, this seems to be working pretty well. I'm still using my test server, but I updated it to serve up message id and target_geo properties to match the spec.
Attachment #831203 - Flags: feedback?(mark.finkle)
Attached patch WIP (obsolete) — Splinter Review
Newer version.
Attachment #831203 - Attachment is obsolete: true
Attachment #831203 - Flags: feedback?(mark.finkle)
Attachment #831216 - Flags: feedback?(mark.finkle)
Comment on attachment 831216 [details] [diff] [review]
WIP

>diff --git a/mobile/android/components/MobileComponents.manifest b/mobile/android/components/MobileComponents.manifest

>+# Snippets.js
>+component {a78d7e59-b558-4321-a3d6-dffe2f1e76dd} Snippets.js
>+contract @mozilla.org/snippets;1 {a78d7e59-b558-4321-a3d6-dffe2f1e76dd}
>+category app-startup Snippets service,@mozilla.org/snippets;1

You are doing two things every time we start the app:
1. Loading snippets from cache for from XHR, based on the daily interval
2. Pushing the snippets into the Banner API

#2 seems fine to do at app-startup, probably by loading the snippets from the cache. For #1 I am wondering if we should piggy back onto the Gecko update timer system. Blocklist pings, Add-on updates, Telemetry and others use the updater timer to group networking workload around a common time.

We do that with our own Add-on Update component to:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/MobileComponents.manifest#76

We should check with people like Gavin to see if this is the preferred way to do timed/interval networking.

>diff --git a/mobile/android/components/Snippets.js b/mobile/android/components/Snippets.js

>+// Lazy getter to get BrowserApp from main chrome window
>+XPCOMUtils.defineLazyGetter(this, "gChromeWin", function() {
>+  let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
>+  let windows = wm.getEnumerator("navigator:browser");
>+  while (windows.hasMoreElements()) {
>+    // Fennec should only have one chrome browser window
>+    let domWindow = windows.getNext().QueryInterface(Ci.nsIDOMWindow);
>+    return domWindow;
>+  }
>+});

It's way easier than that:
XPCOMUtils.defineLazyGetter(this, "gChromeWin", function() {
  return Services.wm.getMostRecentWindow("navigator:browser");
}
Attachment #831216 - Flags: feedback?(mark.finkle) → feedback+
tracking-fennec: ? → 28+
Attached patch WIP v2 (obsolete) — Splinter Review
I found that the profile directory isn't actually available on "app-startup", so I'm observing "profile-after-change" instead. I also found that I could use the update-timer category to pull new snippets from the sever (I set this pref to a lower value to test and verify it worked).

The two things left to do here are actually update the country code from the server (maybe I could also register an update-timer category for that), and send metrics pings (we can figure out if/how we should batch these). Maybe these issues should be split into separate bugs/patches.
Attachment #831216 - Attachment is obsolete: true
Attachment #8334227 - Flags: feedback?(mark.finkle)
Comment on attachment 8334227 [details] [diff] [review]
WIP v2

>+    Home.banner.add({
>+      text: message.text,
>+      icon: message.icon,
>+      onclick: function() {
>+        gChromeWin.BrowserApp.addTab(message.url);
>+      },
>+      onshown: function() {
>+        // XXX: 10% of the time, let the metrics server know which message was shown.
>+        LOG("message shown: " + message.id);
>+      }
>+    });

I'm wondering if we can cache the "10% shown messages" and upload them once a day? Maybe when we do the download. I don't want to waste any bandwidth or battery if we can help it. I don't feel the data needs to be delivered instantly.

f+, this patch is looking good
Attachment #8334227 - Flags: feedback?(mark.finkle) → feedback+
(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 8334227 [details] [diff] [review]
> WIP v2
> 
> >+    Home.banner.add({
> >+      text: message.text,
> >+      icon: message.icon,
> >+      onclick: function() {
> >+        gChromeWin.BrowserApp.addTab(message.url);
> >+      },
> >+      onshown: function() {
> >+        // XXX: 10% of the time, let the metrics server know which message was shown.
> >+        LOG("message shown: " + message.id);
> >+      }
> >+    });
> 
> I'm wondering if we can cache the "10% shown messages" and upload them once
> a day? Maybe when we do the download. I don't want to waste any bandwidth or
> battery if we can help it. I don't feel the data needs to be delivered
> instantly.

Maybe we can just do this bit as part of bug 937373. I believe the issue is that metrics makes dashboards with impressions per day or something like that, so caching/batching the message ids might lead to inaccurate stats, especially if the user doesn't open their browser again for a day or two. To work around this, we could include a timestamp with the messages, but this would require some re-architecting on the metrics side of things.

mkelly, is this description of the issue correct?

To play devil's advocate, in terms of bandwidth, this is a basically an empty request to a URL, so it's not really using much data, and if the user is on about:home, aren't they likely to be visiting some webpage immediately afterwards, so the network stack will need to be up and running anyway?
Flags: needinfo?(mkelly)
(In reply to :Margaret Leibovic from comment #6)

> To play devil's advocate, in terms of bandwidth, this is a basically an
> empty request to a URL, so it's not really using much data, and if the user
> is on about:home, aren't they likely to be visiting some webpage immediately
> afterwards, so the network stack will need to be up and running anyway?

It's possible the stack is up and running. We need to worry about the modem's affect on battery as well as bandwidth issues. Sounds like battery life could be more affected in this case.
(In reply to :Margaret Leibovic from comment #6)
> Maybe we can just do this bit as part of bug 937373. I believe the issue is
> that metrics makes dashboards with impressions per day or something like
> that, so caching/batching the message ids might lead to inaccurate stats,
> especially if the user doesn't open their browser again for a day or two. To
> work around this, we could include a timestamp with the messages, but this
> would require some re-architecting on the metrics side of things.
> 
> mkelly, is this description of the issue correct?

Yep! I've commented on bug 937373 asking if batch metrics is feasible, but our ideal state would be to send the impressions as soon as they occur.
Flags: needinfo?(mkelly)
Attached patch patch (obsolete) — Splinter Review
I want to get something landed in the tree sooner rather than later, so I added a pref to disable snippets until the server component is ready (if they're enabled but the server isn't responding, nothing bad happens, but we do send a request to the server every time we don't find the cached snippets, so that's not so good). The nice thing about this in the tree is that the snippets team can test their server implementation on Nightly, just by flipping some prefs around.

This patch also pings the geo server once every 30 days to update the country code stored in a pref. I don't think you can have 2 different update-timers for the same component, so I just used a last-updated pref inside the current update timer handler.

And I punted on the metrics ping bit, we can solve that in bug 937373.
Attachment #8334227 - Attachment is obsolete: true
Attachment #8335563 - Flags: review?(mark.finkle)
Comment on attachment 8335563 [details] [diff] [review]
patch

It would be good to have somebody besides just mfinkle looking at this. I choose bnicholson!
Attachment #8335563 - Flags: review?(bnicholson)
Comment on attachment 8335563 [details] [diff] [review]
patch

>diff --git a/mobile/android/components/Snippets.js b/mobile/android/components/Snippets.js
>+function updateSnippets(callback) {

>+    lastUpdate = parseFloat(Services.prefs.getCharPref(SNIPPETS_GEO_LAST_UPDATE_PREF));

Do we need to store as a string and convert to a float because of the size? Will it overflow an integer?

>+function updateCountryCode() {

>+    Services.prefs.setCharPref(SNIPPETS_GEO_LAST_UPDATE_PREF, Date.now());

Change this one, if we can just use setIntPref

>+function cacheResponse(response) {

>+  let promise = OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp"});

{ tmpPath: path + ".tmp" }
                        ^

>+function updateBanner(response) {

>+      onclick: function() {
>+        gChromeWin.BrowserApp.addTab(message.url);
>+      },

Yes, we do this in a few of our components. It just occurred to me that I get aggro when other products add FE specific code in XPCOM components and I wondered what we'd do to be less coupled.

Something like this should work:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#94

Translates to:
gChromeWin.browserDOMWindow.openURI(Services.io.newURI(message.url, null, null), null, Ci.nsIBrowserDOMWindow.OPEN_NEWTAB, Ci.nsIBrowserDOMWindow.OPEN_NEW);

OK, maybe it's not as pretty and maybe I shouldn't worry about this particular coupling.

>+  observe: function(subject, topic, data) {
>+    if (!SNIPPETS_ENABLED) {

>+  notify: function(timer) {
>+    if (!SNIPPETS_ENABLED) {

SNIPPETS_ENABLED is read once and if flipped needs a restart to become active. OK by me, but I wanted to make sure it was an explicit decision.
Attachment #8335563 - Flags: review?(mark.finkle) → review+
Just a note, in bug 937373 it was confirmed that we can batch the metrics pings into a single GET request formatted something like this:

https://snippets-stats.mozilla.org/mobile?s1=3825&t1=2013-11-17T18:27Z&s2=6326&t2=2013-11-18T18:27Z

Where s1 is the ID for the first snippet in the batch and t1 is the time that the impression occurred (Timestamps were in ISO8601 for example purposes, but hey, it's standard, why not? Just include timezone info if possible!).
Depends on: 941685
(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 8335563 [details] [diff] [review]
> patch
> 
> >diff --git a/mobile/android/components/Snippets.js b/mobile/android/components/Snippets.js
> >+function updateSnippets(callback) {
> 
> >+    lastUpdate = parseFloat(Services.prefs.getCharPref(SNIPPETS_GEO_LAST_UPDATE_PREF));
> 
> Do we need to store as a string and convert to a float because of the size?
> Will it overflow an integer?

Yes, that was the problem I found when I first tried using int prefs.

> >+function updateCountryCode() {
> 
> >+    Services.prefs.setCharPref(SNIPPETS_GEO_LAST_UPDATE_PREF, Date.now());
> 
> Change this one, if we can just use setIntPref

Is there a cost difference between a char pref and an int pref? We could always convert the timestamp to seconds if that's easier to store, but we'd have to do that conversion in the date comparison as well.

> >+function cacheResponse(response) {
> 
> >+  let promise = OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp"});
> 
> { tmpPath: path + ".tmp" }
>                         ^

We need to get that linter up and running!

> >+function updateBanner(response) {
> 
> >+      onclick: function() {
> >+        gChromeWin.BrowserApp.addTab(message.url);
> >+      },
> 
> Yes, we do this in a few of our components. It just occurred to me that I
> get aggro when other products add FE specific code in XPCOM components and I
> wondered what we'd do to be less coupled.
> 
> Something like this should work:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> BrowserCLH.js#94
> 
> Translates to:
> gChromeWin.browserDOMWindow.openURI(Services.io.newURI(message.url, null,
> null), null, Ci.nsIBrowserDOMWindow.OPEN_NEWTAB,
> Ci.nsIBrowserDOMWindow.OPEN_NEW);
> 
> OK, maybe it's not as pretty and maybe I shouldn't worry about this
> particular coupling.

Hmm, I agree decoupling the front-end code is nice, but BrowserApp.addTab seems like a public enough API that it's okay. Like, it's an API that we encourage add-ons to use.

> >+  observe: function(subject, topic, data) {
> >+    if (!SNIPPETS_ENABLED) {
> 
> >+  notify: function(timer) {
> >+    if (!SNIPPETS_ENABLED) {
> 
> SNIPPETS_ENABLED is read once and if flipped needs a restart to become
> active. OK by me, but I wanted to make sure it was an explicit decision.

This was intentional, and I think it's okay, since flipping the pref wouldn't actually have any effect until restarting the browser, since we only add items to the banner on startup.
Comment on attachment 8335563 [details] [diff] [review]
patch

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

::: mobile/android/app/mobile.js
@@ +796,5 @@
> +
> +// How frequently we check for new snippets, in seconds (1 day)
> +pref("browser.snippets.updateInterval", 86400);
> +
> +// URL used to check for user's conntry code

Nit: We ain't in the deep sayouth, so I'm suspectin' you wanna say "conntry" a bit diff'rent like in these here parts.

::: mobile/android/components/Snippets.js
@@ +49,5 @@
> +XPCOMUtils.defineLazyGetter(this, "gCountryCode", function() {
> +  try {
> +    return Services.prefs.getCharPref(SNIPPETS_COUNTRY_CODE_PREF);
> +  } catch (e) {
> +    // Return an emtpy string if the country code pref isn't set yet.

Nit: emtpy

@@ +74,5 @@
> +  let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
> +  try {
> +    xhr.open("GET", gSnippetsURL, true);
> +  } catch (e) {
> +    Cu.reportError("Exception initalizing request to " + gSnippetsURL + ": " + e);

Data connections on mobile devices can be pretty unreliable: the device may temporarily be in offline mode, not in range, etc. This may be a particularly common issue for people who are behind captive portals; the update would fire immediately when a user starts the browser, fail since the user isn't online yet, and they miss the update once they're signed in.

Would it be worth having some kind of logic for retrying on failures (e.g., exponential backoff)?

@@ +97,5 @@
> +/**
> + * Fetches the user's country code from the geo server and stores the value in a pref.
> + */
> +function updateCountryCode() {
> +  let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);

There's a lot of duplicate XHR boilerplate code between here and updateSnippets, so you may want to factor this out into a separate method.

@@ +202,5 @@
> +  notify: function(timer) {
> +    if (!SNIPPETS_ENABLED) {
> +      return;
> +    }
> +    updateSnippets();

Any reason we don't update the banner in a callback here? If I understand this correctly, we won't actually show anything until Fennec is restarted and the snippet is read from the cache. Apps can stay open for a long time, especially on higher-end devices, so it may take several days before the snippet gets shown.
Attachment #8335563 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #14)

> Data connections on mobile devices can be pretty unreliable: the device may
> temporarily be in offline mode, not in range, etc. This may be a
> particularly common issue for people who are behind captive portals; the
> update would fire immediately when a user starts the browser, fail since the
> user isn't online yet, and they miss the update once they're signed in.
> 
> Would it be worth having some kind of logic for retrying on failures (e.g.,
> exponential backoff)?

I would consider this as a separate bug since it affects all of our network pings, not just this one and this one is not more important than the others.
(In reply to Brian Nicholson (:bnicholson) from comment #14)

> @@ +97,5 @@
> > +/**
> > + * Fetches the user's country code from the geo server and stores the value in a pref.
> > + */
> > +function updateCountryCode() {
> > +  let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
> 
> There's a lot of duplicate XHR boilerplate code between here and
> updateSnippets, so you may want to factor this out into a separate method.

I like this idea, since I'm going to need to make yet another request for the metrics ping.

> @@ +202,5 @@
> > +  notify: function(timer) {
> > +    if (!SNIPPETS_ENABLED) {
> > +      return;
> > +    }
> > +    updateSnippets();
> 
> Any reason we don't update the banner in a callback here? If I understand
> this correctly, we won't actually show anything until Fennec is restarted
> and the snippet is read from the cache. Apps can stay open for a long time,
> especially on higher-end devices, so it may take several days before the
> snippet gets shown.

Interesting point... if we do this, we should keep track of the current set of snippets in the banner, and remove them when we update, so that we don't have a bunch of copies of the same snippets in the banner rotation.
Attached patch patch v2 (obsolete) — Splinter Review
New version with some suggested improvements. I like the idea to update the snippets when we get new ones, since it's also good to remove any old snippets that we might not want to be showing to users anymore.
Attachment #8335563 - Attachment is obsolete: true
Attachment #8336501 - Flags: review?(bnicholson)
Comment on attachment 8336501 [details] [diff] [review]
patch v2

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

::: mobile/android/components/Snippets.js
@@ +107,5 @@
> + */
> +function loadSnippetsFromCache() {
> +  let path = OS.Path.join(OS.Constants.Path.profileDir, "snippets.json");
> +  let promise = OS.File.read(path);
> +  promise.then(array => addSnippetsToBanner(gDecoder.decode(array)), e => {

Where is addSnippetsToBanner defined?

@@ +143,5 @@
> +
> +  let messages = JSON.parse(response);
> +  messages.forEach(function(message) {
> +    // Don't add this message to the banner if it's not supposed to be shown in this country.
> +    if ("target_geo" in message && message.target_geo != gCountryCode) {

I just noticed that there's a race condition with gCountryCode. updateCountryCode() sets SNIPPETS_COUNTRY_CODE_PREF and updateBanner() reads the pref, and both happen asynchronously and independently after an XHR. If the gSnippetsURL XHR finishes before the country code is retrieved, this gCountryCode getter will fail. There should probably be some kind of guard for the updateBanner() call that waits until updateCountryCode() has finished before it's executed.

I'm not sure if the Promise/Deferred objects in Gecko have anything like jQuery's "when" (http://api.jquery.com/jQuery.when/); that's what I've used in the past to execute a given callback after multiple async operations have completed, and it works nicely.

@@ +167,5 @@
> + *
> + * @param url where we send the request
> + * @param callback function that is called with the xhr responseText
> + */
> +function _httpGetRequest(url, callback) {

Nice!
(In reply to Brian Nicholson (:bnicholson) from comment #18)
> Comment on attachment 8336501 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8336501 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/components/Snippets.js
> @@ +107,5 @@
> > + */
> > +function loadSnippetsFromCache() {
> > +  let path = OS.Path.join(OS.Constants.Path.profileDir, "snippets.json");
> > +  let promise = OS.File.read(path);
> > +  promise.then(array => addSnippetsToBanner(gDecoder.decode(array)), e => {
> 
> Where is addSnippetsToBanner defined?

Oops, refactoring fail. I switched it back to updateBanner.

> @@ +143,5 @@
> > +
> > +  let messages = JSON.parse(response);
> > +  messages.forEach(function(message) {
> > +    // Don't add this message to the banner if it's not supposed to be shown in this country.
> > +    if ("target_geo" in message && message.target_geo != gCountryCode) {
> 
> I just noticed that there's a race condition with gCountryCode.
> updateCountryCode() sets SNIPPETS_COUNTRY_CODE_PREF and updateBanner() reads
> the pref, and both happen asynchronously and independently after an XHR. If
> the gSnippetsURL XHR finishes before the country code is retrieved, this
> gCountryCode getter will fail. There should probably be some kind of guard
> for the updateBanner() call that waits until updateCountryCode() has
> finished before it's executed.
> 
> I'm not sure if the Promise/Deferred objects in Gecko have anything like
> jQuery's "when" (http://api.jquery.com/jQuery.when/); that's what I've used
> in the past to execute a given callback after multiple async operations have
> completed, and it works nicely.

Heh, this is mbrubeck's interview question! On quick glance, I don't see anything like this, but I could try implementing something like this.

I actually purposefully punted on this (hence the fallback in the gCountryCode getter), since this is only an issue on first run, and in the failure case we just wouldn't show any snippets for a day. But you're right that this would be more robust if we waited for the country code. Since we're only doing that once per month in the background, and there's no rush to complete the two requests, we could just do them in series. I'll update the patch to do that.
If you want to cheat, you could make updateCountryCode accept a callback, then nest your call to updateBanner() inside that callback. It's not optimal since the second XHR will wait instead of executing in parallel, but it's probably be good enough for this situation.
Attached patch patch v3Splinter Review
Like I mentioned in my last comment, this just makes the country code and snippets requests sequentially. This only happens once every 30 days, and it happens in the background (except on the very first run), so I don't think we should worry about fancier optimizations to do them in parallel (hopefully I'm not just being lazy here :).
Attachment #8336501 - Attachment is obsolete: true
Attachment #8336501 - Flags: review?(bnicholson)
Attachment #8337128 - Flags: review?(bnicholson)
(In reply to Brian Nicholson (:bnicholson) from comment #20)
> If you want to cheat, you could make updateCountryCode accept a callback,
> then nest your call to updateBanner() inside that callback. It's not optimal
> since the second XHR will wait instead of executing in parallel, but it's
> probably be good enough for this situation.

Haha, I did this before seeing this comment.
(In reply to :Margaret Leibovic (away Nov 23 - Dec 1) from comment #22)
> (In reply to Brian Nicholson (:bnicholson) from comment #20)
> > If you want to cheat, you could make updateCountryCode accept a callback,
> > then nest your call to updateBanner() inside that callback. It's not optimal
> > since the second XHR will wait instead of executing in parallel, but it's
> > probably be good enough for this situation.
> 
> Haha, I did this before seeing this comment.

And I overlooked that you commented about doing it before I made my comment!
Comment on attachment 8337128 [details] [diff] [review]
patch v3

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

Much win, many great
Attachment #8337128 - Flags: review?(bnicholson) → review+
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/be9ab82f6227

Snippets folks, starting tomorrow, you'll be able to test snippets in Nightly by toggling these prefs in about:config:

browser.snippets.enabled = true
browser.snippets.updateUrl = <whatever URL you want to test>

You can also set this pref to force the snippets to update more frequently:

browser.snippets.updateInterval = <time in seconds>
https://hg.mozilla.org/mozilla-central/rev/be9ab82f6227
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Depends on: 946486
Depends on: 977844
You need to log in before you can comment on or make changes to this bug.