The following layout test times out on Chromium (all OSes & GPU/SW): fast/canvas/2d.backingStorePixelRatio.html Probable cause: Test was added in change 113780.
Regressions: Unexpected text diff mismatch : (4) fast/canvas/2d.backingStorePixelRatio.html = TEXT fast/canvas/2d.imageDataHD.html = TEXT platform/chromium/virtual/gpu/fast/canvas/2d.backingStorePixelRatio.html = TEXT platform/chromium/virtual/gpu/fast/canvas/2d.imageDataHD.html = TEXT Looks like these started failing due to scale factor related change. Garden-o-matic told me that this start from http://trac.webkit.org/changeset/120547. But I'm not sure if that is correct. Flakiness dashboard: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fcanvas%2F2d.backingStorePixelRatio.html%2Cfast%2Fcanvas%2F2d.imageDataHD.html%2Cplatform%2Fchromium%2Fvirtual%2Fgpu%2Ffast%2Fcanvas%2F2d.backingStorePixelRatio.html%2Cplatform%2Fchromium%2Fvirtual%2Fgpu%2Ffast%2Fcanvas%2F2d.imageDataHD.html
So, this was a progression (from TIMEOUT to TEXT), but I'm going to need to back the change out because it doesn't work for WebKit2.
*** Bug 83941 has been marked as a duplicate of this bug. ***
These tests should go back to timing out now that http://trac.webkit.org/changeset/120627 has landed. Once we implement layoutTestController.setBackingScaleFactor, they'll likely return to TEXT.
Created attachment 149389 [details] Patch
Comment on attachment 149389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149389&action=review > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2227 > + CppVariant a[1]; > + a[0].set(m_arguments[1]); > + CppVariant invokeResult; > + return m_arguments[1].invokeDefault(a, 1, invokeResult); Should we generalize this to work with any number of arguments? > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2243 > + m_callbackQueued = true; Rather than introduce m_callbackQueued, I wonder if we should call m_workQueue.processWorkSoon() directly here.
Comment on attachment 149389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149389&action=review > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2223 > + bool run(TestShell* shell) > + { > + UNUSED_PARAM(shell); Nit: WebKit style is to just not name the unused params. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2239 > + float val = arguments[0].value.doubleValue; Nit: val -> value >> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2243 >> + m_callbackQueued = true; > > Rather than introduce m_callbackQueued, I wonder if we should call m_workQueue.processWorkSoon() directly here. Does this have to be async? In Apple Mac DRT, it looks like a synchronous call.
(In reply to comment #7) > (From update of attachment 149389 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149389&action=review > > >> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2243 > >> + m_callbackQueued = true; > > > > Rather than introduce m_callbackQueued, I wonder if we should call m_workQueue.processWorkSoon() directly here. > > Does this have to be async? In Apple Mac DRT, it looks like a synchronous call. It's async in WebKit2, but maybe test code needs to be prepared to handle both sync and async callbacks.
Comment on attachment 149389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149389&action=review >>>> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2243 >>>> + m_callbackQueued = true; >>> >>> Rather than introduce m_callbackQueued, I wonder if we should call m_workQueue.processWorkSoon() directly here. >> >> Does this have to be async? In Apple Mac DRT, it looks like a synchronous call. > > It's async in WebKit2, but maybe test code needs to be prepared to handle both sync and async callbacks. Ah, you're right. I wonder if we can just post the task directly rather than using m_workQueue. That seems to be more like what WTR is doing.
Created attachment 149582 [details] Patch
(In reply to comment #10) > Created an attachment (id=149582) [details] > Patch m_callbackQueued is gone, WorkItemInvokeDefault is now completely general, and the nits are addressed. What is the verdict on using m_workQueue for an async callback as this patch does vs. making a sync call?
Comment on attachment 149582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149582&action=review > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:715 > - if (!m_waitUntilDone) > - m_workQueue.processWorkSoon(); > + m_workQueue.processWorkSoon(); This change doesn't cause problems for other tests? It might be worth understanding why the branch was here originally. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2215 > + WorkItemInvokeDefault(PassOwnArrayPtr<CppVariant> callbackArgs, uint32_t numArgs) : > + m_callbackArgs(callbackArgs), > + m_numArgs(numArgs) { } This isn't quite in WebKit style. The WebKit style would be something like: WorkItemInvokeDefault(PassOwnArrayPtr<CppVariant> callbackArguments, uint32_t numberOfArguments) : : m_callbackArguments(callbackArguments), , m_numberOfArguments(numberOfArguments) { } The : and the , go in those funny locations to make adding and removing properties easier. Also, we prefer to use complete words in variable names rather than abbreviations. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2219 > + return m_callbackArgs[0].invokeDefault(m_callbackArgs.get(), 1, invokeResult); 1 -> m_numArgs ?
> What is the verdict on using m_workQueue for an async callback as this patch does vs. making a sync call? IMHO, it's better to have this be async given that is how it works on WebKit2. I don't feel strongly about it though if folks have other thoughts.
(In reply to comment #13) > > What is the verdict on using m_workQueue for an async callback as this patch does vs. making a sync call? > > IMHO, it's better to have this be async given that is how it works on WebKit2. I don't feel strongly about it though if folks have other thoughts. I agree, it should be async. Using the work queue is a bit weird because it's for things like queueReload or queueBackNavigation, but it's probably fine since the order in which we run these operations shouldn't matter. It's probably safe to remove that check. All it means is that we'll run tasks (like queueReload) if the user forgot to call waitUntilDone(). Assuming the test is written properly, that should be safe.
> It's probably safe to remove that check. All it means is that we'll run tasks (like queueReload) if the user forgot to call waitUntilDone(). Assuming the test is written properly, that should be safe. I wonder if we could ASSERT that we're waitUntilDone when those queue functions are called.
In reply to comment #14) > It's probably safe to remove that check. All it means is that we'll run tasks (like queueReload) if the user forgot to call waitUntilDone(). Assuming the test is written properly, that should be safe. Actually, I don't understand why we need to remove the check. A quick look through seems to suggest that all uses of testRunner.setBackingScaleFactor also use testRunner.waitUntilDone. Is there a specific test that doesn't do this?
(In reply to comment #16) > In reply to comment #14) > > It's probably safe to remove that check. All it means is that we'll run tasks (like queueReload) if the user forgot to call waitUntilDone(). Assuming the test is written properly, that should be safe. > > Actually, I don't understand why we need to remove the check. A quick look through seems to suggest that all uses of testRunner.setBackingScaleFactor also use testRunner.waitUntilDone. Is there a specific test that doesn't do this? Leaving the check in will cause the tests in fast/hidpi to time out. I've tried this for fast/hidpi/focus-rings.html and fast/hidpi/image-set-background-repeat.html.
From fast/hidpi/focus-rings.html: if (window.testRunner) { testRunner.waitUntilDone(); testRunner.setBackingScaleFactor(2, finishTest); } That would seem to indicate that it would work. I'm not sure what's going on here.
Created attachment 149609 [details] Patch
Comment on attachment 149609 [details] Patch This all looks great. The only question remaining is about the m_waitUntilDone branch.
Comment on attachment 149609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149609&action=review > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:-717 > - if (!m_waitUntilDone) In the case of focusRing, m_waitUntilDone is true (as we would expect since the test calls testRunner.waitUntilDone() ), which means we *won't* kick off the work queue. It sounds like we should indeed remove this branch.
(In reply to comment #21) > (From update of attachment 149609 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149609&action=review > > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:-717 > > - if (!m_waitUntilDone) > > In the case of focusRing, m_waitUntilDone is true (as we would expect since the test calls testRunner.waitUntilDone() ), which means we *won't* kick off the work queue. It sounds like we should indeed remove this branch. I'm a bit confused how the work gets done in the tests that don't use setBackingScaleFactor, but I think it would be safer to just not use the work queue and use postTask (in Task.h) directly. We don't really need to queue the task next to the queue* methods. postTask should just add the message to the message loop and we keep running that in waitTestFinished() until notifyDone is called.
Comment on attachment 149609 [details] Patch Attachment 149609 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13099534 New failing tests: http/tests/navigation/timerredirect-goback.html http/tests/navigation/javascriptlink-goback.html http/tests/navigation/metaredirect-goback.html
Created attachment 149621 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Ok. Sounds like we should use postTask instead.
Created attachment 149969 [details] Patch
Comment on attachment 149969 [details] Patch Looks great. We might need to change the TestExpectations for the hidpi tests from TIMEOUT to something else, but other than that we should be good to go.
(In reply to comment #27) > (From update of attachment 149969 [details]) > Looks great. We might need to change the TestExpectations for the hidpi tests from TIMEOUT to something else, but other than that we should be good to go. I've filed a separate bug for this (https://bugs.webkit.org/show_bug.cgi?id=90190).
Comment on attachment 149969 [details] Patch Stupid question: is there a reason why the callback has to be async? (I realize it's implemented this way in the Mac port.) Is it not possible (or not a good idea) to just call back into JS immediately?
Comment on attachment 149969 [details] Patch Argh, sorry. Restoring r+ and cq+.
> Stupid question: is there a reason why the callback has to be async? (I realize it's implemented this way in the Mac port.) Is it not possible (or not a good idea) to just call back into JS immediately? We just want the tests to work the same on all the ports. That way folks who write tests don't need to worry about whether the callback works differently on different ports.
Comment on attachment 149969 [details] Patch Clearing flags on attachment: 149969 Committed r121453: <http://trac.webkit.org/changeset/121453>
All reviewed patches have been landed. Closing bug.
This patch seems to be causing a large amount of flaky test runs (e.g., http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/27531, unexpected flaky 591). I reverted the patch locally, and it went away. I think what's happening is this: 2d.backingStorePixelRatio passes, but leaves the pixel ratio at 2. Every test after that fails IMAGE, but then passes on retry (since 2d.backingStorePixelRatio doesn't get run, since it passed, and the pixel ratio is left alone). I think the intent of the callback was that it should reset the backing scale factor back to 1 after the callback code is run, although I don't see that in the Mac implementation either.
The scale factor should be reset to 1 at the end of the test. We might need to add something to the reset method.
(In reply to comment #35) > The scale factor should be reset to 1 at the end of the test. We might need to add something to the reset method. I am addressing this in https://bugs.webkit.org/show_bug.cgi?id=90212