Closed Bug 1265525 Opened 4 years ago Closed 4 years ago

Local vs. remote visits: Top Sites should prefer local visits over remote ones

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- verified

People

(Reporter: Grisha, Assigned: Grisha)

References

(Blocks 1 open bug)

Details

Attachments

(11 files)

58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
54.33 KB, image/png
Details
137.60 KB, image/png
Details
156.85 KB, image/png
Details
125.57 KB, image/png
Details
24.66 KB, image/png
Details
24.65 KB, image/png
Details
58 bytes, text/x-review-board-request
sebastian
: review+
mcomella
: review+
Details
254.31 KB, patch
Details | Diff | Splinter Review
Relevant iOS work for inspiration:
- https://github.com/mozilla/firefox-ios/blob/master/Storage/SQL/SQLiteHistory.swift#L516

Involves modification of the combined* views, and our frecency calculations.
Possible performance impact for large Sync users stemming from additional JOIN on the visits table.
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Depends on: 1046709
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/1-2/
Attachment #8742520 - Attachment description: MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 → MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/2-3/
Attachment #8742520 - Attachment description: MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates → MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r?sebastian
Attachment #8742520 - Flags: review?(s.kaspari)
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/3-4/
Attachment #8742520 - Attachment description: MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r?sebastian → MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r?sebastian
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/4-5/
Attachment #8742520 - Attachment description: MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r?sebastian → MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r?sebastian
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/5-6/
Attachment #8742520 - Attachment description: MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r?sebastian → MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r?sebastian
Sebastian, apologies for the email barrage; even though I have all of the changes ready to go for this, I'm realizing that I first need to land patches in Bug 1046709 first.
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

https://reviewboard.mozilla.org/r/47245/#review44099
Attachment #8742520 - Flags: review?(s.kaspari) → review+
(In reply to :Grisha Kruglov from comment #7)
> Sebastian, apologies for the email barrage; even though I have all of the
> changes ready to go for this, I'm realizing that I first need to land
> patches in Bug 1046709 first.

You should be able to push ranges with -r firstChangeset::lastChangeset or single commits with -c changeset to reviewboard - so if the patches already exist then it should be able to get them into reviewboard. :)
Review commit: https://reviewboard.mozilla.org/r/47469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47469/
Attachment #8742520 - Attachment description: MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r?sebastian → MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r?sebastian
Attachment #8742896 - Flags: review?(s.kaspari)
Attachment #8742897 - Flags: review?(s.kaspari)
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/6-7/
There's something really weird going on. Pushing individual commits with -c replaces what's already up for review with that one commit. Pushing a range of commits using -r kind of works, but I ended up missing the Pre commit, and duplicating the "Part 1" :/
Comment on attachment 8742896 [details]
MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47469/diff/1-2/
Attachment #8742896 - Attachment description: MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r?sebastian → MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r?sebastian
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/7-8/
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47471/diff/1-2/
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/8-9/
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47471/diff/2-3/
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/9-10/
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47471/diff/3-4/
Comment on attachment 8742896 [details]
MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47469/diff/2-3/
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/10-11/
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47471/diff/4-5/
Comment on attachment 8742896 [details]
MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

https://reviewboard.mozilla.org/r/47469/#review44523
Attachment #8742896 - Flags: review?(s.kaspari) → review+
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

https://reviewboard.mozilla.org/r/47471/#review45781

Technically this looks good but I have some doubts that the new frecency will lead to much differences. How did you choose the numbers?

Problems I see:
* For age 0 both multiplicators are 100 (attachment 8745200 [details]). You kind of fix that by adding 2 to the local visits. But this advantage is getting smaller the more visits you have (attachment 8745253 [details])
* The local frecency results in a higher multiplicator (attachment 8745200 [details]), but already one remote visits more (or 3 if you include the +2 local visits) is enough to catch up.

