Closed
Bug 725748
Opened 13 years ago
Closed 13 years ago
Measure MozAfterPaint times in capture phase
Categories
(Testing :: Talos, defect)
Testing
Talos
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ttaubert, Assigned: jmaher)
References
Details
Attachments
(1 file)
1.52 KB,
patch
|
ttaubert
:
review+
k0scist
:
review+
|
Details | Diff | Splinter Review |
Bug 715402 regresses MozAfterPaint measurements but only because delayedStartup() blocks subsequent event handlers (not because MozAfterPaint is fired later).
talos/startup_test/tspaint_test.html
talos/startup_test/twinopen/tpaint-window.html
These tests should register MozAfterPaint event handlers for the capturing phase so that other event handlers are executed afterwards.
Assignee | ||
Comment 1•13 years ago
|
||
is there an example of how to register a MozAfterPaint event handler? We do this in the pageloader code as well, so I want to make sure I do the right thing here.
Comment 2•13 years ago
|
||
Just a matter of passing "true" instead of "false" as the third parameter to addEventListener.
Assignee | ||
Comment 3•13 years ago
|
||
So this is a really simple patch, but it doesn't account for all the pageloader data. In addition, this will probably change the existing numbers that we are collecting which means we need to do a whole round of side by side staging. We just did some side by side staging and haven't even changed the values on tbpl yet, so a lot more churn and confusion for everybody.
Do we want this same stuff for the pageloader tests (tdhtml, tp5, tsspider)?
Do we care about any of the old numbers? Can we just accept a shift in the numbers?
Comment 4•13 years ago
|
||
In theory that shouldn't have much impact on the numbers, because there currently should be no other MozAfterPaint handlers involved, so the only difference in workload is a little bit of extra time in event dispatch code (should be negligible).
Comment 5•13 years ago
|
||
Try run for 48215a782c33 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=48215a782c33
Results (out of 55 total builds):
success: 55
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-48215a782c33
Reporter | ||
Comment 6•13 years ago
|
||
Comment on attachment 597266 [details] [diff] [review]
fix mozafterpaint (1.0)
Review of attachment 597266 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #597266 -
Flags: review?(ttaubert) → review+
Reporter | ||
Comment 7•13 years ago
|
||
Oh, while I actually think this is a safe fix, I'm not sure if I can give r+ for talos test modifications?
Assignee | ||
Updated•13 years ago
|
Attachment #597266 -
Flags: review?(jhammel)
Assignee | ||
Comment 8•13 years ago
|
||
do we also want this for tp5 (and related tests)?
Comment 9•13 years ago
|
||
Comment on attachment 597266 [details] [diff] [review]
fix mozafterpaint (1.0)
wfm
Attachment #597266 -
Flags: review?(jhammel) → review+
Assignee | ||
Comment 10•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 11•13 years ago
|
||
Are these changes deployed, or just checked-in?
Assignee | ||
Comment 12•13 years ago
|
||
I never received an answer to comment #8. These are checked in, but not deployed.
Comment 13•13 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #12)
> I never received an answer to comment #8. These are checked in, but not
> deployed.
Does anyone know who can answer that?
Comment 14•13 years ago
|
||
I think we want it for all performance tests that use MozAfterPaint.
Assignee | ||
Comment 15•13 years ago
|
||
it looks like the pageloader bits already do this, so we just need to deploy this change for ts, ts_paint.
Comment 16•13 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #15)
> it looks like the pageloader bits already do this, so we just need to deploy
> this change for ts, ts_paint.
Ok, thaks. What needs to happen for that? Should we file a separate bug, or can that happen here?
You need to log in
before you can comment on or make changes to this bug.
Description
•