Closed Bug 998163 Opened 7 years ago Closed 7 years ago
8% Tp5 Optimized regression on inbound for most all platforms
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/p-yr923nK3U I originally thought it was bug 991210 but backing it out didn't help. Turns out it was bug 991111.
Added p=3 s=it-31c-30a-29b.3
Assignee: nobody → edilee
Delay the click event listener adding to on mouseover. Some reason adding this window level click listener is causing the tp5o regression... ?
jmaher, any idea why the addition of the window-level "click" event listener (9x) in a preloaded hidden document would cause tp5o to regress 8%? Or who would know how to understand this mysteriousness?
Attachment #8408726 - Attachment description: sponsored tiles mock v1 → v1
Adds a page level click listener once. Waiting on talos results https://tbpl.mozilla.org/?tree=Try&rev=3b0b5701f9b6
For those following along, the patch removes the code that used to add a top-level click listener for each Site created and instead adds the click listener once. This fixes the performance regression because before when the click listener was added to the node, the listener and references would be cleaned up when the Site's node was removed.
Ed, this is a regression in the memory (main rss / private bytes) as well as responsiveness. For this to be regressing, I would assume the click listener has a memory hit, if we load 49 pages 25 times each, that could get expensive when talking about a few hundred bytes each.
Comment on attachment 8408740 [details] [diff] [review] v2 Review of attachment 8408740 [details] [diff] [review]: ----------------------------------------------------------------- Glad it worked! r+ with comments below. ::: browser/base/content/newtab/page.js @@ +19,5 @@ > // Listen for 'unload' to unregister this page. > addEventListener("unload", this, false); > > + // XXX bug 991111 - Not all click events are correctly triggered when > + // listening from the xhtml node, so listen from the xul window and filter Please mention that this is needed for middle clicks on tiles specifically. @@ +194,5 @@ > if (this._mutationObserver) > this._mutationObserver.disconnect(); > gAllPages.unregister(this); > break; > case "click": Whereas the click listener added to the sites' nodes only captured event.button == 0 clicks, this one captures all button clicks. (Try and see.) I don't know why that is. So this needs to check button == 0 for the newtab-toggle and button == 0 or 1 for sites. Then, in Site_onClick, the only case we want to observe middle clicks for is for calling _recordSiteClicked, so the other cases there need to ignore button == 1.
Attachment #8408740 - Flags: review?(adw) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.