Our goal is that sites that get visited frequently on desktop but not (often) on mobile (For me this is for example Bugzilla) are ranked lower than sites that are visited frequently on mobile. So I wonder if we should be way more aggressive and for example use the same frecency calculation but divide the results after the multiplication with visits by 2 or 3 (Just a guess at this point; I'd like to create some charts for that too). What do you think?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:86
(Diff revision 5)
> +    static public String getRemoteFrecencySQL(final long now) {
> +        return getFrecencyCalculation(now, 1, 110, Combined.REMOTE_VISITS_COUNT, Combined.REMOTE_DATE_LAST_VISITED);
> +    }

How did you come up with 110? It's almost the half of 225 but not exactly. :)

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:98
(Diff revision 5)
> +     *
> +     * @param now Base time in milliseconds for age calculation
> +     * @return local frecency SQL calculation
> +     */
> +    static public String getLocalFrecencySQL(final long now) {
> +        String visitCountExpr = "(" + Combined.LOCAL_VISITS_COUNT + " + 2)";

Why did you add 2 here? I assume you want to weight the local visits heigher - but shouldn't this be done with the frecency multiplicator? The more visits you have for a particular site the less will 2 additional visits influence the result. Is this intentional?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:101
(Diff revision 5)
> +     */
> +    static public String getLocalFrecencySQL(final long now) {
> +        String visitCountExpr = "(" + Combined.LOCAL_VISITS_COUNT + " + 2)";
> +        visitCountExpr = visitCountExpr + " * " + visitCountExpr;
> +
> +        return getFrecencyCalculation(now, 2, 225, visitCountExpr, Combined.LOCAL_DATE_LAST_VISITED);

I assume you picked 2 so that the min. frecency multiplier will always be higher than the remote frecency one?

It looks like this will only have an effect after 105 days (2 vs. 1). With just 1 for both the local frecency will still be higher until both are 1 after 150 days.

After 150 days it doesn't really matter if it was local or remote, it's probably of no interest anyways. So I think we can just always use a min. of 1?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:113
(Diff revision 5)
> +     * @param maxFrecency Maximum allowed frecency value
> +     * @param multiplier Scale constant
> +     * @param visitCountExpr Expression which will produce a visit count
> +     * @param lastVisitExpr Expression which will produce "last-visited" timestamp
> +     * @return Frecency SQL calculation
> +     */
> +    static public String getFrecencyCalculation(final long now, final int maxFrecency, final int multiplier, @NonNull  final String visitCountExpr, @NonNull final String lastVisitExpr) {

maxFrecency: You pour the value into MAX() but technically it's the minimum frecency.
Attachment #8742897 - Flags: review?(s.kaspari)
I think you've missed that for local visit count, we're not just adding 2, but also squaring the result - which makes a huge difference. So visitCountForCalculation = (visitCount+2)*(visitCount+2).

I've attached some graphs of frecency values for different visit count/age combinations, remote vs local. I think asymmetric visit graphs are more interesting than just static ones - e.g. 20 remote visits vs 5 local visits - that is what we're concerned with optimizing in particular.

(attachment 8745795 [details]) -> formula as is, local visit count is (VISITS+2)*(VISITS+2)
(attachment 8745796 [details]) -> without +2, so local visit count is just VISITS^2.
(attachment 8745797 [details]) -> raw frecency values for very old visits (100, 200, 500 days). As you can see, local always heavily wins.

Steep drop off in frecency on every graph is when age switches to weeks. Up to that it's 1 day, 2 days, ... 7 days.

Specific scaling/etc values were take from what iOS is doing for their local/remote frecency weighting, I figured it's as good of a starting point as any! And looking at these graphs, I think it works well enough.
I almost wonder if local visits are weighted too aggressively... Look at the 150 remote visits vs 10 local visits - remote ones win, but not for all age values! But I suppose that's the goal here.
For the misnamed maxFrecency variable (yes, it's the min frecency!), you're right that it only starts to play a role for old visits. Here's the data when minimum value is 1, instead of 2: (attachment 8745801 [details]) - that probably works ok for the local visits, even for fairly asymmetrical comparisons.

And compare this with data when minimum value is 2: attachment 8745797 [details] -> local visits are weighted even more aggressively, more inline with how the relationship is for low age values.
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

https://reviewboard.mozilla.org/r/47471/#review46043

You are right, I missed looking at the squared result. This looks much better. Let's get this landed and see how it performs in Nightly. The 49 cycle just started, so we have some time to play with the numbers.
Attachment #8742897 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/0f659fc840cd218a8ffd6c8b79a49a3e6166ad3d
Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/d34dc8387cd4cc4e018bd358488dd3d9c8e9dfa4
Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/64ff3f5121d75d61e6f6a9798240f7a79332d006
Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian
Otherwise if there are no visits for a given history record, it won't be included in the results.

Review commit: https://reviewboard.mozilla.org/r/49381/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49381/
Attachment #8742896 - Attachment description: MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r?sebastian → MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian
Attachment #8742520 - Attachment description: MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r?sebastian → MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian
Attachment #8742897 - Attachment description: MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r?sebastian → MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian
Attachment #8746341 - Flags: review?(s.kaspari)
Comment on attachment 8742896 [details]
MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47469/diff/3-4/
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/11-12/
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47471/diff/5-6/
Pushed a fix; thanks for catching this.
Flags: needinfo?(gkruglov)
Comment on attachment 8746341 [details]
MozReview Request: Bug 1265525 - Pre: move history expirations tests to junit4 r=mcomella

https://reviewboard.mozilla.org/r/49381/#review46237
Attachment #8746341 - Flags: review?(s.kaspari) → review+
Comment on attachment 8742896 [details]
MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47469/diff/4-5/
Attachment #8746341 - Attachment description: MozReview Request: Bug 1265525 - Use "history left outer join with visits" r=sebastian → MozReview Request: Bug 1265525 - Pre: move history expirations tests to junit4 r=mcomella
Attachment #8746341 - Flags: review?(michael.l.comella)
Comment on attachment 8746341 [details]
MozReview Request: Bug 1265525 - Pre: move history expirations tests to junit4 r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49381/diff/1-2/
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/12-13/
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47471/diff/6-7/
Comment on attachment 8746341 [details]
MozReview Request: Bug 1265525 - Pre: move history expirations tests to junit4 r=mcomella

https://reviewboard.mozilla.org/r/49381/#review46863

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:2
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

nit: I think we should use the MPL. Nick & Richard used the public domain when they did background sync, which was fine because it was in a separate repo, but now that we're back in m-c, I think it's better to be consistent.

...unless you're particularly passionate about it, in which case I'd say make a case for it! :)

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:20
(Diff revision 2)
> +import org.mozilla.gecko.background.testhelpers.TestRunner;
> +import org.robolectric.shadows.ShadowContentResolver;
> +
> +import static org.junit.Assert.*;
> +
> +@RunWith(TestRunner.class)

nit: Java style puts the javadoc over the annotations. Here and below.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:40
(Diff revision 2)
> +        super.setUp();
> +        final ShadowContentResolver cr = new ShadowContentResolver();
> +        thumbnailClient = cr.acquireContentProviderClient(BrowserContract.Thumbnails.CONTENT_URI);
> +        thumbnailTestUri = testUri(BrowserContract.Thumbnails.CONTENT_URI);
> +        expireHistoryNormalUri = testUri(BrowserContract.History.CONTENT_OLD_URI).buildUpon()
> +                .appendQueryParameter(BrowserContract.PARAM_EXPIRE_PRIORITY, "NORMAL").build();

nit: I'd prefer `BrowserContract.ExpirePriority.NORMAL.toString()` because it's a little safer, but I can go either way.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:55
(Diff revision 2)
> +        final int historyItemsCount = 3000;
> +        insertHistory(historyItemsCount, System.currentTimeMillis());
> +
> +        historyClient.delete(expireHistoryAggressiveUri, null, null);
> +
> +        // Aggressive expiration should leave 500 history items

nit: Why should it leave 500 history items? Granted, I'm not sure how the algorithm is supposed to work, but it'd be good to document why you think that. If the algorithm changes in the future, it'd be good to know the assumptions baked into the tests.

Some of the comments below could also use an explanation.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:59
(Diff revision 2)
> +
> +        // Aggressive expiration should leave 500 history items
> +        Cursor c = historyClient.query(historyTestUri, null, null, null, null);
> +        assertNotNull(c);
> +        assertEquals(500, c.getCount());
> +        c.close();

Are you sure it's okay to not wrap the Cursor in a `try {} finally`?

I tried googling about "closing cursors during tests" and the only relevant thing I got back was that there's a maximum-openable Cursor limit. In theory, if we throw assertion errors from `assert*`, before we close the Cursor, we can run into this issue. As such, unless you have a reason to believe otherwise, I'd prefer to wrap it in `try finally`.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:62
(Diff revision 2)
> +        c = historyClient.query(thumbnailTestUri, null, null, null, null);
> +        assertNotNull(c);
> +        assertEquals(15, c.getCount());

nit: This pattern repeats itself a lot – this could be a good helper method (and then you don't need to tediously close all the cursors!). I envision:

assertRowCount(Uri, count) {
  Cursor c = ...;
  // ...
}

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:97
(Diff revision 2)
> +    /**
> +     * Test aggressive expiration on old history items
> +     */
> +    public void testHistoryExpirationAggressiveOld() throws Exception {
> +        final int historyItemsCount = 3000;
> +        final long threeMonths = 1000L * 60L * 60L * 24L * 30L * 3L;

nit: Use `TimeUnit` class. This is sued in multiple tests so consider making it static.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:141
(Diff revision 2)
> +        c.close();
> +    }
> +
> +    /**
> +     * Insert <code>count</code> history records with thumbnails, and for a third of records insert a visit.
> +     * Inserting visits only for some of the history records is in order to catch

nit: in order to catch what?

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:157
(Diff revision 2)
> +        // inserting a new entry sets the date created and modified automatically
> +        // reset all of them

nit: you should probably mention this functionality in the method javadoc and the motivation of why.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:169
(Diff revision 2)
> +                    new String[] { "https://www.mozilla" + i + ".org" });
> +        }
> +
> +        // insert thumbnails for history items
> +        ContentValues[] thumbs = new ContentValues[count];
> +        for (int i = 0; i < count; i++) {

nit: we loop over the same values 3 times – I think it might be more readable to loop once (and certainly more performant).

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryVisitsTestBase.java:56
(Diff revision 2)
> +
> +    protected Uri insertHistoryItem(String url, String guid, Long lastVisited) throws RemoteException {
>          ContentValues historyItem = new ContentValues();
>          historyItem.put(BrowserContract.History.URL, url);
>          historyItem.put(BrowserContract.History.GUID, guid);
> +        historyItem.put(BrowserContract.History.DATE_LAST_VISITED, lastVisited);

Before we omitted this entirely and the tests worked but now we need to have an explicit insert – why is that? Without it, are we still testing everything we need to test?
Attachment #8746341 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8746341 [details]
MozReview Request: Bug 1265525 - Pre: move history expirations tests to junit4 r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49381/diff/2-3/
Comment on attachment 8742520 [details]
MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47245/diff/13-14/
Comment on attachment 8742897 [details]
MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47471/diff/7-8/
https://reviewboard.mozilla.org/r/49381/#review46863

> nit: I think we should use the MPL. Nick & Richard used the public domain when they did background sync, which was fine because it was in a separate repo, but now that we're back in m-c, I think it's better to be consistent.
> 
> ...unless you're particularly passionate about it, in which case I'd say make a case for it! :)

No preference.

> nit: I'd prefer `BrowserContract.ExpirePriority.NORMAL.toString()` because it's a little safer, but I can go either way.

Yeah, I like this.

> nit: we loop over the same values 3 times – I think it might be more readable to loop once (and certainly more performant).

Not doing bulkInsert for thumbnails is consistently a few seconds slower in my local runs, so keeping that bit - so it's 2 loops now.

> Before we omitted this entirely and the tests worked but now we need to have an explicit insert – why is that? Without it, are we still testing everything we need to test?

We weren't testing history expiration before, and other tests weren't concerned with the last_visited timestamp.  You're correct in that we could expand test coverage further, so I'm making a note of that. Bug 1269492 could be one vehicle for those improvements.
https://hg.mozilla.org/integration/fx-team/rev/540002819545a72938cd4add8bb2268490870b35
Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian
https://hg.mozilla.org/integration/fx-team/rev/f956d7fa4626295324504a6bd091b1113788eb64
Bug 1265525 - Pre: move history expirations tests to junit4 r=mcomella
https://hg.mozilla.org/integration/fx-team/rev/c75d80734ed06686d1a7a381a33ca73e482d0528
Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian
https://hg.mozilla.org/integration/fx-team/rev/7db78c34fd5740fbe6a765c727afdce69c24f26e
Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian
(In reply to Michael Comella (:mcomella) from comment #53)
> Comment on attachment 8746341 [details]
> MozReview Request: Bug 1265525 - Pre: move history expirations tests to
> junit4 r=mcomella
> 
> https://reviewboard.mozilla.org/r/49381/#review46863
> 
> :::
> mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/
> BrowserProviderHistoryTest.java:2
> (Diff revision 2)
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> nit: I think we should use the MPL. Nick & Richard used the public domain
> when they did background sync, which was fine because it was in a separate
> repo, but now that we're back in m-c, I think it's better to be consistent.
> 
> ...unless you're particularly passionate about it, in which case I'd say
> make a case for it! :)

Drive-by comment: I think all tests in the tree are supposed to be public domain. You can look at the tests in other directories to see this (not sure if this is documented somewhere, gerv would have a real answer if you're looking for one).
Verified as fixed on the latest Nightly build (49.0.a1 2016-05-08) on a Sony Xperia Z2 with Android 5.1.1 and on a Samsung Galaxy S7 Edge with Android 6.0.1. 
Carsten, can we uplift this to Aurora?
Flags: needinfo?(cbook)
yeah sure, please request approval, thanks!
Flags: needinfo?(cbook)
Flags: needinfo?(gkruglov)
My main concern with these patches is their performance impact - it's slowing down how quickly the Top Sites panel is able to load. Looking at the telemetry data since these changes landed, roughly speaking the query is twice as slow 95% of the time.

Bug 1274029 tracks work (specifically, caching of top sites query results) necessary to get query load numbers back down.

We could uplift to get a better sense of performance impact and/or expose other potential issues. Ideally we'd then also need to uplift whatever work comes out of Bug 1274029.
Flags: needinfo?(gkruglov)
QA Contact: mihai.ninu
Hi Grisha, when will this be uplifted to 48 builds (beta now)?
Flags: needinfo?(gkruglov)
Approval Request Comment

[Feature/regressing bug #]: user-visible part of the work to modify data behind Top Sites to prefer websites visited locally over those that were visited remotely (i.e. desktop, other devices) (see meta Bug 1265516). Necessary data-layer updates are already in 48.

[User impact if declined]: when syncing history, visit/history data that comes in from Sync and that was generated on other devices (e.g. desktop) is likely to overwhelm visit data generated locally. User-visible impact is that Top Sites are arguably less useful without this change, since Top Sites will display websites visited remotely, not the ones user visits on the device.

[Describe test coverage new/current, TreeHerder]: unit tests for the new frecency ordering statements; unit tests to cover potential regressions from changes to the combined view; manual testing & user feedback from nightly users.

[Risks and why]: low-medium; these changes have been in nightly for a while now, and so far only caused Top Sites/awesomebar search loading performance regression. This patch must be uplifted together with Bug 1274029 which fixes the performance regression. 

[String/UUID change made/needed]: none
Flags: needinfo?(gkruglov)
Attachment #8761363 - Flags: approval-mozilla-beta?
Also requested a beta uplift for Bug 1274029 which fixes performance problems that came out of these changes.
Comment on attachment 8761363 [details] [diff] [review]
Uplift request for 48 (beta)

As discussed in bug 1274029, I prefer this to ride the train from 49.
Attachment #8761363 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.