1053610 - Implement a places database

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

Currently we use an "in-memory datastore" to load all of the places results whenever the search app or new tab page loads. I believe the consensus is that we should be using indexedDB store instead. Since we are rapidly approaching the 2.1 FL date, we should do whatever the minimum amount of work necessary to meet all of our requirements are.

Comment 1

[Kevin Grandon :kgrandon]

Dale - do you know if we have an existing bug anywhere that would track this work? I'm a bit concerned about this one as it seems as it might be tricky to get right and take some iteration. If we want to do this for 2.1, can we make this a priority?

Comment 2

[Dale Harvey (:daleharvey)]

I swear I filed one for candice, but I cant find it now, I will use this for now, but I am not so worried about it, we originally planned for the end of this sprint so going to aim to get a first implementation done for tomorrow

Comment 3

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/23108

Working on the tests now, partially in combination with https://bugzilla.mozilla.org/show_bug.cgi?id=1049143 so I wanted to have this looked at while I worked on them

Francisco this is gonna conflict with your icons patch but trivially at the point we pick up the icon in search.js which has been simplified

Definitely not landing prior to the tests, but I think this is in a good enough place to be looked at

I didnt make a replacement for places_preload since that was a hack and will be working on its proper replacement next

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8476220 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23108

Dale this is awesome, thanks for knocking this out. Few things I noticed during this pass:

1 - Failing unit places_test.js unit test. I suppose we should try to keep this working while we get integration tests up, though seems that might be tricky with IDB.

2 - I'm not seeing places results in the search app. I guess we are trying to get these tests running again. Do you know if this should work?

Comment 5

[Dale Harvey (:daleharvey)]

1. Yeh I have the unit test fixed as well as new unit tests for places, thats all sorted and will be up soon

2. Should definitely be working, one thing I have noticed is that the ordering may differ now that we arent pulling them from memory, but ill take a look before I up the 2nd version with tests

Comment 6

[Dale Harvey (:daleharvey)]

Comment on attachment 8476220 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23108

Fixed places.js unit tests and added new tests for the screenshot functionality, plus new tests for places_idb.js

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8476220 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23108

I think something like this would be fine for now, thanks for knocking it out. Is this the proper algorithm for frecency?

Comment 8

[Dale Harvey (:daleharvey)]

No, firefox proper uses a decaying algorithm for places, this doesnt change the behaviour of the frecency which is the same as we had in the previous browser, since the fxos implementation is vastly different from desktop (one result per host etc) not sure how much of an impact the decay will have, but can look in a follow up

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8476220 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23108

I saw that you took into account the ugly patch for passing the icon url, don't have anything else to add here.

Excellent job!

Comment 10

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/fac2363c9dba7d3fc7d1965dbf555abcd36c3980

I took out test/providers/places.js, it was a single test that is much better covered in places_idb_test.js, the way the places test was setup meant it touched too many things and was extremely brittle