Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738773004:
No longer clamp setTimeout(..., 0) to 1ms. (Closed)
|
Created:
3 years, 9 months ago by Dan Elphick Modified:
3 years, 8 months ago CC:
chromium-reviews, blink-reviews-w3ctests_chromium.org, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, blink-reviews-frames_chromium.org, kozyatinskiy+blink_chromium.org, xlai (Olivia) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNo longer clamp setTimeout(..., 0) to 1ms.
Update several -expected.txt files to remove console logs that don't appear
without the 1ms clamp as the test ends before they can. In all cases these
are wpt tests which were never intended to test the console behaviour (and
in fact will likely be removed soon in a different change).
Update further tests to use setTimeout(..., 1) where the tests were really
relying on this behaviour.
BUG=402694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2738773004
Cr-Commit-Position: refs/heads/master@{#463591}
Committed: https://chromium.googlesource.com/chromium/src/+/d8ade60fbe0ea954ecd5f186c36e6d0ac045c28c
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add a TODO to try removing the singleShot guard #Patch Set 3 : Add unit test to ensure 0ms setTimeouts aren't clamped to 1ms #Patch Set 4 : update expected files (wpt tests no longer have console logging) #Patch Set 5 : revert all throttler related changes #Patch Set 6 : Add a semi-working fix for one failing test using a promise #Patch Set 7 : Use nested setTimeouts due to changes in how commits are scheduled for Offscreen canvases. Added a … #Patch Set 8 : revert set-breakpoint.html #Patch Set 9 : merge in blink reformat changes (and correct some due to my previous mistakes in DOMTimerTest.cpp) #Patch Set 10 : s/toDoubleValue/ToDoubleValue/ in DOMTimerTest.cpp #Patch Set 11 : update webgl_conformance expectations since setTimeout(..., 0) now fails #Patch Set 12 : Change const char* to const char* const to make a proper constant #Patch Set 13 : update webgl2 conformance expectations as well #Messages
Total messages: 73 (39 generated)
Description was changed from ========== No longer clamp setTimeout(..., 0) to 1ms. Change devtools Common.Throttler to use a minimum of 1ms when scheduling with asSoonAsPossible == true, as 0ms setTimeout causes flakiness in the debugger tests. Update throttler test for change in scheduled timers. Update several -expected.txt files to remove console logs that don't appear without the 1ms clamp as the test ends before they can. In all cases these are wpt tests which were never intended to test the console behaviour (and in fact will likely be removed soon in a different change). Update further tests to use setTimeout(..., 1) where the tests were really relying on this behaviour. BUG=402694 ========== to ========== No longer clamp setTimeout(..., 0) to 1ms. Change devtools Common.Throttler to use a minimum of 1ms when scheduling with asSoonAsPossible == true, as 0ms setTimeout causes flakiness in the debugger tests. Update throttler test for change in scheduled timers. Update several -expected.txt files to remove console logs that don't appear without the 1ms clamp as the test ends before they can. In all cases these are wpt tests which were never intended to test the console behaviour (and in fact will likely be removed soon in a different change). Update further tests to use setTimeout(..., 1) where the tests were really relying on this behaviour. BUG=402694 ==========
delphick@chromium.org changed reviewers: + pfeldman@chromium.org, skyostil@chromium.org
The CQ bit was checked by delphick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Pavel, I've had to modify Common.Throttler to use a minimum of 1ms when scheduling using setTimeout(). This doesn't actually change its behaviour since previously it was limited by the implementation of setTimeout anyway. Several tests were failing without this extra change, for instance: inspector/sources/debugger-breakpoints/set-breakpoint.html
lgtm https://codereview.chromium.org/2738773004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right): https://codereview.chromium.org/2738773004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimer.cpp:96: std::max(singleShot ? 0.0 : oneMillisecond, interval * oneMillisecond); Did we try to make the minimum always be 0 for all types of timers? That's what the spec calls for I think. Maybe add a TODO for that here?
On 2017/03/08 14:15:55, Sami wrote: > lgtm > > https://codereview.chromium.org/2738773004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right): > > https://codereview.chromium.org/2738773004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/DOMTimer.cpp:96: std::max(singleShot ? 0.0 > : oneMillisecond, interval * oneMillisecond); > Did we try to make the minimum always be 0 for all types of timers? That's what > the spec calls for I think. Maybe add a TODO for that here? I haven't tried it yet. TODO added. Once this is in, I'll experiment with removing the singleShot guard safe in the knowledge that I only need to search for setInterval when it all comes crashing down.
The CQ bit was checked by delphick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hi Pavel, could you take a look at the Throttler related changes?
On 2017/03/14 11:14:40, Dan Elphick wrote: > Hi Pavel, could you take a look at the Throttler related changes? Wait, you make entire world flaky, but only fix the devtools? You should fix the world as well. What I'm hinting at is that this change should not result in flakiness of any kind and if it does - something went wrong. Thousands of testing frameworks out there have settled on the existing behavior and you are disrupting them just as you disrupt devtools.
On 2017/03/14 20:54:33, pfeldman_slow wrote: > On 2017/03/14 11:14:40, Dan Elphick wrote: > > Hi Pavel, could you take a look at the Throttler related changes? > > Wait, you make entire world flaky, but only fix the devtools? You should fix the > world as well. What I'm hinting at is that this change should not result in > flakiness of any kind and if it does - something went wrong. Thousands of > testing frameworks out there have settled on the existing behavior and you are > disrupting them just as you disrupt devtools. Are you saying we shouldn't make setTimeout(..., 0) actually have a <1ms timeout? I believe other browsers do not have this behaviour and it certainly doesn't match the spec. The flakiness is a result of tests and/or devtools relying on an implementation detail. If you're saying we can't make this change if there's _any_ code out there that could break as a result of this, then we can't make this change at all since we can't make that guarantee. Is there instead some level of testing + proposing of fixes to external frameworks that would make this acceptable?
On 2017/03/14 22:10:49, Dan Elphick wrote: > Are you saying we shouldn't make setTimeout(..., 0) actually have a <1ms > timeout? I believe other browsers do not have this behaviour and it certainly > doesn't match the spec. As far as I can check, Firefox uses 4ms as min value and 1000 ms for background timeouts [1], Safary uses 1ms [2]. Let me quote comment (@alexclarke) from linked issue [3]: 1. There's a lot of content that incorrectly uses setTimeout(0, ...) which will misbehave if we change this 2. There are plenty of alternatives: http://jsperf.com/setimmediate-test/24 3. requestIdleCallback is a much better API for sharing the thread, since it tells you when the browser needs control back https://w3c.github.io/requestidlecallback/ I agree with Pavel that this CL potentially will break the web. Just wondering, why we need this change? [1] https://hg.mozilla.org/mozilla-central/file/2b52802899ae/dom/base/TimeoutMana... [2] https://github.com/WebKit/webkit/blob/66edba20f1dad57000ddb39340816df385a5d86... [3] https://bugs.chromium.org/p/chromium/issues/detail?id=402694#c30
On 2017/03/14 22:52:48, kozy wrote: > On 2017/03/14 22:10:49, Dan Elphick wrote: > > Are you saying we shouldn't make setTimeout(..., 0) actually have a <1ms > > timeout? I believe other browsers do not have this behaviour and it certainly > > doesn't match the spec. > > As far as I can check, Firefox uses 4ms as min value and 1000 ms for background > timeouts [1], Safary uses 1ms [2]. > > Let me quote comment (@alexclarke) from linked issue [3]: > > 1. There's a lot of content that incorrectly uses setTimeout(0, ...) which will > misbehave if we change this By misbehave I had two concerns in mind: Excessive CPU usage and flakiness. Re excessive CPU usage, I no longer think this is a problem, because there is a 4ms clamp after the nesting limit is hit and because the blink scheduler can prioritise compositing and input over js timers as needed. Re flakiness I'm not sure this is actually a problem on real pages (I've only ever seen internal chromium pages affected, all of which were making invalid assumptions about when paints would occur). It *is* a problem for tests which rely on the exact order of console logs vs other chromium tasks. Arguably those tests are already broken because the exact timing of those console logs is not deterministic. Maybe we should fix those tests? > 2. There are plenty of alternatives: http://jsperf.com/setimmediate-test/24 > 3. requestIdleCallback is a much better API for sharing the thread, since it > tells you when the browser needs control back > https://w3c.github.io/requestidlecallback/ The problem with requestIdleCallback is there (quite often) is no idle time at all. > > I agree with Pavel that this CL potentially will break the web. Just wondering, > why we need this change? > > [1] > https://hg.mozilla.org/mozilla-central/file/2b52802899ae/dom/base/TimeoutMana... > [2] > https://github.com/WebKit/webkit/blob/66edba20f1dad57000ddb39340816df385a5d86... > [3] https://bugs.chromium.org/p/chromium/issues/detail?id=402694#c30
On 2017/03/14 22:52:48, kozy wrote:
> On 2017/03/14 22:10:49, Dan Elphick wrote:
> > Are you saying we shouldn't make setTimeout(..., 0) actually have a <1ms
> > timeout? I believe other browsers do not have this behaviour and it
certainly
> > doesn't match the spec.
>
> As far as I can check, Firefox uses 4ms as min value and 1000 ms for
background
> timeouts [1], Safary uses 1ms [2].
This does not appear to be true for Firefox. Running this repeatedly on my Mac I
got values from ~0.5 to ~2 with over 50% below 1ms:
t = window.performance.now(); setTimeout(() =>
{console.log(window.performance.now() - t)}, 0)
I suspect that 4ms value is the clamp that's imposed after nesting 5 levels of
setTimeout.
I guess the question was "what made you change your mind", and it seems the answer is: - "4ms clamp after the nesting limit is hit" and - "blink scheduler can prioritise compositing and input over js timers as needed". That seems fair. I'm just concerned that if it broke us, it'll break others. For example, the change to insecure-plugin-in-iframe.html seems to be deterministic, setTimeout has changed its order wrt something (onload?) in a non-flaky manner. And since it carries risk, it is interesting to get the context on the motivation. Are we gaining anything with the change? Performance improvement? It does not seem to affect code maintainability much.
(could you point to the flaky results in case you don't patch Throttler? - we would look at the flakiness dashboard and see if they were racy?) (same for security tests above, if we know that we change the behavior here without introducing flakiness component, we are fine, just want to make sure we understand why something changed)
On 2017/03/15 11:39:39, pfeldman_slow wrote: > (could you point to the flaky results in case you don't patch Throttler? - we > would look at the flakiness dashboard and see if they were racy?) > (same for security tests above, if we know that we change the behavior here > without introducing flakiness component, we are fine, just want to make sure we > understand why something changed) The inspector tests are not flaky without the change to setTimeout's internal clamp. With the clamp removed but without the Throttler change, the inspector tests would pass something like 30% of the time. With my the Throttler change as well they pass 100% of the time.
On 2017/03/15 11:45:15, Dan Elphick wrote: > On 2017/03/15 11:39:39, pfeldman_slow wrote: > > (could you point to the flaky results in case you don't patch Throttler? - we > > would look at the flakiness dashboard and see if they were racy?) > > (same for security tests above, if we know that we change the behavior here > > without introducing flakiness component, we are fine, just want to make sure > we > > understand why something changed) > > The inspector tests are not flaky without the change to setTimeout's internal > clamp. With the clamp removed but without the Throttler change, the inspector > tests would pass something like 30% of the time. With my the Throttler change as > well they pass 100% of the time. As for the security tests, I wouldn't say they they are flaky without these changes, they will just fail. The first one because the console message changed due to it adding line numbers. The second one fails because the test terminates too quickly for the console message to get output to the test output, causing a text mismatch.
> The inspector tests are not flaky without the change to setTimeout's internal > clamp. With the clamp removed but without the Throttler change, the inspector > tests would pass something like 30% of the time. With my the Throttler change > as well they pass 100% of the time. Right, but do you know why this is happening? > As for the security tests, I wouldn't say they they are flaky without these > changes, they will just fail. The first one because the console message changed > due to it adding line numbers. I can see that, but why did it change? > The second one fails because the test terminates > too quickly for the console message to get output to the test output, causing a > text mismatch. Ditton
On 2017/03/16 00:28:00, pfeldman_slow wrote:
> > The inspector tests are not flaky without the change to setTimeout's
internal
> > clamp. With the clamp removed but without the Throttler change, the
inspector
> > tests would pass something like 30% of the time. With my the Throttler
change
> > as well they pass 100% of the time.
>
> Right, but do you know why this is happening?
Still looking into this.
> > As for the security tests, I wouldn't say they they are flaky without these
> > changes, they will just fail. The first one because the console message
> changed
> > due to it adding line numbers.
>
> I can see that, but why did it change?
The setTimeout changes what causes the warning. With setTimeout unclamped it
happens due to this code:
internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks(document);
which is in the setTimeout block and therefore it has a line number that gets
logged.
Previously it happened during scheduleUpdatePluginsIfNecessary which is not
directly invoked from within the page javascript and so the message had no line
number.
The change is therefore irrelevent to the behaviour being test and so I think my
change to the expected file is reasonable. An alternative would be to change the
timeout to 1 to match the clamped behaviour.
> > The second one fails because the test terminates
> > too quickly for the console message to get output to the test output,
causing
> a
> > text mismatch.
>
> Ditton
In this case, the main test loads a script from a cross origin script. The
loaded script has an onload which just calls finishTest:
function finishTest() {
setTimeout(function() {
if (window.testRunner) {
testRunner.notifyDone();
}
}, 0);
}
The cross origin script creates a promise that always rejects. Since the promise
has no catch, an uncaught promise error is logged to the console. Without the
change to setTimeout in finishTest, this message doesn't appear. The reason is
that RejectedPromises::processQueue is called after onload and it posts a task.
Previously that task would have then run immediately but with the change to
setTimeout, finishTest's setTimeout runs first and so the test terminates.
Since it's the unhandled promise rejection that is being tested, I can't add
code to wait for this to happen as then the promise wouldn't be unhandled. In
this case the only solution is to either use setTimeout(..., 1) or to nest
setTimeouts, setTimeout(() => { setTimeout(..., 0) }, 0);
in_reply_to: 5743114304094208
send_mail: 1
subject: No longer clamp setTimeout(..., 0) to 1ms.
On 2017/03/16 14:45:43, Dan Elphick wrote: > On 2017/03/16 00:28:00, pfeldman_slow wrote: > > > The inspector tests are not flaky without the change to setTimeout's > internal > > > clamp. With the clamp removed but without the Throttler change, the > inspector > > > tests would pass something like 30% of the time. With my the Throttler > change > > > as well they pass 100% of the time. > > > > Right, but do you know why this is happening? > > Still looking into this. So the pass rate is actually a lot lower, although it tends upwards with higher numbers of iterations. E.g. 1-2 passes in first 50 and maybe 10 passes in the second 50 using --iterations=50. Anyway, the major difference between 0 and 1ms setTimeouts is when Sources.JavaScriptBreakpointsSidebarPane.doUpdate (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...) gets called. With a true 0ms setTimeout, it gets called apparently before the source frame is loaded and before any breakpoints are set. It doesn't get called again until after the test is complete. With a 1ms setTimeout, the didShowScriptSource method in set-breakpoint.html gets called first and so the source is loaded and there's a breakpoint set. It seems clear to me that there's a bug in the synchronisation between the various frames in this test that was masked by the previous 1ms clamp in setTimeout. I think it's also very unlikely something like this would happen in the real world and is largely due to the hoops one must jump through to construct a layout test for the debugger.
kozyatinskiy@chromium.org changed reviewers: + kozyatinskiy@chromium.org
fix for set-breakpoint.html will be landed soon: https://codereview.chromium.org/2776523004/. Sorry for delay.
On 2017/03/24 02:54:21, kozy wrote: > fix for set-breakpoint.html will be landed soon: > https://codereview.chromium.org/2776523004/. > Sorry for delay. Thanks for looking into this. I can confirm your patch makes the test pass without the Throttler change. Can you also take a look at these tests which have the same problem: inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html inspector/sources/debugger-breakpoints/disable-breakpoints.html inspector/sources/debugger/js-with-inline-stylesheets.html
On 2017/03/24 10:20:17, Dan Elphick wrote: > On 2017/03/24 02:54:21, kozy wrote: > > fix for set-breakpoint.html will be landed soon: > > https://codereview.chromium.org/2776523004/. > > Sorry for delay. > > Thanks for looking into this. I can confirm your patch makes the test pass > without the Throttler change. Can you also take a look at these tests which have > the same problem: > > inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html > inspector/sources/debugger-breakpoints/disable-breakpoints.html > inspector/sources/debugger/js-with-inline-stylesheets.html I added fixes for these tests too and run all tests with this CL - it works. Thanks for the list of bad inspector tests.
On 2017/03/24 15:55:11, kozy wrote: > On 2017/03/24 10:20:17, Dan Elphick wrote: > > On 2017/03/24 02:54:21, kozy wrote: > > > fix for set-breakpoint.html will be landed soon: > > > https://codereview.chromium.org/2776523004/. > > > Sorry for delay. > > > > Thanks for looking into this. I can confirm your patch makes the test pass > > without the Throttler change. Can you also take a look at these tests which > have > > the same problem: > > > > inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html > > inspector/sources/debugger-breakpoints/disable-breakpoints.html > > inspector/sources/debugger/js-with-inline-stylesheets.html > > I added fixes for these tests too and run all tests with this CL - it works. > Thanks for the list of bad inspector tests. Great! I've removed all Throttler related changes from this CL now in anticipation of your fix landing. Pavel, can you take another look?
lgtm
The CQ bit was checked by delphick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2738773004/#ps80001 (title: "revert all throttler related changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== No longer clamp setTimeout(..., 0) to 1ms. Change devtools Common.Throttler to use a minimum of 1ms when scheduling with asSoonAsPossible == true, as 0ms setTimeout causes flakiness in the debugger tests. Update throttler test for change in scheduled timers. Update several -expected.txt files to remove console logs that don't appear without the 1ms clamp as the test ends before they can. In all cases these are wpt tests which were never intended to test the console behaviour (and in fact will likely be removed soon in a different change). Update further tests to use setTimeout(..., 1) where the tests were really relying on this behaviour. BUG=402694 ========== to ========== No longer clamp setTimeout(..., 0) to 1ms. Update several -expected.txt files to remove console logs that don't appear without the 1ms clamp as the test ends before they can. In all cases these are wpt tests which were never intended to test the console behaviour (and in fact will likely be removed soon in a different change). Update further tests to use setTimeout(..., 1) where the tests were really relying on this behaviour. BUG=402694 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by delphick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by delphick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
delphick@chromium.org changed reviewers: + junov@chromium.org
Justin, I've added the agreed nested setTimeouts to the offscreen canvas tests. Can you LGTM since I've marked the TODOs against you:)
The CQ bit was checked by skyostil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by skyostil@chromium.org
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2738773004/#ps140001 (title: "revert set-breakpoint.html")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by delphick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, junov@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2738773004/#ps180001 (title: "s/toDoubleValue/ToDoubleValue/ in DOMTimerTest.cpp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by delphick@chromium.org
Description was changed from ========== No longer clamp setTimeout(..., 0) to 1ms. Update several -expected.txt files to remove console logs that don't appear without the 1ms clamp as the test ends before they can. In all cases these are wpt tests which were never intended to test the console behaviour (and in fact will likely be removed soon in a different change). Update further tests to use setTimeout(..., 1) where the tests were really relying on this behaviour. BUG=402694 ========== to ========== No longer clamp setTimeout(..., 0) to 1ms. Update several -expected.txt files to remove console logs that don't appear without the 1ms clamp as the test ends before they can. In all cases these are wpt tests which were never intended to test the console behaviour (and in fact will likely be removed soon in a different change). Update further tests to use setTimeout(..., 1) where the tests were really relying on this behaviour. BUG=402694 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
PTAL Justin, I've updated the expectations file for the offscreen canvas webgl conformance tests since that now fails because setTimeout(..., 0) will execute before the commit completes. (The promise returned by commit() didn't help).
This is fine. I will take care of updating the WebGL tests. Thanks for bringing it to my attention. lgtm
The CQ bit was checked by delphick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2738773004/#ps200001 (title: "update webgl_conformance expectations since setTimeout(..., 0) now fails")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by delphick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, junov@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2738773004/#ps240001 (title: "update webgl2 conformance expectations as well")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491898782165440,
"parent_rev": "f7fb871305a1a2a2c3bf0edf137d14bd72c530ff", "commit_rev":
"d8ade60fbe0ea954ecd5f186c36e6d0ac045c28c"}
Message was sent while issue was closed.
Description was changed from ========== No longer clamp setTimeout(..., 0) to 1ms. Update several -expected.txt files to remove console logs that don't appear without the 1ms clamp as the test ends before they can. In all cases these are wpt tests which were never intended to test the console behaviour (and in fact will likely be removed soon in a different change). Update further tests to use setTimeout(..., 1) where the tests were really relying on this behaviour. BUG=402694 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== No longer clamp setTimeout(..., 0) to 1ms. Update several -expected.txt files to remove console logs that don't appear without the 1ms clamp as the test ends before they can. In all cases these are wpt tests which were never intended to test the console behaviour (and in fact will likely be removed soon in a different change). Update further tests to use setTimeout(..., 1) where the tests were really relying on this behaviour. BUG=402694 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2738773004 Cr-Commit-Position: refs/heads/master@{#463591} Committed: https://chromium.googlesource.com/chromium/src/+/d8ade60fbe0ea954ecd5f186c36e... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/d8ade60fbe0ea954ecd5f186c36e... |
