Bug 102184

Created attachment 176235 [details] Patch

Comment on attachment 176235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176235&action=review Nice, thanks for doing this useful work. (Disclaimer: I am not a reviewer, just doing an informal review since I adapted the tests originally). Other than those minor suggestions, looks good to me. > LayoutTests/ChangeLog:8 > + Reduce all timeouts by five. by five -> to a fifth of the previous value. But aren't you reducing them only to a third? > LayoutTests/http/tests/resources/load-and-stall.cgi:25 > + Time::HiRes::sleep($stallFor) Did you check whether the other tests using load-and-stall.cgi continue to work fine? I'd recommend to put this change into a separate bug and land this first, see whether the bots stay green. Once it's in the tree, land the rest of the change. This would help avoiding someone rolling out the whole time adjustment change. > LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:14 > +var timeNormalLoad = 1000; I'd spell these capitalized to indicate they're constant, e.g. TIME_NORMAL_LOAD, TIME_LATE_TIMEOUT. > LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:17 > +var timeTimeout = 400; As mentioned below when commenting on usage of timeDelay/TIME_DELAY, you could have an extra timeout value here: TIME_REGULAR_TIMEOUT and TIME_SYNC_TIMEOUT. > LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:21 > + * This should point to a resource that responds after a delay. You could make this comment include the variable, e.g. "This should point to a resource that responds after a delay of TIME_XHR_LOAD_EVENT." > LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:270 > + req.timeout = timeDelay; timeDelay has a different meaning in the other cases it's used, i.e. it describes when to do a late update to the timer. I'd say we should use a separate constant here, something like TIME_SYNC_TIMEOUT. > LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:284 > + req.timeout = timeDelay; Ditto.

Comment on attachment 176235 [details] Patch 220 secs -> 48 secs, nice progression ;)

I just saw Dominik's comment, it sounds reasonable. Could you fix them before landing?

(In reply to comment #2) > (From update of attachment 176235 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176235&action=review > > LayoutTests/ChangeLog:8 > > + Reduce all timeouts by five. > > by five -> to a fifth of the previous value. But aren't you reducing them only to a third? Sorry, I checked again, a fifth it is - for the important values. I was mislead by the line that removes the testharness.js timeout factor "3 *".

Committed r135852: <http://trac.webkit.org/changeset/135852>