Add an even-more-delayed startup event

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: snorp, Assigned: jchen)

Tracking

(Blocks: 1 bug)

34 Branch
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Currently we have the browser-delayed-startup-finished event for doing "delayed" initialization. This event is fired in response to DOMContentLoaded, though, which is still pretty early in the page load. We should add a new even later event to do some of the things that get done here (probably connected to the document 'load' event).
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #0)
> Currently we have the browser-delayed-startup-finished event for doing
> "delayed" initialization.

If this is the delayed-startup event used in testing (and afaik only in testing), is there any reason we shouldn't just move that event to fire at this later point, rather than adding another event?

Note that we expect the browser to open about:home on startup (on all tests?) which may not fire the same events as a regular page-load.
Flags: needinfo?(snorp)
As Jim said, we do several things there where the event is emitted -- but not actually in response to the browser-delayed-startup-finished event. I guess we wouldn't necessarily have to add a new 'more delayed' event, but we do need to init some of those things later.
Flags: needinfo?(snorp)
I wanted that event to fire after the first page was shown to the user. So if there's a good event after DOMContentLoaded I think we should just use it.
(In reply to Wesley Johnston (:wesj) from comment #4)
> I wanted that event to fire after the first page was shown to the user. So
> if there's a good event after DOMContentLoaded I think we should just use it.

I think just 'load' would be an improvement at least. DOMContentLoaded is, I believe, fired after body parsing is complete.
'pageshow' might fire a little after 'load'

I would be OK with adding a "browser-initial-pageload-finished" notification. We also support a "sessionstore-windows-restored", like desktop. I have never tested as to when it fires wrt other notifications:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#157
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#1224
(Assignee)

Comment 7

4 years ago
Created attachment 8614263 [details] [diff] [review]
Add DelayedInit module for Fennec startup (v1)

This patch adds a DelayedInit module for performing delayed initializations. There are more descriptions inside DelayedInit.jsm
Attachment #8614263 - Flags: review?(mark.finkle)
(Assignee)

Comment 8

4 years ago
Created attachment 8614265 [details] [diff] [review]
Convert delayed startup to using DelayedInit (v1)

This patch converts the current delayed startup block to using DelayedInit. Instead of having a big block of code that runs on DOMContentLoaded, we will have a series of initializers that are run more granularly and will run only when the message loop is idle.
Comment on attachment 8614265 [details] [diff] [review]
Convert delayed startup to using DelayedInit (v1)

Have you pushed this to Try for autophone? I am wondering if some of these changes are too aggressive. The GMPInstallManager, for example. It could get inited too early. We wait 60 seconds currently.
Comment on attachment 8614263 [details] [diff] [review]
Add DelayedInit module for Fennec startup (v1)

This looks good. Let's see what might need tweaks as we start to use it in the app.
Attachment #8614263 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 8616066 [details] [diff] [review]
Convert delayed startup to using DelayedInit (v2)

I made several autophone runs but the results were a bit inconclusive -- some numbers were down but some others were up.

So while I investigate the results more, I made this patch a lot more conservative -- this new patch only converts the existing delayed startup tasks to using DelayedInit. It should still give us some wins. I moved the delayed startup block to the bottom of the function to make it more clear that it will run later.

As for the GMP installer, I think we can get rid of the minute-long wait, because DelayedInit will only run it on idle anyways, so the impact is small.
Attachment #8614265 - Attachment is obsolete: true
Attachment #8616066 - Flags: review?(mark.finkle)
Comment on attachment 8616066 [details] [diff] [review]
Convert delayed startup to using DelayedInit (v2)

Looks safe enough to try out. Nice work.
Attachment #8616066 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

4 years ago
Blocks: 807322
https://hg.mozilla.org/mozilla-central/rev/f2535394c205
https://hg.mozilla.org/mozilla-central/rev/f84de1ca1d11
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(Assignee)

Updated

4 years ago
Blocks: 1173379

Comment 15

4 years ago
FYI, win on throbber stop especially noticeable in twitter and nytimes.

http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstop/local-twitter/norejected/2015-06-09/2015-06-09/notcached/errorbars/standarderror/notry

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=acd94173f7e1&tochange=f84de1ca1d11

either due to Bug 1166452 - Add an even-more-delayed startup event or Bug 1166309 - java.lang.UnsatisfiedLinkError: nativeAllocateDirectBuffer when using standalone GeckoView
You need to log in before you can comment on or make changes to this bug.