1071057 - [Top Sites] Once flashed "gaia" with a topsite in resources/topsites_movistar.json file, User can see this topsite

Status: RESOLVED FIXED

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

Description

[Loli (:lolimartinezcr)]

2.1
flame
User
Gecko-bdd77c4
Gaia-b3f9b97

Pre-requisites:
Having a variant.json configured for our sim card with a line similar to: "topsites": "resources/topsites_movistar.json"

Having a file "resources/topsites_movistar.json" with at least a topsite configured

Having a configured sim card with pin

Without data connection.

STRs:
1. Completing the FTE with a configured sim with pin inserted
2. Open browser app
3. Open top sites tab

Actual result:
Only user can see default topside (about mozilla) and other about warning about "without connection". 

When user actives the connection and user can't see topside configured in "topsites_movistart.json"

Expected result: 
Operator Top sites are inserted and displayed in Top Sites list

Comment 1

[Kevin Grandon :kgrandon]

Loli - Can you attach the topsites_movistar.json file you've used here?

Comment 2

[Dale Harvey (:daleharvey)]

I am thinking this is a feature request, sim customisation of topsites hasnt been implemented

Comment 3

[Kevin Grandon :kgrandon]

(In reply to Dale Harvey (:daleharvey) from comment #2)
> I am thinking this is a feature request, sim customisation of topsites hasnt
> been implemented

Right, that makes sense. And it looks like from the bug description this was referencing the old browser.

Comment 4

[Marcelino Veiga Tuimil [:sonmarce]]

This was tested with system browser. It was implemented in old browser app, and it should have been migrated to system browser like all other features, so in my opinion this a regression.

Comment 5

[Loli (:lolimartinezcr)]

Attachment: topsites_movistar.json

Comment 6

[Marcelino Veiga Tuimil [:sonmarce]]

[Blocking Requested - why for this release]: This is a regression, work well in 2.0, now broken in 2.1

Comment 7

[Gregor Wagner [:gwagner]]

Feature regression.

Comment 8

[Dale Harvey (:daleharvey)]

I am having problems building a distribution with customisations

https://gist.github.com/daleharvey/e81d199acae04db2f832 

is a lot with all the relevant files shown, there is no operatorvariant app in the build_stage, any ideas?

(same results with make production)

Comment 9

[Dale Harvey (:daleharvey)]

So there was was a console error in there, but mostly didnt realise you had to customise everything or nothing, it bailed out with a plain custimisation and modified the current one instead

Comment 10

[Dale Harvey (:daleharvey)]

So I meant to unneedinfo Carmen on the last comment, but now re needinfoing :)

So I am just testing with 

$ GAIA_DISTRIBUTION_DIR=/Volumes/firefoxos/gaia/customization/ make reset-gaia

which has a bunch of top sites there, my plan is to reuse that settings data from the original browser, however when I run that, topsitescustomiser never runs while I am going through the ftu, and the setting is never populated, any idea?

Comment 11

[Carmen Jimenez Cabezas]

The variant.json file you linked to on comment 8 is wrong. Although on comment 9 you say you've fixed it, you haven't linked the new one so I can't really say what might be happening. In any case, you have a set of test files (a complete GAIA_DISTRIBUTION_DIR) on [1]. You can use that as base for your own tests.

If you don't want to use that one, there's another sample variant.json file on [2]

That said, I believe that what might be happening is that you fixed the build error just adding an apps object to the variant.json file, but that you haven't assigned the topsites to the correct mcc-mnc for your SIM card.

A basic variant.json file should look like: 

{
  "apps": {
   ...
  },
  "operators": [
    {
      "id": "mobizilla",
      "mcc-mnc": [
        "310-260"  <-- Important. Put here your operator mcc-mnc value/s
      ],
      "apps": [
       ...
      ],
      "topsites": "mobizilla/mobizilla_topsites.json"
    }
   ]
}

Also, you have documentation for the variant.json file on [3]


[1] https://github.com/telefonicaid/firefoxos-gaia-testsbuild
[2] https://github.com/mozilla-b2g/gaia/blob/master/customization/variant.json
[3] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide#Other_single_variant_customizations

Comment 12

[Dale Harvey (:daleharvey)]

Thanks, the issue was I was forgetting to add my mnc / mcc to the customisation variant, cheers all working now and should have a patch up shortly

Comment 13

[Dale Harvey (:daleharvey)]

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

Reuses the existing custimisation files so should be all good for existing customisations (assets may need changed)

Comment 14

[Marcelino Veiga Tuimil [:sonmarce]]

Redirected feedback to Albert

Comment 15

