1377335 - Re-enable DRAWWINDOW_DRAW_VIEW flag for Marionette screen captures

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, enhancement, P3)

Description

[Andreas Tolfsen ❲:ato❳]

As noted in a TODO in testing/marionette/capture.js, we have disabled two flags on the canvas context when taking screenshots:

>   let ctx = canvas.getContext(CONTEXT_2D);
>   if (flags === null) {
>     flags = ctx.DRAWWINDOW_DRAW_CARET;
>     // Disabled in bug 1243415 for webplatform-test
>     // failures due to out of view elements.  Needs
>     // https://github.com/w3c/web-platform-tests/issues/4383 fixed.
>     /*
>     ctx.DRAWWINDOW_DRAW_VIEW;
>     */
>     // Bug 1009762 - Crash in [@ mozilla::gl::ReadPixelsIntoDataSurface]
>     /*
>     ctx.DRAWWINDOW_USE_WIDGET_LAYERS;
>     */
>   }

We want to investigate re-enabling these.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You could try to test in a try push but the underlying crash is not fixed yet, and as such it should still appear.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Note that with the patch on bug 1559592 landed this bug will become invalid.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Actually let me fix that bug first, which will help me with the refactoring on bug 1559592 afterward.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Note that both flags can be re-enabled now given that all dependencies have been fixed, or are no longer reproducible.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0acaea225f5ddf4dddb426b42b6fab4055d33483

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1377335 - [marionette] Re-enable DRAWWINDOW_DRAW_VIEW and DRAWWINDOW_USE_WIDGET_LAYERS flags for screen captures.

All dependencies are fixed, or no longer reproducible. As such both
flags can be re-enabled. Also reftests won't have to specify the
exactly same flags on its own anymore.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dab6e3dc6ed
[marionette] Re-enable DRAWWINDOW_DRAW_VIEW and DRAWWINDOW_USE_WIDGET_LAYERS flags for screen captures. r=webdriver-reviewers,ato

Comment 8

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/4dab6e3dc6ed

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I would suggest to backout this patch given the fact that doing full screenshots are not working with the DRAWWINDOW_DRAW_VIEW flag not being present. There is a strange behavior change in combination with DRAWWINDOW_USE_WIDGET_LAYERS maybe. And given that we shortly change the API to drawSnapshot() we could try to get it working properly there again.

Comment 10

[Cristina Coroiu [:ccoroiu]]

Backout: https://hg.mozilla.org/integration/autoland/rev/8fc7373e435e88815954316cdba18b3f91b87fa6

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We will have to wait for bug 1571341 before we can try to enable the flags again.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The new API only got the DRAWWINDOW_DRAW_VIEW flag. Using the widget one isn't necessary because drawSnapshot doesn't readback the data from the compositor.

Given that bug 1559592 hasn't been finished yet, and I don't want to cause a regression with its landing, I will most likely have to take care of it now. So lets mark this bug dependent on it instead.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Actually by moving to the new snapshot API we no longer need that flag. When only the viewport has to be captured the correct absolute coordinates will have to be specified including the x and y scroll position. It's way more flexible now.