1020158 - Start the population process from FTU via IAC

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Description

[Cristian Rodriguez (:crdlc)]

This will be performed the first time that the device 2.0 is started or after upgrading 1.x to 2.0

Comment 1

[Ran Ben Aharon [:ranbena] EverythingMe]

This seems like a low priority task to me.

In 1.x the population occurs when Collections are inited. I assume it's going to still be like that anyway.

Comment 2

[Cristian Rodriguez (:crdlc)]

Here we need a consensus...

How I see it?

* OEMs can configure layouts saying, I want tv, social or whatever here.
* Vertical has only ids for those collections (tv and social)
* We need to pre-propulate the datastore because while the vertical homescreen is rendering, it asks to datastore to get the collection's name and icon given ids
* Obviously we need to populate the datastore with pre-installed collections while FTE (before rendering home)

The other alternative:

* OEMs can configure layouts saying, I want tv, social or whatever here.
* Vertical has ids, names and icons pre-cooked from build-time for those collections (tv and social)
* We do NOT need to pre-propulate the datastore

I want to see your opinions please today, Ran, Kevin thanks a lot.

Carmen, Albert, which alternative to see more accurate being aware of our time?

(In reply to Ran Ben Aharon (Everything.me) from comment #1)
> This seems like a low priority task to me.
> 
> In 1.x the population occurs when Collections are inited. I assume it's
> going to still be like that anyway.

Comment 3

[Kevin Grandon :kgrandon]

I am still catching up on these issues, so please excuse me if I'm wrong, but my ideal situation is:

- The Collection app has a build time script which populates a collections.json file (i think we have this). Each collection can be defined with an ID.
- FTU sends a message to the Collection app, to populate the datastore on first boot or upgrade.
- The homescreen has a grid definition which maps the IDs in the collection customization to grid positions in the homescreen.

There is a little bit of brittleness by having to map the ID between two different apps, but I think it should be doable.

This is fairly important, and as James is doing some locale work - I want to make sure he is in the loop. Adding a ni? to him here.

Comment 4

[Cristian Rodriguez (:crdlc)]

(In reply to Kevin Grandon :kgrandon from comment #3)
> I am still catching up on these issues, so please excuse me if I'm wrong,
> but my ideal situation is:
> 
> - The Collection app has a build time script which populates a
> collections.json file (i think we have this). Each collection can be defined
> with an ID.

Yes, it is

> - FTU sends a message to the Collection app, to populate the datastore on
> first boot or upgrade.

Pending work, this is how I see the implementation

> - The homescreen has a grid definition which maps the IDs in the collection
> customization to grid positions in the homescreen.

Following our approach, yes

> 
> There is a little bit of brittleness by having to map the ID between two
> different apps, but I think it should be doable.
> 
> This is fairly important, and as James is doing some locale work - I want to
> make sure he is in the loop. Adding a ni? to him here.

Comment 5

[Albert [:albert]]

(In reply to Cristian Rodriguez (:crdlc) from comment #2)
> Here we need a consensus...
> 
> How I see it?
> 
> * OEMs can configure layouts saying, I want tv, social or whatever here.
> * Vertical has only ids for those collections (tv and social)
> * We need to pre-propulate the datastore because while the vertical
> homescreen is rendering, it asks to datastore to get the collection's name
> and icon given ids
> * Obviously we need to populate the datastore with pre-installed collections
> while FTE (before rendering home)
> 
> The other alternative:
> 
> * OEMs can configure layouts saying, I want tv, social or whatever here.
> * Vertical has ids, names and icons pre-cooked from build-time for those
> collections (tv and social)
> * We do NOT need to pre-propulate the datastore
> 
> I want to see your opinions please today, Ran, Kevin thanks a lot.
> 
> Carmen, Albert, which alternative to see more accurate being aware of our
> time?
> 
> (In reply to Ran Ben Aharon (Everything.me) from comment #1)
> > This seems like a low priority task to me.
> > 
> > In 1.x the population occurs when Collections are inited. I assume it's
> > going to still be like that anyway.

IMHO first approach is more reasonable because avoids synchronization problems by having data managed by the collection application and allows the homescreen to be agnostic about collection details.

Comment 6

[Carmen Jimenez Cabezas]

On one hand, the Collections app already manages the initial load of predefined collections. That code is already landed and working (bug 1020155).
On the other hand, the homescreen app simply loads the data from a Datastore, and it doesn't really mind who loads that data or how is it generated. It would be an error for the homescreen app to have special code to manage the collections, since that's what the collections app is for. For the homescreen app they're just another kind of grid element.
The only element missing here is the trigger of the loading process. That will be a IAC message sent from the FTU to the collections app.
I can implement that part if everybody agrees on this solution.

Comment 7

[Carmen Jimenez Cabezas]

Attachment: V1 Proposed patch

Comment 8

[Fernando Campo (:fcampo)]

Comment on attachment 8440612 [details] [review]
V1 Proposed patch

same as in bug 1018854, we need unit tests, and a memory test with firebug is more than welcome, as another IAC process on FTU could be dangerous.

Comment 9

[Carmen Jimenez Cabezas]

Comment on attachment 8440612 [details] [review]
V1 Proposed patch

added unit test

Comment 10

[Fernando Campo (:fcampo)]

Comment on attachment 8440612 [details] [review]
V1 Proposed patch

Nice :) thanks for adding the unit tests.

Please add a small comment explaining why we are notify the collections, and you're free to merge

Comment 11

[Cristian Rodriguez (:crdlc)]

I think that we obtained a agreement so deleting pending ni?

Comment 12

[Carmen Jimenez Cabezas]

merged in master:
https://github.com/mozilla-b2g/gaia/commit/06ea932d52fa5ec36285428c04ad6c7fdc413659

Comment 13

[Carmen Jimenez Cabezas]

Requesting blocking 2.0 because populate preinstalled collections is essential for vertical homescreen funcionality

Comment 14

[Carmen Jimenez Cabezas]

Sorry for the mistake, this should be approval only, not blocking

Comment 15

[Carmen Jimenez Cabezas]

Comment on attachment 8440612 [details] [review]
V1 Proposed patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Vertical homescreen
[User impact] if declined: Without this bug it's not possible to predefine collections. For the users that means that new phones won't have any collections. For operators and OEMs that means they won't be able to define collections (which might impact on commercial agreements)
[Testing completed]: On the device. The patch includes unit tests also.
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Reverted for frequent OSX Gaia UI test failures on TBPL.
Master: https://github.com/mozilla-b2g/gaia/commit/adc515798fc22949c428996010b1f40ca4fd082f

https://tbpl.mozilla.org/php/getParsedLog.php?id=41884795&tree=B2g-Inbound

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> Reverted for frequent OSX Gaia UI test failures on TBPL.
> Master:
> https://github.com/mozilla-b2g/gaia/commit/
> adc515798fc22949c428996010b1f40ca4fd082f
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41884795&tree=B2g-Inbound

b2g emulator debug mochitest-4 runs started frequently failing like https://tbpl.mozilla.org/php/getParsedLog.php?id=41894617&tree=B2g-Inbound a few pushes after this bug got bumped into b2g-inbound, and appears to have stopped working soon after this revert got bumped into b2g-inbound.

Comment 18

[Carmen Jimenez Cabezas]

I believe the problem is not with this patch but with Bug 1021737. What this patch does is populate the collection database, which in turn become icons on the homescreen. And I don't think that's expected on the patch that bug 1021737 added. Zacc, can you please check this?

Comment 19

[Zac C (:zac)]

Carmen, 

If you look at the output report from tests with this patch you can see that the homescreen icons are not loaded or all mashed on top of one another:
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/b2g-inbound/sha512/e863889a7518ff5f450b64d4f854c84eb29836a327d4a350181fb66fb8faab5318ade87d418483e1024e01da55337dffafe9f0c479179a563fc9fa5676633563
If you look at the HTML (tap Source) you can see that only Collection items are there and all the usual icons have disappeared. Thus this seems to be a bug on its own.


The only possible flaw in bug 1021737 is that it assumes that there are enough elements on the homescreen that we can scroll the first few icons out of the viewport. On line 40 it tries to scroll to the bottom and as there were not enough icons on the page to extend outside the viewport. (after that the test taps the home button and checks that the homescreen zips back up to 0!) but I don't think this invalidates the test case, it's reasonable to expect all icons on the homescreen.

Comment 20

[Carmen Jimenez Cabezas]

I'm pretty sure that this patch is not the cause of the observed failure.
I think that the problem is caused by a race condition on the vertical homescreen between the grid rendering and the collection datastore being populated.
I'm still searching for the error root cause, but it hasn't failed locally for me yet.
I'm not going to try landing this again until we have found what causes the problem

Comment 21

[Kevin Grandon :kgrandon]

Attachment: Same patch, new pull request

A bunch of stuff landed yesterday. Re-opening a pull request to see what gaia-try does with it.

Comment 22

[Kevin Grandon :kgrandon]

I'm mainly going to be looking at Gu on OS X as that's what we were backed out for.

Got 1 green run so far, triggered a few more: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=cc4afdcf78f382ab7dafe49b1b4de2ad4fadfc69

Comment 23

[Kevin Grandon :kgrandon]

I'm doing a bunch of testing on this, but not much luck yet.

Could bug 1026737 be affecting this as well?

Comment 24

[Carmen Jimenez Cabezas]

Yes, bug 1026737 does affect this one because here we are requesting navigator.mozApps.getSelf() but
the effect would be that the iac message would not be sent and therefore we would not have this failure.
We would have the failure of collections not being populated, though

Comment 25

[Zac C (:zac)]

I have spent 5 hours debugging this on a mac I borrowed and run this test hundreds of times. I can replicate it fairly consistently about 1-2 in 10.
I've tried numerous variations of Gaia profile, modifying the setup of the test suite, generous hardcoded waits and so forth. I've debugged with the jsconsole and with gecko.log but I just cannot find any useful info. I can't understand what causes all of the other icons to disappear.

I may try to replicate it manually too.

Comment 26

[Zac C (:zac)]

I've now replicated it manually.
I created a shell script to copy a virgin profile to a tmp directory and start b2g using that tmp profile.
When it started up I clicked through the FTU and after about 15 attempts the Homescreen came up with just 1 collection icon for 'Showbiz'. There is nothing different in the gecko log compared to when it starts up normally.
I was clicking through the FTU a bit quickly as it's tedious to do it a dozen times.

Comment 27

[Kevin Grandon :kgrandon]

I've spent a lot of time investigating this fix as well, my hunch is that it could potentially be a platform bug. I have a try run here which applies the patch in bug 1026737. There is one error present in the same test, but it has a different assertion, but similar screenshot.

https://tbpl.mozilla.org/?tree=Try&rev=f02fcef6012d

We need this fixed today. I am going to implement a similar test in MarionetteJS to achieve the same coverage. If it works, I'm going to suggest that we disable the gaia ui test in favor of the marionetteJS test - while we look for a fix for the python test.

We should not put the entire vertical homescreen at risk because of one failing test.

Comment 28

[Zac C (:zac)]

Kevin, if you do can you disable it for mac only ? as linux versions seem stable on both Gaia-Try and Travis.

Comment 29

[Kevin Grandon :kgrandon]

(In reply to Zac C (:zac) from comment #28)
> Kevin, if you do can you disable it for mac only ? as linux versions seem
> stable on both Gaia-Try and Travis.

Yes, good point. I assure you I hate disabling tests, and I promise to help spend some time debugging this with you and working on the tests once we know the vertical homescreen has a chance of making 2.0.

Comment 30

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #27)
> I've spent a lot of time investigating this fix as well, my hunch is that it
> could potentially be a platform bug. I have a try run here which applies the
> patch in bug 1026737. There is one error present in the same test, but it
> has a different assertion, but similar screenshot.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=f02fcef6012d
> 
> We need this fixed today. I am going to implement a similar test in
> MarionetteJS to achieve the same coverage. If it works, I'm going to suggest
> that we disable the gaia ui test in favor of the marionetteJS test - while
> we look for a fix for the python test.
> 
> We should not put the entire vertical homescreen at risk because of one
> failing test.

Sorry, but that's not going to work for me. MarionetteJS is not on device, so this is not the same coverage as what's given in gaia ui tests. QA cannot risk downtime on test coverage around the homescreen, as this greatly impacts productivity of our daily testing.

Comment 31

[Kevin Grandon :kgrandon]

(In reply to Jason Smith [:jsmith] from comment #30)
> Sorry, but that's not going to work for me. MarionetteJS is not on device,
> so this is not the same coverage as what's given in gaia ui tests. QA cannot
> risk downtime on test coverage around the homescreen, as this greatly
> impacts productivity of our daily testing.

If we do disable this, we are not going to disable this test on devices, only on the Mac b2g desktop runtime. See comment 28.

Comment 32

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #31)
> (In reply to Jason Smith [:jsmith] from comment #30)
> > Sorry, but that's not going to work for me. MarionetteJS is not on device,
> > so this is not the same coverage as what's given in gaia ui tests. QA cannot
> > risk downtime on test coverage around the homescreen, as this greatly
> > impacts productivity of our daily testing.
> 
> If we do disable this, we are not going to disable this test on devices,
> only on the Mac b2g desktop runtime. See comment 28.

Oh in that case, then that's fine. Just want to make sure we're not losing on device coverage.

Comment 33

[Kevin Grandon :kgrandon]

Landing for now with test disabled for mac. https://github.com/mozilla-b2g/gaia/commit/535c29c42f8d40f0e6bad9bccce0e77a25ec1338

Going to follow-up both with a js marionette test and investigation into the gaia ui test in bug 1027708.

Comment 34

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

I don't see anything in notifyHomescreenApp that would prevent Tutorial.done() being called before the postMessage-s. Might that be what's going on here? (I'm also not really sure why this is in Tutorial - seems completely unrelated and like it should be in FTU AppManager if not FtuLauncher but we can refactor that another time.)

Comment 35

[Kevin Grandon :kgrandon]

2.0:
https://github.com/mozilla-b2g/gaia/commit/e3ca816552cf8f73264b08d3c3fabe5f5efd7413
https://github.com/mozilla-b2g/gaia/commit/83aa02be90e4ae31e602d66bd0457db2918434f1