1191745 - (new-homescreen) [meta] Switch to new homescreen by default

Status: VERIFIED FIXED

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

Description

[Chris Lord [:cwiiis]]

Meta bug to track switching to the new homescreen.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

Comment 2

[Chris Lord [:cwiiis]]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

This switches over the default homescreen to the new homescreen and fixes all the marionette test failures caused by this.

One test is removed from system that checked the dialog size on the homescreen (this is doesn't apply in the same way anymore and that functionality is adequately tested by homescreen already).

This also incorporates a gaia-app-icon update necessary for app_window_manager_bookmarked_sites_test to pass.

Most, if not all changes here are trivial.

Comment 3

[Chris Lord [:cwiiis]]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

Naoki, could you take a quick look at this and let me know if there are any issues QA side, maybe with tests outside of integration/unit tests?

I don't really know anything about what other testing frameworks we use, I'm guessing this may cause issues elsewhere.

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

Playing around with launch times the application felt quite slow to get to a visual readiness state.

Eli - I was looking for datapoints for launch times regarding the verticalhome and homescreen apps, do you have cycles to help with this? I looked on raptor.mozilla.org and couldn't find apps labeled verticalhome or homescreen.

Comment 5

[:Eli Perelman]

Sure! First, verticalhome results are from the reboot test, not coldlaunch, hence why it isn't listed with the other apps. I could probably make a change for that, but for now, here is the data you seek:

https://raptor.mozilla.org/dashboard/script/measures?var-device=flame-kk&var-memory=319&var-branch=master&var-test=reboot

As far as testing out the new homescreen, and making sure we can properly test existing apps with the new homescreen using Raptor, this should help (based on a new homescreen on the device with the origin homescreen.gaiamobile.org):

Run the restart-b2g test against the new homescreen:
raptor test restart-b2g --homescreen homescreen

To test an existing app works against the new homescreen:
raptor test coldlaunch --app clock --homescreen homescreen

If you have trouble with any of these commands, lemme know and I can get a fresh build on my device tomorrow and try it out (provided I get tips on enabling the new homescreen).

Comment 6

[Chris Lord [:cwiiis]]

Running raptor on a Z3C, new homescreen takes ~2 seconds longer to reach visually loaded state. About the same time to reach fully-loaded state. I've not tried too hard to optimise startup performance, there are certainly quite a few things we can do to reduce the cost of layout on startup.

Kevin, do you think this should block switching over? I'd really like to get some more testing and fix this post-switch, but I can reshuffle priorities and start tackling it now if you think this is a blocker.

Comment 7

[Gregor Wagner [:gwagner]]

Lets switch to the new homescreen and optimize the performance in the next few weeks.

Comment 8

[Chris Lord [:cwiiis]]

I've filed bug 1210737 to track startup performance. My initial comment #6 is incorrect, visuallyLoaded is basically the same, it's fullyLoaded that takes longer to reach.

Comment 9

[Kevin Grandon :kgrandon]

One of the requirements for the verticalhome was that we stress tested it with a tool which installed hundreds of apps. I need to locate and perform tests with that tool.

Using a local indexedDB used to be a requirement in order to have acceptable load times with many apps installed. I'm not sure of the current state of mozapps, but we need to stress test it. I'll try to find the tool and take a look at this.

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Kevin Grandon :kgrandon from comment #9)
> One of the requirements for the verticalhome was that we stress tested it
> with a tool which installed hundreds of apps. I need to locate and perform
> tests with that tool.
> 
> Using a local indexedDB used to be a requirement in order to have acceptable
> load times with many apps installed. I'm not sure of the current state of
> mozapps, but we need to stress test it. I'll try to find the tool and take a
> look at this.

Sounds good - I'm looking at improving our startup performance now, I think this particular test will probably kill new homescreen perf-wise, but we can easily make it better.

Hundreds does seem overkill to me (I don't even have hundreds of apps on my Android devices...), but it can't hurt to know.

Comment 11

[Kevin Grandon :kgrandon]

(In reply to Chris Lord [:cwiiis] from comment #10)
> Hundreds does seem overkill to me (I don't even have hundreds of apps on my
> Android devices...), but it can't hurt to know.

I agree, but it was a partner certification blocker. I'm not sure if that's still relevant or not, but I'll try to dig up some bugs.

Comment 12

[Kevin Grandon :kgrandon]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

Unfortunately I don't think I can review this while bug 1211266 exists - I don't think we can enable it with missing icons and I'm not sure if it's masking performance issues. Chris - can you either look at that issue or perhaps change it to use the certified APIs?

Comment 13

[Chris Lord [:cwiiis]]

Kevin, can you let me know if there are any issues besides bug 1211266 so that I can make sure this is ready when that gets fixed?

Comment 14

[Kevin Grandon :kgrandon]

(In reply to Chris Lord [:cwiiis] from comment #13)
> Kevin, can you let me know if there are any issues besides bug 1211266 so
> that I can make sure this is ready when that gets fixed?

I still want to run through everything functionality wise again, and I would like to do some additional perf testing once bug 1211266 has landed. Leaving NI.

Comment 15

[:Eli Perelman]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

Handled in comment 5.

Comment 16

[Chris Lord [:cwiiis]]

Kevin, any update? We'd really like to make the switch so we can get some better testing done. Going on updates in bug 1211266, it sounds like the bug affects all homescreens, not just this one, and it doesn't sound like the fall-out (very occasional missing app icon images only after flashing) is so terrible that we need to block this on it. It should certainly be a release blocker though, no question about that.

The longer we leave to switch over, the less time we're going to have to address issues that come up.

Comment 17

[Kevin Grandon :kgrandon]

(In reply to Chris Lord [:cwiiis] from comment #16)
> Kevin, any update? We'd really like to make the switch so we can get some
> better testing done. Going on updates in bug 1211266, it sounds like the bug
> affects all homescreens, not just this one

I was never able to reproduce bug 1211266 on the current home screen, so I think it's only with privileged APIs. I'm not willing to review something with a regression like that - maybe someone else would though? If you can prove that it also happens on verticalhome I could change my mind.

One option could be to use certified APIs until that bug is fixed, then switch back to privileged APIs.

Comment 18

[:gerard-majax]

Kevin, what makes you thinking that the problem is because of privileged API ? So far my finding points toward something bad happening with the Blob we are sending back with |getIcon()|, and it might be cause of process separation because the error happens in codepath involving sharing blob in the same process. However that is crashing on the Parent and obviously the homescreen's Child is not the same process.

But if you have more infos to share I'd be happy to hear them :)

Comment 19

[:gerard-majax]

Oh and all the STRs I could attempt were mostly pointing toward some kind of bad race condition: i.e., on a too slow device, no problem.

Comment 20

[Chris Lord [:cwiiis]]

I spoke to janv on IRC, he says that if things go well, he should get to bug 1211266 next week. Either way, that bug is a blocker for release because replaceable homescreens is one of our core features. I don't think it makes sense to block on this before switching over, given it doesn't make applications inaccessible (just bad-looking, in reasonably rare circumstance).

Changing to certified is unlikely to fix this going on Alexandre's comments, verticalhome probably escapes this by not using the getIcon utility function (and instead using system XHR to fetch the icon directly).

Comment 21

[Kevin Grandon :kgrandon]

(In reply to Alexandre LISSY :gerard-majax from comment #18)
> Kevin, what makes you thinking that the problem is because of privileged API?

As Chris mentioned in comment 20 this is only apparent with the getIcon method, which previous certified home screens do not use. I personally would like to see this fixed before introducing known visual regressions.

Comment 22

[Chris Lord [:cwiiis]]

Bug 1211266 is extremely rare with this patch applied - I can more easily reproduce it when switching homescreens, as opposed to when the homescreen is the default. Does this tally at all with your experience Kevin?

In about a dozen reset-gaia's, I reproduced it once with two icons in the dev apps (I couldn't reproduce it with a production build with new homescreen as default).

Comment 23

[Kevin Grandon :kgrandon]

It's probably more pronounced on a flame or slower device. I've seen this with up to four missing icons, which looks pretty bad: https://bug1042807.bmoattachments.org/attachment.cgi?id=8669431

My recommendation is that you either get the platform bug fixed first, or temporarily switch the code to use systemXHR instead of getIcon(). Once the platform code is fixed you could switch back to a privileged app.

Comment 24

[Chris Lord [:cwiiis]]

Kevin, I've coded a work-around that works consistently for me (I've put in far more conservative consts than there need to be): https://github.com/gaia-components/gaia-site-icon/pull/12 - is this ok with you, in lieu of the platform bug being fixed? (which should also happen soon, after which we can back this change out)

Comment 25

[Chris Lord [:cwiiis]]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

Re-adding r? with patch rebased on master with the work-around applied (which is ready to land, pending tests).

Comment 26

[Chris Lord [:cwiiis]]

The work-around is merged now, so the pull request is a single commit again.

Comment 27

[Kevin Grandon :kgrandon]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

I'm sorry, but I am going to clear the review request because I don't think that this is an approach that we should be doing in 2.5. I'm not happy trading one heavily used feature (app grouping) for another (pin the web). If there was some way to continue to get to verticalhome and it's functionality, bookmarks included, then I would probably feel differently.

I don't necessarily want to block people, or have people think I am the downfall of an entire project, so here are some suggested options to move forward:

- Since you are the home screen owner, you can delegate the patch to another non-peer to review (Ben, or anyone on the Pin the Web project). This is how we grow peers after all, and we've discussed this possibility on the mailing list before.

- If you are looking for a symbolic approval, consider flagging Tim and Vivien as owners of gaia, or Fabrice as the chief architect, for a review.

If you take either of these approaches please remove my name from the r= in the commit message.

Comment 28

[Wilfred Mathanaraj [:WDM]]

we decided at UX and Product level that we will be ok for 2.5 not to have app grouping. We will be reviewing the app grouping feature as part of the UX evolution and how to come up with a design that would work better for the new UI.

there will be re-definition of appgrouping in future release.

Comment 29

[Chris Lord [:cwiiis]]

I'm uncertain as to the next course of action at this point. The least risk if we want to ship pin-the-web is to carry on with what we were doing and I think this would be the best choice long-term.

Some alternatives that are all a mix of riskier/poorer UX, in order from least to most risky:
1. Ship the homescreen as a marketplace app (see bug 1196992), with the intention of replacing app-grouping style functionality asap.

2. Remove pinned pages, retain pinned sites (basically replacing pre-existing bookmark functionality with very little difference), ship verticalhome.

3. Ship both home-screens, allow the user to enable pin-the-web, toggle homescreens when enabling/disabling pin-the-web.

4. Ship verticalhome, put pinned pages in the empty rocketbar search page
 - Requires porting of the pinned pages bits from homescreen into search (the code is quite modular, so may not be *too* hard - I'd be most worried about how long it'd take to get good test coverage and the lack of anyone having tried it)

5. Ship verticalhome, port pinned pages into verticalhome
 - There were good reasons we didn't do this in the first place. gaia-grid is very inflexible, and the current searchbar arrangement is hard to change. I'm still not confident we can do this well, and especially so if we only have a week to do it and get it tested.

6. Play banjo


I'm providing alternatives because I think that's what I should do, but I must also be absolutely clear that picking any of them (except possibly the first one) is more work to something that was already at risk.

I'd also like to state that I'm no big fan of losing app grouping, but we had long meetings about this where we set out what was physically possible in the time, and agreed on this course of action. This isn't really the right time to be changing our minds about things.

Comment 30

[Chris Lord [:cwiiis]]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

I'm flagging a few people that I think would make suitable reviewers for this. Please see the conversation above - remember we can always back this out, but we need an r+ before we can continue.

What this patch does:
- Switch default homescreen from 'verticalhome' to 'homescreen'
- Rename 'New Home Screen' to 'Default Home Screen'
- Rename 'Default Home Screen' to 'Legacy Home Screen'
- Amend test-system tests to use homescreen instead of verticalhome where appropriate
- Fix system integration tests to use homescreen's convenience library instead of verticalhome's

What this patch doesn't do:
- Remove verticalhome
- Disable installation of verticalhome
- Disable switching to verticalhome
- Migrate users from verticalhome to homescreen (see bug 1202422)

In short, this patch will make newly flashed/reset builds use the new home screen, but existing builds will continue to use verticalhome until migration patches land (which will probably be in pretty short order after this lands) or they manually switch.

This patch mostly needs someone to do a basic sanity check on the configuration and test changes, and preferably to also give it a run-through on a device to make sure nothing is seriously broken (via both install-gaia over an existing build and reset-gaia).

Comment 31

[Guillaume Marty [:gmarty]]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

I left a comment on Github but it looks good to me!

Comment 32

[Théo Chevalier]

I left a comment about localizing the App name in manifest.webapp
 (this should fix bug 1215706)

Comment 33

[Chris Lord [:cwiiis]]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

Sorry for the bug spam, other reviewers - and thanks Guillaume. I've made the changes you and Theo suggested and, pending a full run of green tests that isn't obviously broken, will merge... Fingers crossed!

Advanced apologies to sheriffs, I'm only intermittently around this weekend, but as best I can I'll try to keep an eye on it. If we get catastrophic failures, we should back out immediately - small failures (e.g. intermittent integration test failures), I'd prefer a bit of leniency to fix if possible.

Cheers to everyone that's provided feedback - I don't think anyone is 100% happy, so let's knock this one out and get to improving it for 2.x/3.0 :)

Comment 34

[Chris Lord [:cwiiis]]

Well, everything's green, merged: https://github.com/mozilla-b2g/gaia/commit/2032dcac9f6f98617fc3ce10bf0256c4e2822418

Fingers crossed...

Comment 36

[Ben Francis [:benfrancis]]

Thank you everyone.

Be assured that figuring out a new solution for organising pinned content on the homescreen is our top priority for 2.6.

Flagging QA to get this on their radar so we can test for regressions and make sure the migration works smoothly.

Comment 37

[Nigel Babu [:nigelb]]

I've backed out bug 1202422 and bug 1191745 on causing the fairly disastrous Gij17 bustage. Could you please look into it?

Comment 38

[Chris Lord [:cwiiis]]

Re-landed: https://github.com/mozilla-b2g/gaia/commit/497143e60a6eb1f4c266b84d244a788cd1a93806

Comment 39

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note: A new FOTA build is on the way to update people completely on the gonk layer for the migration to be complete.

Comment 40

[Chris Lord [:cwiiis]]

Comment on attachment 8666724 [details] [review]
[gaia] Cwiiis:bug1191745-new-homescreen-by-default > mozilla-b2g:master

This has long since been checked in, removing f?

Comment 41

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Ya, it's been long since checked in.  We've filed bugs here and there in regards to it, those bugs have been looked at; clearing NI on me.

Comment 42

[Chris Lord [:cwiiis]]

This bug is already closed, it solves little to add polish blockers to it.

Comment 43

[Chris Lord [:cwiiis]]

A note to people that find this bug (hey Ed :)) - we aren't hanging new bugs off of this bug anymore, the new homescreen has been the default for a long time and we aren't going to back it out.

Any issues you find now, feel free to individually nominate as blockers to 2.5 or 2.6 (and note that I track all homescreen bugs, so I'll be aware of anything that gets filed).