reftest failures on desktop - can't drawWindow remote content fails

RESOLVED FIXED in mozilla8

Status

Core Graveyard
Tracking
RESOLVED FIXED
6 years ago
11 months ago

People

(Reporter: dougt, Assigned: blassey)

Tracking

Other Branch
mozilla8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 524253 [details] [diff] [review]
patch v.1
(Reporter)

Updated

6 years ago
Attachment #524253 - Flags: review?(jmaher)
Comment on attachment 524253 [details] [diff] [review]
patch v.1

this patch is simple and fine.  I don't understand all the requirements that existed before, so if cjones or somebody else with better knowledge of layers and the canvas, I would appreciate their feedback.

What i do know is from a  testing perspective this will get us green on many reftests.
Attachment #524253 - Flags: review?(jmaher) → review+
(Reporter)

Comment 2

6 years ago
Comment on attachment 524253 [details] [diff] [review]
patch v.1

chris, this is the patch I was talking about that unconditionally passes the DRAWWINDOW_USE_WIDGET_LAYERS to DrawWindow if we are using a browser that is remote.
Attachment #524253 - Flags: review?(jones.chris.g)
Comment on attachment 524253 [details] [diff] [review]
patch v.1

No, this patch is wrong.  If the window isn't sized properly, we won't be comparing the rendered content properly and tests can spuriously pass.

However, I'm OK with adding this behavior as a non-default flag that developers can use.
Attachment #524253 - Flags: review?(jones.chris.g) → review-
(Reporter)

Comment 4

6 years ago
cjones, in general, i think we always want to use the DRAWWINDOW_USE_WIDGET_LAYERS flag when gBrowserIsRemote is true.  Is that your understanding as well?

Updated

6 years ago
Duplicate of this bug: 660923
(Reporter)

Updated

6 years ago
Assignee: doug.turner → nobody
Component: General → Tracking
OS: Mac OS X → All
QA Contact: general → chofmann
Hardware: x86 → All
The attached patch also breaks the !gBrowserIsRemote case to never use DRAWWINDOW_USE_WIDGET_LAYERS, right?

Comment 7

6 years ago
Is there no way for us to get around this insane resolution requirement on mobile reftests?  What's the use of adding this as a non-default flag?  A developer's local run would then pass the reftest, then when they check in.  Next, the automation runs the test at the proper resolution and the test would fail.

We really need to move away from requiring the giant resolution size to run remoted (e10s enabled) reftests.

What are the next steps on this bug?
Version: unspecified → Other Branch
If I am not mistaken, we require a large resolution so the entire reftest is completely visible in the <browser> element. The DRAWWINDOW_USE_WIDGET_LAYERS magic can only render what the IPC <browser> has visible. Any off-screen parts are not rendered to the canvas, causing the test to fail.

There is another canvas rendering API that can be used for IPC web content. ctx.drawAsyncXULElement will render the entire web content of a <browser> into a canvas, even the parts that are off screen.

The up-side is the reftests can be run on any resolution. The down-side is the IPC magic used to share the web content from the child process to the parent process is skipped and not part of the test.

We also run IPC shared image reftests on desktops, so we would not totally be without coverage.
In addition to the reftest failure Joel and Doug are working to fix, the current high resolution can cause OOM failures too. Also, setting the resolution can add 15-20 minutes to a test run and it affects all tests, not just reftests.
cjones, can we get this bug fixed this week?  It would really free up a LOT of resources and allow developers to run tests easier on phones.

Updated

6 years ago
Blocks: 669883
Created attachment 544850 [details] [diff] [review]
patch v.2
Assignee: nobody → blassey.bugs
Attachment #524253 - Attachment is obsolete: true
Attachment #544850 - Flags: review?(jones.chris.g)
Attachment #544850 - Flags: review?(jmaher)
Comment on attachment 544850 [details] [diff] [review]
patch v.2

Review of attachment 544850 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the one nit fixed.

::: layout/tools/reftest/runreftest.py
@@ +93,1 @@
>  

ignoreWidnowSize <- typo.

Also this should be:
if options.ignoreWindowSize != False  <- that is the default set below
Attachment #544850 - Flags: review?(jmaher) → review+
Comment on attachment 544850 [details] [diff] [review]
patch v.2

>+    if options.ignoreWindowSize == False:

|if not options.ignoreWindowSize:|

>+        if (width < 1050 or height < 1050):
>+            print "ERROR: Invalid screen resolution %sx%s, please adjust to 1366x1050 or higher" % (width, height)

The reftest window needs to be 800x1000, but I guess the screen
resolution needs to be higher because of status bar etc?

>+    if options.ignoreWindowSize != None:

More python-y is |if options.ignoreWindowSize is not None:|, but the
surrounding code doesn't use that.

>+    self.add_option("--ignore-window-size",
>+                    dest = "ignoreWindowSize", action = "store_true",
>+                    help = "ignore the window size, which may cause spuriously failures")

"may cause spurious failures and passes"
Attachment #544850 - Flags: review?(jones.chris.g) → review+
pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/afa0db2e1985
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/afa0db2e1985
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Depends on: 672704
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.