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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Grisha Kruglov]

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.

Comment 1

[:Grisha Kruglov]

Attachment: MozReview Request: Bug 1265525 - Part 1: Combined view migration, add local/remote visit aggregates r=sebastian

Review commit: https://reviewboard.mozilla.org/r/47245/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47245/

Comment 2

[:Grisha Kruglov]

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/

Comment 3

[:Grisha Kruglov]

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/

Comment 4

[:Grisha Kruglov]

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/

Comment 5

[:Grisha Kruglov]

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/

Comment 6

[:Grisha Kruglov]

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/

Comment 7

[:Grisha Kruglov]

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 8

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(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. :)

Comment 10

[:Grisha Kruglov]

Attachment: MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

Review commit: https://reviewboard.mozilla.org/r/47469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47469/

Comment 11

[:Grisha Kruglov]

Attachment: MozReview Request: Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Review commit: https://reviewboard.mozilla.org/r/47471/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47471/

Comment 12

[:Grisha Kruglov]

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/

Comment 13

[:Grisha Kruglov]

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 14

[:Grisha Kruglov]

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/

Comment 15

[:Grisha Kruglov]

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 16

[:Grisha Kruglov]

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 17

[:Grisha Kruglov]

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 18

[:Grisha Kruglov]

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 19

[:Grisha Kruglov]

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 20

[:Grisha Kruglov]

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 21

[:Grisha Kruglov]

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 22

[:Grisha Kruglov]

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 23

[:Grisha Kruglov]

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 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8742896 [details]
MozReview Request: Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

https://reviewboard.mozilla.org/r/47469/#review44523

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Chart: Local frecency vs. Remote frecency

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Charts: Visits * Frecency Multiplicator (Local vs. Remote)

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 28

[:Grisha Kruglov]

Attachment: Local vs Remote Frecency, formula as is

Comment 29

[:Grisha Kruglov]

Attachment: Local vs Remote Frecency, formula without +2 for local visit count

Comment 30

[:Grisha Kruglov]

Attachment: Local vs Remote Frecency, very old visits

Comment 31

[:Grisha Kruglov]

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.

Comment 32

[:Grisha Kruglov]

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.

Comment 33

[:Grisha Kruglov]

Attachment: LocalVsRemoteVeryOldLocalMin1.png

Comment 34

[:Grisha Kruglov]

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 35

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 36

[:Grisha Kruglov]

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

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://treeherder.mozilla.org/logviewer.html#?job_id=9032826&repo=fx-team because it seems to be the cause of https://treeherder.mozilla.org/logviewer.html#?job_id=9032826&repo=fx-team

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Oops, clipboard fail. Backout was in https://hg.mozilla.org/integration/fx-team/rev/4d32f073e413

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This failure also started up on your push: https://treeherder.mozilla.org/logviewer.html#?job_id=9033174&repo=fx-team

Comment 40

[:Grisha Kruglov]

Attachment: MozReview Request: Bug 1265525 - Pre: move history expirations tests to junit4 r=mcomella

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/

Comment 41

[:Grisha Kruglov]

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 42

[:Grisha Kruglov]

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 43

[:Grisha Kruglov]

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/

Comment 44

[:Grisha Kruglov]

Pushed a fix; thanks for catching this.

Comment 45

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 46

[:Grisha Kruglov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4ae0907ff61

Comment 47

[:Grisha Kruglov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08f8f695d50d

Comment 48

[:Grisha Kruglov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=294b31dfc78b

Comment 49

[:Grisha Kruglov]

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/

Comment 50

[:Grisha Kruglov]

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 51

[:Grisha Kruglov]

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 52

[:Grisha Kruglov]

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 53

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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?

Comment 54

[:Grisha Kruglov]

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 55

[:Grisha Kruglov]

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 56

[:Grisha Kruglov]

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/

Comment 57

[:Grisha Kruglov]

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.

Comment 58

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/540002819545a72938cd4add8bb2268490870b35
Bug 1265525 - Pre: move BrowserContract tests to junit4 r=sebastian

Comment 59

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/f956d7fa4626295324504a6bd091b1113788eb64
Bug 1265525 - Pre: move history expirations tests to junit4 r=mcomella

Comment 60

[:Grisha Kruglov]

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

Comment 61

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/7db78c34fd5740fbe6a765c727afdce69c24f26e
Bug 1265525 - Part 2: Frecency calculation and top sites query updates r=sebastian

Comment 62

[:Margaret Leibovic]

(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).

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/540002819545
https://hg.mozilla.org/mozilla-central/rev/f956d7fa4626
https://hg.mozilla.org/mozilla-central/rev/c75d80734ed0
https://hg.mozilla.org/mozilla-central/rev/7db78c34fd57

Comment 64

[u549602]

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?

Comment 65

[Carsten Book [:Tomcat]]

yeah sure, please request approval, thanks!

Comment 66

[:Grisha Kruglov]

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.

Comment 67

[u549602]

Hi Grisha, when will this be uplifted to 48 builds (beta now)?

Comment 68

[:Grisha Kruglov]

Attachment: Uplift request for 48 (beta)

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

Comment 69

[:Grisha Kruglov]

Also requested a beta uplift for Bug 1274029 which fixes performance problems that came out of these changes.

Comment 70

[Sylvestre Ledru [:Sylvestre]]

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.