Closed
Bug 1002567
Opened 10 years ago
Closed 10 years ago
Support weighted snippets
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox32 verified, fennec32+)
VERIFIED
FIXED
Firefox 32
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(1 file)
4.71 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
See bug 1001379 for details on the API.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
(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 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
Sorry for the duplicate comment, not sure how that happened...
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f84446d45985 Filed bug 1004153 to address the "don't show the banner all the time" issue.
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f84446d45985
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Updated•10 years ago
|
tracking-fennec: ? → 32+
Comment 11•10 years ago
|
||
Verified as fixed in Build: 32.0a1 (2014-06-05) Device: Motorola Razr (Android 4.0.4)
Status: RESOLVED → VERIFIED
status-firefox32:
--- → verified
Updated•10 years ago
|
QA Contact: cristina.madaras
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•