Closed Bug 1009111 Opened 8 years ago Closed 8 years ago

Enable vertical homescreen by default


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

Gonk (Firefox OS)


(feature-b2g:2.0, b2g-v2.0 fixed)

2.0 S4 (20june)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed


(Reporter: kgrandon, Assigned: kgrandon)



(Keywords: perf, regression, Whiteboard: [p=3],[systemsfe][c=regression p= s= u=2.0])


(3 files)

No description provided.
Duplicate of this bug: 1007121
Attached file Github pull request
Here is a patch which enables the homescreen by default, but breaks all of the marionette and python tests. Likely some work to be done here in the testing framework.
Hey Zac - 

Any chance someone from your team would be willing to help me fix test failures when enabling the vertical homescreen? We plan on enabling it by default in ~2 weeks or so, so if anyone could work with me over that time it would be appreciated. We're seeing these kinds of errors:

Right now it looks like there is a bunch of homescreen hooks in the testing framework itself. Any chance we could try being more generic to work with either homescreen?
Flags: needinfo?(zcampbell)
Kevin, yes we can help you (just ni? us) but from the look of that test run there are some other problems in app_window.js that are causing most of the problems.
The rest shouldn't be too much trouble.
Flags: needinfo?(zcampbell)
feature-b2g: --- → 2.0
Hey Zac - wondering if you can give me a hand debugging this, I'm not having much luck. I think it's something weird going on with the python runner. 

When I use a profile (generated with the attached pull request) with b2g desktop, everything works fine - and the new homescreen comes up. When I try using it with gaia-ui-tests, I get FTU, but it appears that the homescreen does not come up. 

Can you tell me if you guys are overriding any settings in the profile? Also - is there any specific homescreen logic in the startup path of the ui tests? Thanks!
Flags: needinfo?(zcampbell)
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Suggested to kgrandon in email that he should look at the killAll atom in GaiaApps.js which is probably killing the 'vertical' app and subsequently the test is not waiting for the homescreen to restart.
Flags: needinfo?(zcampbell)
Comment on attachment 8425000 [details] [review]
Github pull request

Vivien, Tim - as we near getting ready to enable the homescreen, I think it would make sense to open up a review to iron out anything before landing this into master. Being the owners of gaia, you two seem like the right choice for a review. If you want me to pass it off to another reviewer, please let me know.

The relevant code for the homescreen is split across many places. You can find the main features of the vertical homescreen in the following places:

/apps/vertical - Vertical homescreen specific code.
/apps/collection - Collection app, responsible for managing collections.
/apps/bookmark - Bookmark app, responsible for managing bookmarks.
/apps/search - Entry point for searching (current code lives in the homescreen app)
/shared/elements/gaia_grid - A simple wrapper for creating a grid like object view. Will be used in the homescreen, search, and collections. (Potential for use in media apps in the future)

Pretty much all code within these directories has landed with an R+, but if we would like to take this opportunity to review all of it again I would be more than happy for the additional quality checks. Thanks and please let me know if you have any questions.
Attachment #8425000 - Flags: review?(timdream)
Attachment #8425000 - Flags: review?(21)
Comment on attachment 8425000 [details] [review]
Github pull request

I don't see any outstanding changes to review, so I will rubber-stamp here.

The one thing I care about is the name "vertical" -- it does not seem to be very descriptive (than "home2") and we really need to get the name right before rolling out to production, so let's spend a little time consider my two candidates here:

- homescreen2
- homescreen-vertical

I would be better if the word starts with home* so people would know this is a home screen.
Attachment #8425000 - Flags: review?(timdream) → review+
Ok, let me think about this. FWIW - we planned on having it appear in settings as simply "Vertical", and that is the app name deemed by UX, so the folder "Vertical" made sense. "homescreen-vertical" seems pretty verbose, but may be an option.

At this point I am leaning towards using "verticalhome", or "vertical_home". (We should not have dashes in folder names, but I'm not sure what happened to emergency-call). I have a bug filed to fix that app, but there are unit test failures I think.
To make the landing less of a shock I'm going to split up the pull request into a few pieces. It will probably go something like:

1 - Move the collection app into apps/collection
2 - Move the home2 app into apps/verticalhome
3 - Update configuration and tests.
I verified the bugs from the test plan. Every current issue is fixed. From the QA side, the feature can be pref'd on.
(In reply to Johan Lorenzo [:jlorenzo] from comment #11)
> I verified the bugs from the test plan. Every current issue is fixed. From
> the QA side, the feature can be pref'd on.

Thanks for that Johan!
Landed the first part to move collection into apps/:
Here we move the vertical homescreen into the apps folder. It is still turned off.
Attachment #8432795 - Attachment description: Part 1 - Move home2 into apps/verticalhome → Part 2 - Move home2 into apps/verticalhome
Landed part 2:

(Vertical homescreen is still turned off)
Blocks: 1016928
Comment on attachment 8425000 [details] [review]
Github pull request

It seems a bit strange to me that some of the tests runs on top of and the others on top of

What is the reasoning behing that ?
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #16)
> Comment on attachment 8425000 [details] [review]
> Github pull request
> It seems a bit strange to me that some of the tests runs on top of
> and the others on top of
> What is the reasoning behing that ?

Some of the tests are *actual* homescreen tests, which we may want to keep running for things like flatfish. Others are due to specific hooks into homescreen for test logic. I think it's generally fine to keep the python tests running for now with the old homescreen, and we can perhaps move to more marionette testing for the new homescreen.
Comment on attachment 8425000 [details] [review]
Github pull request

LGTM then.
Attachment #8425000 - Flags: review?(21) → review+
Closed: 8 years ago
Resolution: --- → FIXED
From bug 1021037, we have a start up regression by enabling the vertical home screen by 70ms. 

Results for Gallery, cold_load_time: median:984, mean:984, std: 51, max:1131, min:898, all:1061,954,949,987,1042,1024,1021,990,960,1008,1131,984,1008,1052,989,982,916,925,968,916,969,960,997,898,928

Results for Gallery, cold_load_time: median:1001, mean:993, std: 65, max:1111, min:857, all:1111,972,998,1019,1003,917,908,966,919,985,1002,1017,901,893,857,1009,1090,1086,1068,1095,997,1035,1001,1006,985

Results for Gallery, cold_load_time: median:991, mean:982, std: 57, max:1104, min:888, all:1041,1007,1016,904,900,1058,1027,967,991,1017,1004,946,904,888,1104,1049,1042,979,986,1008,912,904,937,942,1019
Keywords: perf, regression
OS: Mac OS X → Gonk (Firefox OS)
Priority: -- → P1
Hardware: x86 → ARM
Resolution: FIXED → ---
Whiteboard: [p=3],[systemsfe] → [p=3],[systemsfe][c=regression p= s= u=2.0]
Any thoughts on what we can do here?
Flags: needinfo?(kgrandon)
See Also: → 1021037
I would like to either reopen bug 1021037 to track this, or file a new bug to block this one. I would prefer to not leave this reopened if we're not going to back it out. Closing for now, let me know what you'd prefer to do.

Something that would be good to measure is if app launch really regresses from the user standpoint. To do this we would need to take a timestamp from the icon touch, to the onload event before and after the vertical homescreen.
Closed: 8 years ago8 years ago
Flags: needinfo?(kgrandon)
Resolution: --- → FIXED
Depends on: 1021037
See Also: 1021037
No longer depends on: 1021037
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
You need to log in before you can comment on or make changes to this bug.