Closed Bug 1377335 Opened 7 years ago Closed 5 years ago

Re-enable DRAWWINDOW_DRAW_VIEW flag for Marionette screen captures

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Version 3
enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ato, Unassigned)

References

Details

Attachments

(1 obsolete file)

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.
You could try to test in a try push but the underlying crash is not fixed yet, and as such it should still appear.
Priority: -- → P3

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

Depends on: 1559592

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

Assignee: nobody → hskupin
Blocks: 1559592
Status: NEW → ASSIGNED
No longer depends on: 1559592
Priority: P3 → P1

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

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.

Pushed by hskupin@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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.

Flags: needinfo?(sheriffs)
Status: RESOLVED → REOPENED
Flags: needinfo?(sheriffs) → needinfo?(hskupin)
Resolution: FIXED → ---
Target Milestone: mozilla70 → ---
No longer regressions: 1571659

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

Assignee: hskupin → nobody
Status: REOPENED → NEW
Depends on: 1571341
Flags: needinfo?(hskupin)
Priority: P1 → P3

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.

No longer blocks: 1559592
Depends on: 1559592
Summary: Re-enable DRAWWINDOW_DRAW_VIEW and DRAWWINDOW_USE_WIDGET_LAYERS flags for Marionette screen captures → Re-enable DRAWWINDOW_DRAW_VIEW flag for Marionette screen captures

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.

Status: NEW → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → WONTFIX
Attachment #9082566 - Attachment is obsolete: true
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: