Closed
Bug 512645
Opened 15 years ago
Closed 15 years ago
Only clamp nested timeouts
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: vlad, Assigned: peterv)
References
Details
(Keywords: perf, Whiteboard: [ts])
Attachments
(1 file)
(deleted),
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
chrome uses setTimeout(0) often to just wait for an event loop cycle or similar; we should not enforce the 10ms minimum for chrome windows.
Reporter | ||
Updated•15 years ago
|
Whiteboard: [ts]
Reporter | ||
Updated•15 years ago
|
Summary: setTimeout sohuld not enforce DOM_MIN_TIMEOUT_VALUE for chrome → setTimeout should not enforce DOM_MIN_TIMEOUT_VALUE for chrome
Comment 1•15 years ago
|
||
Booyah.
Assignee | ||
Comment 2•15 years ago
|
||
I'm going to morph this, we just discussed this yesterday. We shouldn't special-case chrome, we should follow HTML5 and only clamp for nested timeouts. HTML5 says to clamp for nesting level >= 1, Webkit seems to clamp for nesting level >= 5. iBench chains 3 timeouts, so following Webkit would also help our score there.
Assignee: nobody → peterv
Summary: setTimeout should not enforce DOM_MIN_TIMEOUT_VALUE for chrome → Only clamp nested timeouts
Whatever we do here, we should make sure to tweak the worker setTimeout code as well as the normal Window one.
Assignee | ||
Comment 4•15 years ago
|
||
Still trying to write a testcase for this, but it's not easy to make a reliable one.
Attachment #402351 -
Flags: review?(jst)
Assignee | ||
Comment 5•15 years ago
|
||
I copied WebKit for now, using 5 for the nesting level at which to start clamping.
(In reply to comment #3)
> Whatever we do here, we should make sure to tweak the worker setTimeout code as
> well as the normal Window one.
Workers don't seem to clamp timeouts at all. Want to file a separate bug for adding clamping?
Actually, would it make sense to not do what webkit does and instead simply follow the spec, and then point to this as for why iBench is totally broken in its current state?
Filed bug 518406 on worker timeouts.
(In reply to comment #7)
> Filed bug 518406 on worker timeouts.
Punks! ;)
(In reply to comment #6)
> Actually, would it make sense to not do what webkit does and instead simply
> follow the spec, and then point to this as for why iBench is totally broken in
> its current state?
I don't think it would.
I don't see how allowing some nesting this way is worse for the web, since spec-compliant code still runs fine, and complaining about iBench is unlikely to have any effect on the world (it'll never get changed).
I agree iBench is unlikely to get fixed. And that this fix isn't bad for the web.
However my understanding was that it was so broken that it's unlikely that we'll ever get any useful data out of it, so we'd really like Apple to quit using it. Having glaring bugs like this to point at would give us a strong case for that Apple is not acting in good faith if they keep using it.
iBench has the bug regardless of what we do here, so point away!
Comment 12•15 years ago
|
||
are there open questions here or just waiting on review? if the broadening of the change suggested in comment #0 is slowing things down, can we just do the minimal change first?
I think we're just waiting for jsts review.
Updated•15 years ago
|
Attachment #402351 -
Flags: superreview+
Attachment #402351 -
Flags: review?(jst)
Attachment #402351 -
Flags: review+
Updated•15 years ago
|
Whiteboard: [ts] → [ts][has patch][can land]
Assignee | ||
Comment 14•15 years ago
|
||
Sorry for the delay in checking this in.
http://hg.mozilla.org/mozilla-central/rev/350ffc1d793a
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [ts][has patch][can land] → [ts]
Target Milestone: --- → mozilla1.9.3a1
You need to log in
before you can comment on or make changes to this bug.
Description
•