837671 - [calendar] Implement new startup loading events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect, P2)

Description

[Julien Wajsberg [:julienw]]

We need to measure when the app is usable by the user. For that we'll need to send an event (the moment is specific to the app) to |window| that the performance test will be able to receive.

The event name can be "x-moz-perf-user-ready" amongst all apps so that the performance test can be similar.

For the Calendar, it should be triggered when the user can actually manipulate the calendar (ie any touch events have been registered)

Comment 1

[:Eli Perelman]

Bug 996038 introduces new events outlining the phases of application startup. Each of these 5 events needs to be implemented.

Comment 2

[:Eli Perelman]

As an FYI, this implementation needs to land in 2.0 as it is important for meeting release performance acceptance criteria.

https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance

Comment 3

[Miller Medeiros [:millermedeiros]]

I'll take this since Gareth is already working on other tasks this week. Will try to submit a patch tomorrow for review.

Comment 4

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20948

I'm almost sure this is measuring the right things, but please double check it. it took me a while to realize that app was not really ready after the view was loaded, that it really needed the "pending manager" to complete the sync/db operations. Thanks.

Comment 5

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8445620 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20948

So I think we're definitely on the right track and I am convinced that (1) and (2) are basically correct. I think that the description of (3), (4), and (5) imply that the busytimes need to be available in the month view and month day view and I don't think we have that here [yet].

I think accomplishing that bit may be a bit more complicated (though I still think it's worthwhile of course). When we get the `expandComplete` ping from the RecurringEvents controller, we know that https://github.com/millermedeiros/gaia/blob/master/apps/calendar/js/provider/caldav_pull_events.js#L210 has been called for the busytimes in the initial time range. But between that and where https://github.com/millermedeiros/gaia/blob/master/apps/calendar/js/views/day_based.js#L371 gets invoked, there are a few levels of busytime/time observer event hops. Even after that, the time controller goes to the database to find the busytimes belonging to the slice that the view is subscribed https://github.com/millermedeiros/gaia/blob/master/apps/calendar/js/controllers/time.js#L572. Only then does the dom representation of the busytime actually end up in front of the user https://github.com/millermedeiros/gaia/blob/master/apps/calendar/js/views/day_based.js#L421.

Comment 6

[Gareth Aye [:gaye] (back from PTO)]

So I actually think that this https://github.com/millermedeiros/gaia/blob/master/apps/calendar/js/views/day_based.js#L150 might be a good place to note when its called first that the busytimes have been loaded. Of course we also need to do the same thing for the month view which has diverged a bit. I think that this might be a good place to note that https://github.com/millermedeiros/gaia/blob/master/apps/calendar/js/views/month_child.js#L77.

Comment 7

[:Eli Perelman]

Review granted from the perf standpoint that the whitelisting is good and names are correct, and from an outsider's POV, look like they would be accurate. I will leave the final location implementation up to Gareth's review.

Comment 8

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8445620 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20948

Gareth, any updates?

Comment 9

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8445620 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20948

Aside from a couple nits, this looks great to me. Thanks Miller!

Comment 10

[Miller Medeiros [:millermedeiros]]

landed into master: https://github.com/mozilla-b2g/gaia/commit/d3f97eb1d9197ac071c71e2bf5e3f1097b513412

Comment 11

[:Eli Perelman]

As mentioned in comment 2, this needs to be uplifted to 2.0.

Comment 12

[:Eli Perelman]

Comment on attachment 8445620 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20948

Requesting uplift to 2.0 as it is important for meeting our release performance acceptance criteria.

[Feature/regressing bug #]: bug 996038
[User impact if declined]: none
[Describe test coverage new/current, TBPL]: Feature only triggers events for testing, no user-facing features or tests
[Risks and why]: Low, as there are no user-perceived changes
[String/UUID change made/needed]: n/a

Comment 13

[Gareth Aye [:gaye] (back from PTO)]

(In reply to Eli Perelman, :Eli from comment #12)
> Comment on attachment 8445620 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20948
> 
> Requesting uplift to 2.0 as it is important for meeting our release
> performance acceptance criteria.

Can you describe how exactly this will help you all? I don't think there are any great risks here either, but would like to understand what material difference this makes for the perf team

Comment 14

[:Eli Perelman]

Sure thing. On Datazilla, we are running performance metrics against v2.0 as well as master. We cannot give performance sign-off on an application for v2.0 if the means to test that application in that branch are not available.

https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance

Comment 15

[Gareth Aye [:gaye] (back from PTO)]

Ok makes sense. Thanks Eli!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/814c972d7e588e54c60b89119b8df478ff3b682f