398950 - Investigate contentprefsink event registration

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Robert Sayre]

It looks like the ContentPrefSink might be registering for DOM and WPL events that browser.js already listens for, and many of the WPL implementations are empty. It might be better to make the CPS get events from browser.js.

Comment 1

[Robert Sayre]

Attachment: 30 loads of Google Italia

This shows the JS functions executing during 30 loads of the Italian Google page. There are no scrollbars or complicated content in the page. It looks like the ContentPrefSink might be doubling our event handling XPConnect traffic. This probably won't be significant on a complicated page, but I think profile shows a significant increase in overhead for a simple page.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Hmm, indeed, it looks like we could save some cycles by integrating ContentPrefSink's functionality into the nsBrowserStatusHandler in browser.js.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: moves WPL event handling to nsBrowserStatusHandler

Here's a patch that moves the WPL event handling over to nsBrowserStatusHandler.  I don't move the DOM event handling over, as nothing in browser.js listens for that DOM event.

I still don't have a working dtrace-enabled build, so I haven't tested this for performance impact.

Comment 4

[Robert Sayre]

Comment on attachment 284401 [details] [diff] [review]
patch v1: moves WPL event handling to nsBrowserStatusHandler

yes, let's try this.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 284401 [details] [diff] [review]
patch v1: moves WPL event handling to nsBrowserStatusHandler

Requesting approval on this low-risk patch that simply moves location change listening from ContentPrefSink to nsBrowserStatusHandler for a possible perf win.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Checked in at approximately 11:35pm PDT 2007-10-10.

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.866; previous revision: 1.865
done
Checking in browser/base/content/browser-contentPrefSink.js;
/cvsroot/mozilla/browser/base/content/browser-contentPrefSink.js,v  <--  browser-contentPrefSink.js
new revision: 1.4; previous revision: 1.3
done

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Note: preliminary results showed some improvement, although it was within the margin of error, but an unrelated regression shortly after I checked this in makes it impossible to see if there was sustained improvement.