[Chromium] Chromium's LayoutTestController is missing setBackingScaleFactor

• Component: Tools / Tests
• Status: RESOLVED FIXED

[James Simonsen]

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.

[Hajime Morrita]

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]

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]

*** Bug 83941 has been marked as a duplicate of this bug. ***

[Adam Barth]

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]

Created attachment 149389 [details]
Patch

[Adam Barth]

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 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]

(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 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]

Created attachment 149582 [details]
Patch

[Terry Anderson]

(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 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]

> 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]

(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]

> 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]

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]

(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]

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]

Created attachment 149609 [details]
Patch

[Adam Barth]

Comment on attachment 149609 [details]
Patch

This all looks great.  The only question remaining is about the m_waitUntilDone branch.

[Adam Barth]

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]

(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 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]

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]

Ok.  Sounds like we should use postTask instead.

[Terry Anderson]

Created attachment 149969 [details]
Patch

[Adam Barth]

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]

(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 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 on attachment 149969 [details]
Patch

Argh, sorry.  Restoring r+ and cq+.

[Adam Barth]

> 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 on attachment 149969 [details]
Patch

Clearing flags on attachment: 149969

Committed r121453: <http://trac.webkit.org/changeset/121453>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

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]

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]

(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