If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fix performance hit that we were taking for p3p

VERIFIED FIXED in mozilla1.0

Status

()

Core
Networking: Cookies
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Stephen P. Morse, Assigned: Stephen P. Morse)

Tracking

Trunk
mozilla1.0
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ADT1])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

16 years ago
Bug 129101 implemented some enhanced p3p cookie-management features.  
Unfortunately it also introduced a performance hit on window-open time when p3p 
is turned on.

This bug is to remove that performance hit.
(Assignee)

Comment 1

16 years ago
The way the p3p icon was implemented was that each time a window was opened, a 
test was made to determine if the icon should be visible.  That test was being 
done in the cookie overlay for the status bar.  The status bar does not yet 
exist at the time that the javascript in the overlay is executed, so the test 
needed to be delayed.

The method used to delay the test was to use a setTimeout call.  And it appears 
that that was causing the performance hit randomly -- that is, for the most part 
windows were being open in the usual amount of time, but a few times the time 
was significantly increased (enough so to increase the median time by 15%).

The fix is to not use setTimeout, but rather to add another routine to the 
onload handler of the parent window (navigator.xul already has a handler there).  
Attaching patch that makes this change.

This patch has been tested on the tinderbox comet machine which was the one 
machine on which we were able to see the performance hit.  With this patch, the 
comet Txul numbers were back in the 480 ms range rather than in the 560 range.
Status: NEW → ASSIGNED
Keywords: nsbeta1
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 2

16 years ago
Created attachment 77385 [details] [diff] [review]
remove performance hit by avoiding the use of setTimeout code

Comment 3

16 years ago
Looks good, but it may be cleaner to use DOM Events to do this (instead of
munging the `onload' string:

<html>
 <head>
  <script>
  var listener = {
    handleEvent: function(event) {
      alert("hello!");
    }
  };
  addEventListener("load", listener, false);
  </script>
 </head>
 <body onload="alert('blah!');">
 </body>
</html>
(Assignee)

Comment 4

16 years ago
Created attachment 77389 [details] [diff] [review]
using method described by waterson
Attachment #77385 - Attachment is obsolete: true

Comment 5

16 years ago
Comment on attachment 77389 [details] [diff] [review]
using method described by waterson

sr=waterson. great!
Attachment #77389 - Flags: superreview+

Comment 6

16 years ago
Comment on attachment 77389 [details] [diff] [review]
using method described by waterson

Cool. That should take care of window open.
Attachment #77389 - Flags: review+

Comment 7

16 years ago
Has this been checked in yet?  What's left to do?

Comment 8

16 years ago
Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!
(Assignee)

Comment 9

16 years ago
For starters, the bug is waiting for approval from drivers.

Comment 10

16 years ago
Comment on attachment 77389 [details] [diff] [review]
using method described by waterson

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #77389 - Flags: approval+

Comment 11

16 years ago
Adding ADT1 to Status Whiteboard, nsbeta1+ and adt1.0.0.
Keywords: nsbeta1 → adt1.0.0, nsbeta1+
Whiteboard: [ADT1]

Comment 12

16 years ago
Adding adt1.0.0+ for adt approval to check in.
Keywords: adt1.0.0 → adt1.0.0+
(Assignee)

Comment 13

16 years ago
*** Bug 133624 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 14

16 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 15

16 years ago
verified fix checked in using lxr
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.