1025829 - [Single Variant] Top Sites are not ordered correctly

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[rafael.marquez]

*Pre-requisites
topsites_movistar.json:

{
  "topSites": [
    {
      "title": "Movistar",
      "uri": "http://www.movistar.es/",
      "iconPath": "resources/movistar.ico"
    },
    {
      "title": "Betis",
      "uri": "http://www.realbetisbalompie.es/es/",
      "iconPath": "resources/pig.ico"
    },
    {
      "title": "Marca",
      "uri": "http://www.marca.com/",
      "iconPath": "resources/pig.ico"
    },
    {
      "title": "As",
      "uri": "http://as.com/",
      "iconPath": "resources/movistar.ico"
    }
  ]
}


*Procedure
1. Complete the FTE with a configured sim card
2. Open browser app

*Expected Result
Top Sites are ordered correctly: Movistar, Betis, Marca and AS

*Actual Result
Top Sites are not ordered correctly: Betis, Movistar, Marca and AS

Comment 1

[Albert [:albert]]

Currently database returns elements with same frequency sorted alphabetically in inverse order.

Comment 2

[Albert [:albert]]

Attachment: Patch

Used negative values to sort topsites.

Comment 3

[Ben Francis [:benfrancis]]

Comment on attachment 8440785 [details]
Patch

R+ with nits and failing tests fixed, thanks.

Comment 4

[Albert [:albert]]

Comment on attachment 8440785 [details]
Patch

Sorry for requesting review again Ben, but I had to modify some code because travis error was produced by a race condition. When opening the database first time callback is called before database is populated and this was causing the error.

Updated patch simplifies single variant population, instead of search default topsites in the database before saving the single variant ones, I assign a frequency of -20 (max history) to the default topsites, then I can save single variant topsites faster.

I also added small piece of code that fixes bug 1026439, when calling initTopSite if frequency for that place exists and it is bigger than 0 the topsite is not overridden.

Comment 5

[Ben Francis [:benfrancis]]

Comment on attachment 8440785 [details]
Patch

Hmm, the number 20 is a bit arbitrary and overloading the frecency value like this is a bit of hack, but let's go with this for 2.0. It will be different if we implement it in the system browser anyway.

Comment 6

[Albert [:albert]]

(In reply to Ben Francis [:benfrancis] from comment #5)
> Comment on attachment 8440785 [details]
> Patch
> 
> Hmm, the number 20 is a bit arbitrary and overloading the frecency value
> like this is a bit of hack, but let's go with this for 2.0. It will be
> different if we implement it in the system browser anyway.

I agree, I used 20 because is the default value for topsites in the awesome screen https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/js/awesomescreen.js#L13

Comment 7

[Albert [:albert]]

Master: https://github.com/mozilla-b2g/gaia/commit/f57032a24bfa9777e637045f91fa1d69971bd1a9

Comment 8

[rafael.marquez]

when you will do the merge to v2.0?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

I was on PTO last week and am still catching up.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

v2.0:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/17f2dbfd78c12dbf45ab232b4b6159080107f15f

Comment 12

[rafael.marquez]

Verified in 2.0