889154 - Move the addition of the MozApplicationManifest event listener to delayedStartup

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

I wrote a basic high performance profiler that uses window.performance.now() to measure the time of browser.js' onLoad which runs before the first window is painted.

I noticed that there is a slow spot in the code where we run document.getElementById for the social-sidebar-browser. Moving this block of code to delayedStartup speeds up onLoad by 30.8% (from 5.888ms to 4.069ms).

The only code that should be in onLoad should be code that is *required* to be run before the first window is painted, and I don't see why these event listeners need that priority.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 769940 [details] [diff] [review]
Patch

I'm fine with the move of the social listener, but if it is only the getElementById causing the slowdown, I would suggest leaving the gBrowser listener where it is.  There is a longer history of that working where it is.  definite r+ moving only social, otherwise some explanation why both are necessary to move.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

In theory MozApplicationManifest events can fire before delayedStartup, right?

I don't suppose we support onMozApplicationManifest attributes?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

An MXR search for MozApplicationManifest doesn't show any mentions of onMozApplicationManifest. Just code for adding and removing of event listeners using addEventListener/removeEventListener. If we supported it, I would have expected to find a GK_ATOM for onmozapplicationmanifest.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Ah, but this does dispatch if there is a "manifest" attribute on the document, http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#4667, during DispatchContentLoadedEvents.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

So in turn, moving the event listener to _delayedStartup would introduce a bug if the user's home page was set to a document that included a manifest attribute on the document.

But...

Since bug 756313 we have stopped loading the home page until _delayedStartup. I'll move this event listener code to right before we start loading the home page.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb1667940542

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb1667940542