Closed Bug 648104 Opened 10 years ago Closed 10 years ago

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


(Core Graveyard :: Tracking, defect)

Other Branch
Not set


(Not tracked)



(Reporter: dougt, Assigned: blassey)




(1 file, 1 obsolete file)

Attached patch patch v.1 (obsolete) — Splinter Review
No description provided.
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+
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-
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?
Duplicate of this bug: 660923
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?
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.
Attached patch patch v.2Splinter Review
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/
@@ +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+
Closed: 10 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.