736272 - Add extra awesomeness weight to bookmarks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

See bug 704977 comment 7. We can't do this until bug 721731, because right now we have no way of knowing if a history entry is a bookmark or not.

Comment 1

[Madhava Enros [:madhava]]

There was a proposal for how much additional weight to give bookmarks over here: bug 704977

Or is this something where we can draw from the desktop algorithm?

Comment 2

[:Margaret Leibovic]

(In reply to Madhava Enros [:madhava] from comment #1)
> There was a proposal for how much additional weight to give bookmarks over
> here: bug 704977
> 
> Or is this something where we can draw from the desktop algorithm?

We're keeping things simpler than desktop, so our current algorithm is a bit of a cheap imitation, but we could follow the logic used to add a "bonus" for bookmarks:

https://developer.mozilla.org/en/The_Places_frecency_algorithm

In bug 704977 comment 7, johnath suggested giving bookmarks a "100 point bonus" - johnath, did you mean this to be a factor we multiply with our current multplier? If so, this value is too high, since the current maximum value for the multipler is 100. I think we'd want something more like 2 (that would resemble what desktop does).

Comment 3

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #2)

> If so, this value is too high, since the current maximum
> value for the multipler is 100. I think we'd want something more like 2
> (that would resemble what desktop does).

Actually, we're multiplying the whole multiplier by 100 right now, and on closer inspection our math is more different than desktop math than I thought.

Given the examples in that comment, if we just multiply our current multipier by 100 for bookmarks:
- 1 visit last week (not a bookmark) = 82
- 3 visits yesterday (not a bookmark) = 299
- a bookmark that was visited once = 100

So I think an extra factor of 100 for bookmarks sounds right. Bookmarks with a bunch of visits are going to have a huge score, but that sounds like what we want.

Comment 4

[:Margaret Leibovic]

Hm, maybe 100 is too high. Consider these two cases:
- a bookmark you visited once, but today = 10000
- a non-bookmark you visited 50 times today = 5000

I think visiting a site 50 times today makes it more important than a bookmark.

Comment 5

[:Margaret Leibovic]

Attachment: patch

I decided to go with 50 as the multiplier in this patch, but that's simple enough to change.

Per usual, I ran testBrowserProviderPerf locally, and performance looks about the same. Once again, it's hard to tell what's noise or not.

Without patch: 374, 381, 379, 385, 375
With patch: 375, 389, 382, 379, 380

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 616312 [details] [diff] [review]
patch

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

Looks good. Reminder: we need tests for our frecency code :-)

::: mobile/android/base/db/LocalBrowserDB.java
@@ +167,5 @@
>          // approximation using the Cauchy distribution: multiplier = 15^2 / (age^2 + 15^2).
>          // Using 15 as our scale parameter, we get a constant 15^2 = 225. Following this math,
>          // frecencyScore = numVisits * max(1, 100 * 225 / (age*age + 225)). (See bug 704977)
> +        // We also give bookmarks an extra bonus boost, since they're likely more important.
> +        final String bookmarkBonus = "(CASE WHEN " + Combined.BOOKMARK_ID + " > -1 THEN 50 ELSE 1 END)";

IIRC, history entries with no bookmarks will have id = 0. I think the correct query would be Combined.BOOKMARK_ID > 0 (instead of -1).

Comment 7

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #6)

> Looks good. Reminder: we need tests for our frecency code :-)

I have a WIP in my queue for bug 736171, I should lap back to that :)

> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +167,5 @@
> >          // approximation using the Cauchy distribution: multiplier = 15^2 / (age^2 + 15^2).
> >          // Using 15 as our scale parameter, we get a constant 15^2 = 225. Following this math,
> >          // frecencyScore = numVisits * max(1, 100 * 225 / (age*age + 225)). (See bug 704977)
> > +        // We also give bookmarks an extra bonus boost, since they're likely more important.
> > +        final String bookmarkBonus = "(CASE WHEN " + Combined.BOOKMARK_ID + " > -1 THEN 50 ELSE 1 END)";
> 
> IIRC, history entries with no bookmarks will have id = 0. I think the
> correct query would be Combined.BOOKMARK_ID > 0 (instead of -1).

I was testing this against a copy of a DB I have on my desktop, and I found that the non-bookmark entries actually have null entries for bookmark_id. Looks like that comes from this outer join:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#427

bookmark_id 0 should always correspond to our fixed root folder, anyway, so this change shouldn't change the result set.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Margaret Leibovic [:margaret] from comment #4)
> Hm, maybe 100 is too high. Consider these two cases:
> - a bookmark you visited once, but today = 10000
> - a non-bookmark you visited 50 times today = 5000
> 
> I think visiting a site 50 times today makes it more important than a
> bookmark.

Yeah. Even 50 might be too large, but we could tweak that after getting some feedback on the current change. We certainly want to weight bookmarks, but not necessarily always push them to the top.

Madhava - do you want to land with 50, or try something smaller (maybe 5 or 10) and see how it feels?

Comment 9

[:Margaret Leibovic]

Following discussion on IRC, Madhava and I were thinking about trying to add the bonus factor, as opposed to multiplying it. I want to try to do some math to figure out what the differences between those approaches would be, but our theory is that adding a constant would have more of a minimum value/tie breaker effect for bookmarks. Maybe I can try making some graphs johnath-style.

Comment 10

[:Margaret Leibovic]

Attachment: patch w/ additive bonus

Sorry about being slow to lap back to this. This patch just adds 100 bonus points to the final awesomeness score if the entry is a bookmark, rather than multiplying a bonus.

I was trying to figure out how to get a feel for how right this is, and it's hard to visualize the data since there are multiple variables, but I made a spreadsheet that computes the awesomeness score for different visits/age values, and it seems like a bonus value of 100 will give us the minimum-value/tie-breaker effect we're looking for. In case you're curious, here's the data I played with:
https://docs.google.com/spreadsheet/ccc?key=0AkOgea8OE_GzdG1PYUJqRTRjMXNGTF9wN2FJTzFSVlE

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 620130 [details] [diff] [review]
patch w/ additive bonus

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

Sounds like a good approach to start with.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/mozilla-central/rev/272f4e6ce38a

Madhava and/or Ian, can you help pay attention to this to see if it has the right feel? If we like it, we can ask to uplift to aurora, since it's pretty low-risk code-wise.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 620130 [details] [diff] [review]
patch w/ additive bonus

[Triage Comment]
Very minor change, just adds 100 points to the frecency of bookmarks. Near zero risk of any regressions. If anything /at all/ looks wonky here, we can back out for our final beta next week. Please land on mozilla-aurora, mozilla-beta tip, and mozilla-beta MOBILE140_2012061216_RELBRANCH.

Comment 14

[Joe Drew (not getting mail)]

This doesn't need to land on aurora, since it's already in firefox 15 due to when it landed.

Comment 15

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-beta/rev/c2e94f8077f3
https://hg.mozilla.org/releases/mozilla-beta/rev/6ebda0cacad3