Closed Bug 1002567 Opened 6 years ago Closed 6 years ago

Support weighted snippets

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox32 --- verified
fennec 32+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

See bug 1001379 for details on the API.
On desktop, this logic is implemented as part of the snippet JS payload:
https://github.com/mozilla/snippets-service/blob/master/snippets/base/templates/base/includes/snippet_js.html#L119

For Fennec, we'll need to figure out a way to do this in Snippets.js. Right now the Home.banner API just lets you add messages to a set of messages, and we randomly choose which one to show when the user opens about:home.

Ideally, it would be nice to just implement this in Snippets.js, but I think we should change the Home.banner API to let you specify a weight there, and then we can implement this weight logic where we choose the message to show in _sendBannerData.
I found the snippets code really confusing to read, so I tried using different variable names to make things easier to follow, but I'm still not convinced this is very readable.

I tested with this set of snippets, and it does work as expected:
http://people.mozilla.org/~mleibovic/snippets.json

However, I'm open to suggestions about how to make this easier to read.
Attachment #8414794 - Flags: review?(bnicholson)
With this patch, we'll still always show a banner message if one is available, but maybe we should also add some logic to sometimes never show a banner, especially if all the messages have a low weight.
(In reply to :Margaret Leibovic from comment #3)
> With this patch, we'll still always show a banner message if one is
> available, but maybe we should also add some logic to sometimes never show a
> banner, especially if all the messages have a low weight.

I would like to support "not always showing a banner if we have one" and by that I mean, let's give the user a break.

Also, do we have a way to only ever show a banner once? This came up with doing surveys. We never want to show the same survey banner if the user has already tapped the banner once.
(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > With this patch, we'll still always show a banner message if one is
> > available, but maybe we should also add some logic to sometimes never show a
> > banner, especially if all the messages have a low weight.
> 
> I would like to support "not always showing a banner if we have one" and by
> that I mean, let's give the user a break.

Currently we randomly choose a message to show. We could also add some randomization to sometimes never show any message. I think this is separate from weighted snippets, though (since those weights are used to calculate relative frequency).

> Also, do we have a way to only ever show a banner once? This came up with
> doing surveys. We never want to show the same survey banner if the user has
> already tapped the banner once.

We don't currently have a way to do this. If we want to do this for all snippets, that would be a one-line fix, since we already have logic for that when they hit the close button. If we only want to do this for certain messags we'd have to change the snippets JSON API.
Comment on attachment 8414794 [details] [diff] [review]
Support weighted snippets

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

This seems readable enough to me, maybe just because I'm viewing these changes in the context of this patch and not looking through all of Snippets/Home. But I can't see a better way to do this without a larger refactor.

::: mobile/android/components/Snippets.js
@@ +197,5 @@
>      }
>      let id = Home.banner.add({
>        text: message.text,
>        icon: message.icon,
> +      weight: parseInt(message.weight, 10),

Won't this break cached snippets that don't have a weight? We should probably use the default weight if the parse fails. You might want to move the parseInt to Home.jsm instead so you can fall back to the DEFAULT_WEIGHT constant.

::: mobile/android/modules/Home.jsm
@@ +56,5 @@
>  
>    if ("ondismiss" in options && typeof options.ondismiss === "function")
>      this.ondismiss = options.ondismiss;
> +
> +  if ("weight" in options && typeof options.weight === "number")

Nit: the `"weight" in options` here is redundant; you could just use `typeof options.weight === "number"`. But I guess it's fine since we're already doing the same thing in several places above.

@@ +86,5 @@
>    let _messages = {};
>  
> +  // Choose a random message from the set of messages, biasing towards those with higher weight.
> +  // Weight logic copied from desktop snippets:
> +  // https://github.com/mozilla/snippets-service/blob/master/snippets/base/templates/base/includes/snippet_js.html#L119

Code changes, so it's better to link to a specific revision instead of head. In this case, use https://github.com/mozilla/snippets-service/blob/7d80edb8b1cddaed075275c2fc7cdf69a10f4003/snippets/base/templates/base/includes/snippet_js.html#L119.

Pro tip: if you go to a URL on head (e.g., https://github.com/mozilla/snippets-service/blob/master/snippets/base/templates/base/includes/snippet_js.html#L119), then hit 'y', github will change the URL to point to the specific revision for you.
Attachment #8414794 - Flags: review?(bnicholson) → review+
(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > With this patch, we'll still always show a banner message if one is
> > available, but maybe we should also add some logic to sometimes never show a
> > banner, especially if all the messages have a low weight.
> 
> I would like to support "not always showing a banner if we have one" and by
> that I mean, let's give the user a break.

Currently we randomly choose a message to show. We could also add some randomization to sometimes never show any message. I think this is separate from weighted snippets, though (since those weights are used to calculate relative frequency).

> Also, do we have a way to only ever show a banner once? This came up with
> doing surveys. We never want to show the same survey banner if the user has
> already tapped the banner once.

We don't currently have a way to do this. If we want to do this for all snippets, that would be a one-line fix, since we already have logic for that when they hit the close button. If we only want to do this for certain messags we'd have to change the snippets JSON API.
Sorry for the duplicate comment, not sure how that happened...
Blocks: 1004153
https://hg.mozilla.org/integration/fx-team/rev/f84446d45985

Filed bug 1004153 to address the "don't show the banner all the time" issue.
https://hg.mozilla.org/mozilla-central/rev/f84446d45985
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
tracking-fennec: ? → 32+
Verified as fixed in
Build: 32.0a1 (2014-06-05)
Device: Motorola Razr (Android 4.0.4)
Status: RESOLVED → VERIFIED
QA Contact: cristina.madaras
You need to log in before you can comment on or make changes to this bug.