1015412 - [FMRadio] Implement new startup loading events

Status: RESOLVED FIXED

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

Description

[:Eli Perelman]

Bug 996038 introduces new events outlining the phases of application startup which we use for determining the performance of the application. Each of these 5 events needs to be implemented so we can start gathering metrics.

Comment 1

[Hema Koka [:hema]]

Adding to sprint4 for discussion during planning

Comment 2

[Justin D'Arcangelo [:justindarc]]

Eli: I'm starting to go through the FM Radio app to add these new events. However, I noticed that the PerformanceTestingHelper was already being used to emit a different set of events:

- start
- fm-radio-enabled
- startup-path-done

Should I remove these, or are they still in use?

Comment 3

[:Eli Perelman]

I can't speak for the intentions of the app owners at the time when those are implemented, but if you believe their actions would be duplicated by these new events then I would recommend that they be removed. The 'fm-radio-enabled' looks like it would be specific to the app, so I would leave that one. I'm not really sure what 'start' did, and many apps will probably be replacing 'startup-path-done' with 'moz-content-interactive' or 'moz-app-loaded'.

Also, with the new events, make sure you are triggering them using `window.dispatchEvent` and not using the PerformanceTestingHelper directly. It's ok to use the PerformanceTestingHelper for the custom events like 'fm-radio-enabled', but we want to keep these platform-standard startup events generically triggered.

Comment 4

[:Eli Perelman]

Also, feel free to include me for feedback or review when you're ready.

Comment 5

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Eli: Let me know if this looks like I'm emitting these events from the right places. Unfortunately, FM Radio is a bit of a small app so there's quite a bit of overlap with some of the events (namely: moz-chrome-interactive, moz-app-visually-complete and moz-app-content-interactive).

One thing I wasn't sure about was with 'moz-chrome-dom-loaded' -- I'm firing it with the window.onload event (I think that's correct in the case of FM Radio), but I wasn't sure if it needed to even wait for window.onload or if it could just be triggered at the bottom of fm.js by itself.

Also, the very last thing the app has to wait on is the loading of the favorites list. So, in that case, I wait until that is complete before triggering 'moz-app-complete'. Anyhow, let me know. If I'm on the right track with understanding the placement of these events, I can go ahead and get started integrating them within the Camera app.

Comment 6

[:Eli Perelman]

(In reply to Justin D'Arcangelo [:justindarc] from comment #5)
> Eli: Let me know if this looks like I'm emitting these events from the right
> places. Unfortunately, FM Radio is a bit of a small app so there's quite a
> bit of overlap with some of the events (namely: moz-chrome-interactive,
> moz-app-visually-complete and moz-app-content-interactive).
>
> One thing I wasn't sure about was with 'moz-chrome-dom-loaded' -- I'm firing
> it with the window.onload event (I think that's correct in the case of FM
> Radio), but I wasn't sure if it needed to even wait for window.onload or if
> it could just be triggered at the bottom of fm.js by itself.

If the markup for the chrome or the app content exists in the DOM at time of launch, e.g. in the index.html file, then as long as there is no localizing that needs to be done, these can usually be triggered the first chance you get in JS, no need to wait for window load.

Also if the chrome and app content exists in the DOM at time of launch, you'll typically see these events fired one right after the other.
 
> Also, the very last thing the app has to wait on is the loading of the
> favorites list. So, in that case, I wait until that is complete before
> triggering 'moz-app-complete'. Anyhow, let me know. If I'm on the right
> track with understanding the placement of these events, I can go ahead and
> get started integrating them within the Camera app.

I mistakenly didn't update the meta bug with the new name, so I take the blame for not letting you know that 'moz-app-complete' should be 'moz-app-loaded'. And it sounds like you are on the right track, this event shouldn't be fired until all startup loading is done and the app should theoretically should be in a state of stability.

Comment 7

[Justin D'Arcangelo [:justindarc]]

(In reply to Eli Perelman, :Eli from comment #6)
> If the markup for the chrome or the app content exists in the DOM at time of
> launch, e.g. in the index.html file, then as long as there is no localizing
> that needs to be done, these can usually be triggered the first chance you
> get in JS, no need to wait for window load.
> 

Ok, sounds good. I'll update the PR to move that event outside of the window.onload handler.

Comment 8

[Justin D'Arcangelo [:justindarc]]

Eli: I've updated the PR with corrected event names and updated the `whitelistedUnifiedApps` array as you requested. It should be ready for your final review when you're ready. Thanks!

Comment 9

[:Eli Perelman]

Comment on attachment 8436102 [details] [review]
pull-request (master)

Looks good on the performance front.

Comment 10

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8436102 [details] [review]
pull-request (master)

Looks good to me.

Comment 11

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/04f6d08b66d28849b8e0920b69633ad656c58a8d

Comment 12

[:Eli Perelman]

Justin, this implementation needs to land in 2.0 as it is important for meeting our release performance acceptance criteria [1]. Can you get this uplifted to 2.0?

[1] https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance

Comment 13

[Justin D'Arcangelo [:justindarc]]

(In reply to Eli Perelman, :Eli, PTO thru Jun-22 from comment #12)
> Justin, this implementation needs to land in 2.0 as it is important for
> meeting our release performance acceptance criteria [1]. Can you get this
> uplifted to 2.0?
> 
> [1] https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance

Setting NI? for Hema for 2.0 approval.

Comment 14

[Hema Koka [:hema]]

Comment on attachment 8436102 [details] [review]
pull-request (master)



Based on justification in comment 12, approving for 2.0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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