1191978 - No thumbnail in browser

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

User-Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Dogfooding with the same profile since day one (before 1.0), I have absolutely no thumbnail in the browser's first page. That is not happening on other devices so probably something in my data.

Comment 1

[:gerard-majax]

Attachment: 2015-08-06-22-04-27.png

Comment 2

[Sam Foster [:sfoster] (he/him)]

I have thumbnails in the new tab page from Whistler (i.e. June 2015 when I first started using the device and created the profile) but all the other thumbnails since then are blank/white.

Comment 3

[:gerard-majax]

Dale, when you have time, just tell me what you need :)

Comment 4

[Dale Harvey (:daleharvey)]

So the issue here is fairly tricky

We create and store 'places' in the system app https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/places.js#L87 and display them in https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/providers/places.js#L245

We try to keep track of the topSites in the system app @ https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/places.js#L299, if the visit is in the top sites we record a screenshot in the places database so that its ready to be displayed in the browser.

If the top sites stored in the system app are not the actual top sites then we wont get the correct screenshots generated. Now I am fairly certain looking at the code that we will drift out of sync if the user ever clears browsing data so thats one bug, but I expect it will be possible to drift out of sync for other reasons

Comment 5

[:gerard-majax]

So I have debugged a bit and it looks like we have a |null| screenshot in search/js/providers/places.js.

Comment 6

[:gerard-majax]

this.screenshotRequested(place.url) does properly update the screenshot. I have spotted, however, this.topSites seems to contain strange data.

Comment 7

[:gerard-majax]

Attachment: Capture du 2015-11-26 13-23-08.png

Looks like asyncStorage topSites contains something different than what is shown in top sites UI

Comment 8

[:gerard-majax]

Attachment: b2g_topsites.png

Matching topSites. Is it expected?

Comment 9

[:gerard-majax]

So, search app makes use of "frecency" store to populate the top website in the browser app. That seems in discrepency with asyncStorage("top-sites") within the system app, but the behavior might be the one we want. I am just wondering about the way we handle updating top websites from withtin the system app since we rely on last top frecency value from asyncStorage("top-sites"): I see values above 400 (like documented in comment 7/attachment 8692481 [details]). Other websites within the "top sites" stored in search app is for example around 50-60.

Consequence of that is that we are not performing update of "top-sites" asyncStorage (which would update the screenshot and fix my bug).

Comment 10

[:gerard-majax]

Attachment: topSites updat

Comment 11

[:gerard-majax]

Attachment: Avoid duplicated URLs in asyncStorage topSites

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] lissyx:bug1191978 > mozilla-b2g:master

Comment 13

[:gerard-majax]

First part is completed: at load time, we will remove dupes. I'll continue by changing |checkTopSites| to make it NOT add dupes

Comment 14

[:gerard-majax]

Comment on attachment 8692594 [details] [review]
[gaia] lissyx:bug1191978 > mozilla-b2g:master

Looks like it's all working as expected :)

Comment 15

[Dale Harvey (:daleharvey)]

Comment on attachment 8692594 [details] [review]
[gaia] lissyx:bug1191978 > mozilla-b2g:master

Clearing review for mostly minor code formatting issues. I think it could be a tad cleaner but its doing the right thing and the tests are great, thanks a lot.

Comment 16

[:gerard-majax]

Comment on attachment 8692594 [details] [review]
[gaia] lissyx:bug1191978 > mozilla-b2g:master

Your approach is easier to read, my mind was blown to write such complex code :). It's passing tests locally, I'm verifying on real data on device but it should be good.

Comment 17

[:gerard-majax]

Looks good on device

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 8692594 [details] [review]
[gaia] lissyx:bug1191978 > mozilla-b2g:master

Looks awesome, thanks a lot

Comment 19

[:gerard-majax]

full green https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=8094aaea2492d9acb4958c2f3bebbfb78fd29992

Comment 20

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/09c3d2f593799fa781b94006d3389e9cbb156c6e