[Dale Harvey (:daleharvey)]

Updated the PR to not do a runtime modification and use the new format only

Comment 16

[Kevin Grandon :kgrandon]

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

This looks fine to me. Thanks!

Comment 17

[Albert [:albert]]

IMHO we should be consistent about the format, bookmarks and topsites represent different concepts at high level but both have the same definition. And now with your last changes we have that bookmarks are defined as:

"bookmarks": [
  {
    "title": "Mozilla Marketplace",
    "uri": "https://marketplace.firefox.com",
    "iconPath": "mobizilla/mobizilla_market_icon.png"
  }
]

and topsites as:

"topSites": [
  {
    "title": "Mozilla Marketplace",
    "url": "https://marketplace.firefox.com",
    "tilePath": "mobizilla/mobizilla_market_icon.png"
  },
]

From my point of view, both should have the same format.

Comment 18

[Dale Harvey (:daleharvey)]

I would prefer so as well, however we already have topsites build customisations in the above topsites sim customisation in that format and its more important build / sim customisations to have the same format than bookmark vs topsites.

Comment 19

[Marcelino Veiga Tuimil [:sonmarce]]

I do not understand well this change in top sites configuration. I think we should keep the same format as in version 2.0 [1], otherwise it would be confusing for OEMs to customize their builds. Is there any special reason to change format?

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide#Pre-populate_browser_top_sites

Comment 20

[Dale Harvey (:daleharvey)]

Its consistency and accuracy with the terms used in the internal implementation. The topSites build customisation changed a while ago and used more accurate terms (uri vs url) which also matches the internal representation of the places also using |tile| over |icon|, which again is more accurate.

The gaia/customization/* files contain documentation for the change and mdn will be updated. its a small change and I think its better to just update bookmarks to use url next time its touched (the tile vs icon difference is totally file)

We arent going to go back and change the entire implementation of places so we can keep |uri|, and having the customisation match up to its implementation that to be backwards compatible with previous customizations is more important imo

Comment 21

[Albert [:albert]]

(In reply to Dale Harvey (:daleharvey) from comment #20)
> Its consistency and accuracy with the terms used in the internal
> implementation. The topSites build customisation changed a while ago and
> used more accurate terms (uri vs url) which also matches the internal
> representation of the places also using |tile| over |icon|, which again is
> more accurate.
> 
> The gaia/customization/* files contain documentation for the change and mdn
> will be updated. its a small change and I think its better to just update
> bookmarks to use url next time its touched (the tile vs icon difference is
> totally file)
> 
> We arent going to go back and change the entire implementation of places so
> we can keep |uri|, and having the customisation match up to its
> implementation that to be backwards compatible with previous customizations
> is more important imo

then is it possible to modify bookmarks in order to be consistent with topsites?

Comment 22

[Dale Harvey (:daleharvey)]

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

Carrying r+ from kevin, fixed a typo in the build test file and waiting for a green run (Gij failure looks unrelated)

Comment 23

[Dale Harvey (:daleharvey)]

> then is it possible to modify bookmarks in order to be consistent with topsites?

Yup, I will file a follow up after this lands

Comment 24

[Albert [:albert]]

(In reply to Dale Harvey (:daleharvey) from comment #23)
> > then is it possible to modify bookmarks in order to be consistent with topsites?
> 
> Yup, I will file a follow up after this lands

great, thank you

Comment 25

[Dale Harvey (:daleharvey)]

All green run, landed @ https://github.com/mozilla-b2g/gaia/commit/ca05b8a8020096c205dfe26b671eb1a2261360aa

Albert, thinking about it, calling the bookmark a 'uri' is fairly valid, places have to be a url, they are where someone has actually visited, but bookmarks can get sent elsewhere etc, and as previously mentioned topSites use tiles, bookmarks use icons, so that definitely shouldnt change.

If someone wanted to change uri to url in the bookmark customisation then I wouldnt complain, but I think they are actually fine being inconsistent here, they represent different things.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Please nominate this patch for Gaia v2.1 approval when you get a chance :)

Comment 27

[Dale Harvey (:daleharvey)]

Yup, just gonna give it a few hours to settle on master and make sure all is green first, will leave needinfo to remind me

Comment 28

[Dale Harvey (:daleharvey)]

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

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Missing feature regressed by system browser
[User impact] if declined: Carriers wont be able to do sim customisations in the browser
[Testing completed]: integration tests added and manual testing
[Risk to taking this patch] (and alternatives if risky): fairly low risk
[String changes made]: none

Comment 29

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/b26068437f21d5baf347ad701f55bbc5533162c2