WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
83635
[Chromium] Chromium's LayoutTestController is missing setBackingScaleFactor
https://bugs.webkit.org/show_bug.cgi?id=83635
Summary
[Chromium] Chromium's LayoutTestController is missing setBackingScaleFactor
James Simonsen
Reported
2012-04-10 16:19:35 PDT
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.
Attachments
Patch
(5.93 KB, patch)
2012-06-25 16:35 PDT
,
Terry Anderson
no flags
Details
Formatted Diff
Diff
Patch
(5.08 KB, patch)
2012-06-26 12:16 PDT
,
Terry Anderson
no flags
Details
Formatted Diff
Diff
Patch
(5.19 KB, patch)
2012-06-26 14:36 PDT
,
Terry Anderson
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-04
(494.35 KB, application/zip)
2012-06-26 15:53 PDT
,
WebKit Review Bot
no flags
Details
Patch
(4.76 KB, patch)
2012-06-28 10:37 PDT
,
Terry Anderson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Hajime Morrita
Comment 1
2012-06-17 21:47:38 PDT
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
Adam Barth
Comment 2
2012-06-18 14:29:19 PDT
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.
Adam Barth
Comment 3
2012-06-18 14:29:42 PDT
***
Bug 83941
has been marked as a duplicate of this bug. ***
Adam Barth
Comment 4
2012-06-18 14:42:08 PDT
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.
Terry Anderson
Comment 5
2012-06-25 16:35:28 PDT
Created
attachment 149389
[details]
Patch
Adam Barth
Comment 6
2012-06-25 16:51:58 PDT
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.
Tony Chang
Comment 7
2012-06-25 17:01:12 PDT
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.
Adam Barth
Comment 8
2012-06-25 17:03:27 PDT
(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.
Tony Chang
Comment 9
2012-06-25 17:09:20 PDT
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.
Terry Anderson
Comment 10
2012-06-26 12:16:00 PDT
Created
attachment 149582
[details]
Patch
Terry Anderson
Comment 11
2012-06-26 12:20:47 PDT
(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?
Adam Barth
Comment 12
2012-06-26 13:22:09 PDT
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 ?
Adam Barth
Comment 13
2012-06-26 13:33:35 PDT
> 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.
Tony Chang
Comment 14
2012-06-26 14:15:48 PDT
(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.
Adam Barth
Comment 15
2012-06-26 14:19:16 PDT
> 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.
Tony Chang
Comment 16
2012-06-26 14:22:51 PDT
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?
Terry Anderson
Comment 17
2012-06-26 14:29:03 PDT
(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.
Adam Barth
Comment 18
2012-06-26 14:31:32 PDT
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.
Terry Anderson
Comment 19
2012-06-26 14:36:29 PDT
Created
attachment 149609
[details]
Patch
Adam Barth
Comment 20
2012-06-26 14:39:15 PDT
Comment on
attachment 149609
[details]
Patch This all looks great. The only question remaining is about the m_waitUntilDone branch.
Adam Barth
Comment 21
2012-06-26 15:24:31 PDT
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.
Tony Chang
Comment 22
2012-06-26 15:50:56 PDT
(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.
WebKit Review Bot
Comment 23
2012-06-26 15:53:53 PDT
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
WebKit Review Bot
Comment 24
2012-06-26 15:53:58 PDT
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
Adam Barth
Comment 25
2012-06-26 16:18:16 PDT
Ok. Sounds like we should use postTask instead.
Terry Anderson
Comment 26
2012-06-28 10:37:16 PDT
Created
attachment 149969
[details]
Patch
Adam Barth
Comment 27
2012-06-28 11:23:20 PDT
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.
Terry Anderson
Comment 28
2012-06-28 11:37:24 PDT
(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
).
Stephen White
Comment 29
2012-06-28 11:45:26 PDT
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?
Stephen White
Comment 30
2012-06-28 11:47:07 PDT
Comment on
attachment 149969
[details]
Patch Argh, sorry. Restoring r+ and cq+.
Adam Barth
Comment 31
2012-06-28 11:50:07 PDT
> 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.
WebKit Review Bot
Comment 32
2012-06-28 12:45:25 PDT
Comment on
attachment 149969
[details]
Patch Clearing flags on attachment: 149969 Committed
r121453
: <
http://trac.webkit.org/changeset/121453
>
WebKit Review Bot
Comment 33
2012-06-28 12:45:33 PDT
All reviewed patches have been landed. Closing bug.
Stephen White
Comment 34
2012-06-28 14:42:16 PDT
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.
Adam Barth
Comment 35
2012-06-28 14:47:30 PDT
The scale factor should be reset to 1 at the end of the test. We might need to add something to the reset method.
Terry Anderson
Comment 36
2012-06-28 14:57:53 PDT
(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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug