Bug 212647

<rdar://problem/63882960>

This test appears to have started failing with the changes in r262366 also: webgl/webgl2-primitive-restart.html

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=webgl%2Fwebgl-backing-store-size-update.html&test=webgl%2Fwebgl2-primitive-restart.html

This test:

- starts with a canvas that has a composited size of 100x100
- sets the canvas drawing buffer size to 200x200
- renders into the canvas
- changes the composited size of the canvas to 50x50
- waits for a layout pass
- sets the canvas drawing buffer size to 50x50
- renders into the canvas

I'm not sure why the issues are specific to WK1.

For WK1, at least in the test harness, it appears to be possible for display to be called *before* prepare.

Notice that the "canvas is displayed" happens earlier.

Good case
---------

canvas size set to 200x200
GCGL reshape to 200x200
Paint happens - document takes note

canvas size set to 100x200 (via setting just .width)
GCGL reshape to 100x200

canvas size set to 100x100 (via setting just .height)
GCGL reshape to 100x100

Paint happens - document takes note

All JS finishes, Document looks for canvas elements needing prep
Document calls prepare on canvas

canvas is displayed

Bad case
---------

canvas size set to 200x200
GCGL reshape to 200x200
Paint happens - document takes note

canvas size set to 100x200 (via setting just .width)
GCGL reshape to 100x200

canvas size set to 100x100 (via setting just .height)
GCGL reshape to 100x100

Paint happens - document takes note

canvas is displayed

All JS finishes, Document looks for canvas elements needing prep
Document calls prepare on canvas

So either the logic of doing the preparation in Page::doAfterUpdateRendering() is incorrect for WebKit1, or something about the canvas backbuffer sizing causes a display to trigger early.

Calling setNeedsDisplay from prepare would probably fix this particular test, but would still be two paints - we need the prepare to happen before the paint.

Maybe it is the fact that the resize and paint happens inside a rAF?

It's the test app that triggers the issue:

dino> display 0x10d7c06b8
1   0x11ef0ad84 -[WebGLLayer display]
2   0x108da1224 CA::Layer::display_if_needed(CA::Transaction*)
3   0x108ed35b5 CA::Context::commit_transaction(CA::Transaction*, double, double*)
4   0x108d7ec59 CA::Transaction::commit()
5   0x10b3d5772 -[WebView(WebPrivate) _forceRepaintForTesting]
6   0x1013a2d87 updateDisplay()
7   0x1013a2753 dump()
8   0x10140e892 TestRunner::notifyDone()

TestRunner forces a repaint.

The backing store test fix is easy.

Now for the primitive restart issue....

Same problem. Rather than fixing the tests I should probably call the document's canvas preparation stuff from _forceRepaintForTesting

I was hoping I could just call Page::updateRendering() inside the forceRepaint, but that doesn't appear to help - display is still called before prepare. I'll work out why this is.

Ah. _forceRepaintForTesting already triggers the Page::updateRendering code to run, but it exits early because it is already updating the rendering.

updateRendering calls document.serviceRequestAnimationFrameCallbacks(timestamp) and this particular test is triggering _forceRepaintForTesting from within the callback. 

I could try calling Page::doAfterUpdateRendering() from within _forceRepaintForTesting. Under normal circumstances it would have run through whatever triggered updateRendering.

However, rendering definitely hasn't been updated by this point, so it could be a mistake.

All this is to avoid having to edit the tests, but it would be more accurate.

Created attachment 401108 [details]
Patch

Comment on attachment 401108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401108&action=review

Looks good to me - one small comment.

> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:223
> +    [self setNeedsDisplay];

Is this call cheap? If not, should it be done only if _preparedForDisplay == NO?

Comment on attachment 401108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401108&action=review

>> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:223
>> +    [self setNeedsDisplay];
> 
> Is this call cheap? If not, should it be done only if _preparedForDisplay == NO?

This is probably the most concerning part of the patch. We really shouldn't have to do this, and it is only necessary because the test infrastructure can bypass the normal rendering cycle. Changing that might impact a lot of tests across all features, but probably should be done anyway.

However, my understanding is that this call is cheap. It's just setting a bit that will be detected in CoreAnimation's rendering loop that will cause it to execute `display`. Technically we shouldn't have even got into this method without it already being called, since the same logic that tells the system that this method is necessary also calls setNeedsDisplay itself. The problem is that the test infrastructure can trigger a `display` before the code in Page that would make preparation happen.

Committed r262643: <https://trac.webkit.org/changeset/262643>

Comment on attachment 401108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401108&action=review

> Source/WebCore/dom/Document.cpp:8597
> +        auto protectedCanvas = makeRefPtr(canvas);

I would have done makeRef(*canvas)

> LayoutTests/webgl/webgl-backing-store-size-update.html:35
> +            window.requestAnimationFrame(function() {

This "window." is not needed and in future I would suggest leaving it out.

Follow-up in r262652.