1083197 - [Top sites] Order in top sites isn't correct.

Status: VERIFIED FIXED

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

Description

[Loli (:lolimartinezcr)]

Flame
User
2.2
Gecko-72c1b1d
Gaia-bc8be50

Pre-requisites:
1. Top sites establised 

In my case it was top sites:
{
  "topSites": [
    {
      "title": "Facebook",
      "url": "https://m.facebook.com/",
      "tilePath": "resources/movistar.ico"
    },
    {
      "title": "Betis",
      "url": "http://www.realbetisbalompie.es/es/",
      "tilePath": "resources/movistar.ico"
    },
    {
      "title": "Marca",
      "url": "http://www.marca.com/",
      "tilePath": "resources/movistar.ico"
    }


  ]
}


STRs:
1. Write some url (for example: www.hotmail.com) -> It is in first position.
2. Tap in a top site (for example: http://www.marca.com/)
3. Tap in top site of step 2 -> page is opened
4. Again tap in top site of step 2 -> page is opened 

Expected result:
Top site in step 4 is in first position, after page in step 1 and the last default top site.

Actual result:
Page in step 1 is first position, after top site in step 4 and the last default top site.

Note: If user taps in *default top site* (mozilla) is in first position.

Comment 1

[Marcelino Veiga Tuimil [:sonmarce]]

[Blocking Requested - why for this release]: Asking to be a blocker, as it is a regression

Comment 2

[Dale Harvey (:daleharvey)]

Taking a look at this now, however I would like to ask that we do not flag ever minor change of behaviour as a blocker, would we not ship the device if this was not fixable? if we dont honestly believe that then flagging these as blockers is abuse of the flag imo. This is not a regression, we implemented an entirely new browser, the fact that some details may be different is not a regression.

Comment 3

[Dale Harvey (:daleharvey)]

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

Its a simple fix, we dont have an integration test but we are fairly deperately needing them for sim variant things. 

I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1083938 and will ask around about how to get a start on that

Comment 4

[Dale Harvey (:daleharvey)]

So the issue here is that the places object stored by the sim variant was missing the icons field and the places code doesnt handle that gracefully which meant places would fail to ever be able to edit that object.

Comment 5

[Kevin Grandon :kgrandon]

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

Thanks. Can we add a unit test that gets into the debouncePlaceChanges function without icons?

Comment 6

[Marcelino Veiga Tuimil [:sonmarce]]

Is it possible to land it as is (adding tests in a follow up)? This way we could test it in master.

Comment 7

[Dale Harvey (:daleharvey)]

I added the tests, however I have been pushing this for 3 days to try and get a green test run, I made a big deal about noone landing without a green run the other day so I dont really want to go and be a hypocrite, right now my pushes arent generating any try runs at all so going to debug

Comment 8

[Marcelino Veiga Tuimil [:sonmarce]]

Sorry, I did not know that, let´s wait for a green run then

Comment 9

[Dale Harvey (:daleharvey)]

Finally a green run! (I think that was near 15 tries)

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=2c8a52bf277e

Merged: https://github.com/mozilla-b2g/gaia/commit/54036558c12df2f87b6e0ba0ef2b8689cc95410a

will let settle on master then request uplift, needinfoing myself to remind

Comment 10

[Dale Harvey (:daleharvey)]

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

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 1078202
[User impact] if declined: Default top sites wont ever be modifyable
[Testing completed]: Unit tests added, settled on master (work on ability to write integration tests started)
[Risk to taking this patch] (and alternatives if risky): Extremely low risk patch, adds null checks
[String changes made]:

Comment 11

[bhavana bajaj [:bajaj]]

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

Lets get this verified once it lands on 2.1

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.1 uplift.

Comment 13

[Duane Dixon [:ddixon]]

Issue is verified fixed in Flame 2.2 (Full Flash, nightly). 

Actual Results: Top Sites in Browser app behave correctly when a new window is opened in the app. 

Device: Flame Master
Build ID: 20141022040201
Gaia: 4d7f051cede6544f4c83580253c743c22b0cb279
Gecko: ae4d9b4ff2ee
Version: 36.0a1 (Master)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Leaving 'verifyme' tag for 2.1 uplift.

Comment 14

[Dale Harvey (:daleharvey)]

This bug doesnt reproduce in 2.1, https://bugzilla.mozilla.org/show_bug.cgi?id=1068888 which diverged the places code fairly significantly hasnt been approved

Comment 15

[Duane Dixon [:ddixon]]

Issue is verified working in Flame 2.1 build (Full Flash, nightly). 

Actual Results: Top Sites behave correctly in Browser app. 

Environmental Variables:
Device: Flame 2.1
Build ID: 20141024001204
Gaia: 0f76e0baac733cca56d0140e954c5f446ebc061f
Gecko: 7d78ff7d25b6
Version: 34.0 (2.1)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0