Closed
Bug 398950
Opened 17 years ago
Closed 17 years ago
Investigate contentprefsink event registration
Categories
(Firefox :: Settings UI, defect, P1)
Firefox
Settings UI
Tracking
()
RESOLVED
FIXED
Firefox 3 beta1
People
(Reporter: sayrer, Assigned: myk)
Details
Attachments
(2 files)
18.97 KB,
text/plain
|
Details | |
3.18 KB,
patch
|
sayrer
:
review+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•17 years ago
|
||
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.
Assignee | ||
Comment 2•17 years ago
|
||
Hmm, indeed, it looks like we could save some cycles by integrating ContentPrefSink's functionality into the nsBrowserStatusHandler in browser.js.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 3 M9
Assignee | ||
Comment 3•17 years ago
|
||
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.
Attachment #284401 -
Flags: review?(sayrer)
Reporter | ||
Comment 4•17 years ago
|
||
Comment on attachment 284401 [details] [diff] [review] patch v1: moves WPL event handling to nsBrowserStatusHandler yes, let's try this.
Attachment #284401 -
Flags: review?(sayrer) → review+
Assignee | ||
Comment 5•17 years ago
|
||
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.
Attachment #284401 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #284401 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 6•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•17 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•