Closed Bug 1015390 Opened 5 years ago Closed 5 years ago

[SMS] Implement new startup loading events

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S1 (1aug)
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: Eli, Assigned: steveck)

References

Details

(Keywords: perf, Whiteboard: [c=automation p= s=2014.08.01.t u=][sms-sprint-2.0S6])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
julienw
: review+
Details | Review
+++ 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.
Bug 996038 introduces new events outlining the phases of application startup. Each of these 5 events needs to be implemented.
Summary: [SMS] "ready to use" perf measurement → [SMS] Implement new startup loading events
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
I don't see where you see this on this wiki page.
Note that Bug 837666 already introduced some events. This bug should both rename them and add the new ones.
(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?
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
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.
Attached file 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.
Attachment #8457819 - Flags: review?(felash)
Attachment #8457819 - Flags: feedback?(eperelman)
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 :)
Attachment #8457819 - Flags: review?(felash) → review+
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
Status: NEW → ASSIGNED
Comment on attachment 8457819 [details] [review]
Link to github

f- until the changes in comment 10 are made.
Attachment #8457819 - Flags: feedback?(eperelman) → feedback-
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?
Attachment #8457819 - Flags: feedback- → feedback?
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.
Attachment #8457819 - Flags: feedback? → feedback+
In master: f6bb7a99803f040b811fadd5f49d0990d7eccbbf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8457819 - Flags: approval-gaia-v2.0?
Assignee: nobody → schung
Whiteboard: [c=automation p= s= u=] → [c=automation p= s=2014.08.01.t u=]
Whiteboard: [c=automation p= s=2014.08.01.t u=] → [c=automation p= s=2014.08.01.t u=][not-part-of-initial-sprint]
No longer blocks: sms-sprint-2.0S6
Whiteboard: [c=automation p= s=2014.08.01.t u=][not-part-of-initial-sprint] → [c=automation p= s=2014.08.01.t u=][sms-sprint-2.0S6]
Attachment #8457819 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
You need to log in before you can comment on or make changes to this bug.