1015390 - [SMS] Implement new startup loading events

Status: RESOLVED FIXED

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

Description

[:Eli Perelman]

+++ This bug was initially created as a clone of Bug #837668 +++

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 events for implementation are outlined in bug 996038.

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

[Julien Wajsberg [:julienw]]

I don't see where you see this on this wiki page.

Comment 4

[Julien Wajsberg [:julienw]]

Note that Bug 837666 already introduced some events. This bug should both rename them and add the new ones.

Comment 5

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Note that Bug 837666 already introduced some events. This bug should both
> rename them and add the new ones.

Since we already have PerformanceTestingHelper to dispatch the event for profiling, is there any reason that we don't dispatch these dom-loaded/interactive/... events directly, but add the listener in helper then dispatch instead?

Comment 6

[:Eli Perelman]

The reason is that there are plans in the future to do more with these events than just performance testing, but possible indicators to the platform for other optimizations, etc. Not to mention that the helper script is used by several other applications and cuts down on the amount of duplicate code.

Comment 7

[Julien Wajsberg [:julienw]]

Steve, well, since that's how all other apps are doing now, let's do the same :)

For example, I think the integration tests might use some of these events to know when the app is ready. Right now they can do it but they need to set mozPerfHasListener to true before and soon enough. This looked like a good idea back when Rik and I designed the API (don't do work that we don't need), but maybe it was too much complexity for a very small arguable perf win.

Comment 8

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, I replace the events with new one, but I still keep the performanceTestHelper for profiling other events we want.

Hi Eli, hope that I didn't misunderstand your comments, and thanks for the reply.

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8457819 [details] [review]
Link to github

r=me with the changes I requested; if you don't agree with this let's discuss :)

Comment 10

[:Eli Perelman]

Comment on attachment 8457819 [details] [review]
Link to github

The only thing missing from the patch is a whilelist entry in the array at [1]. Add "sms" to that array and everything else looks good to go.

[1] https://github.com/mozilla-b2g/gaia/blob/master/tests/performance/startup_events_test.js#L26

Comment 11

[:Eli Perelman]

Comment on attachment 8457819 [details] [review]
Link to github

f- until the changes in comment 10 are made.

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8457819 [details] [review]
Link to github

Thanks for the feedback, I already applied them to the patch. Just a question about the whitelist: Do we still need the whitelistedUnifiedApps for distinct the type of lastEvent?

Comment 13

[:Eli Perelman]

Comment on attachment 8457819 [details] [review]
Link to github

This patch looks good. r+

In regards to the whitelistedUnifiedApps, that array will be going away once all apps have implemented the new events.

Comment 14

[Steve Chung [:steveck]]

In master: f6bb7a99803f040b811fadd5f49d0990d7eccbbf

Comment 15

[:Eli Perelman]

Comment on attachment 8457819 [details] [review]
Link to github

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 16

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/96e8dc653b78e7cfa8ab1d473bb3f50a5144df8d