Last Comment Bug 648104 - reftest failures on desktop - can't drawWindow remote content fails
: reftest failures on desktop - can't drawWindow remote content fails
Product: Core Graveyard
Classification: Graveyard
Component: Tracking (show other bugs)
: Other Branch
: All All
-- normal (vote)
: mozilla8
Assigned To: Brad Lassey [:blassey] (use needinfo?)
: chris hofmann
: 660923 (view as bug list)
Depends on: 672704
Blocks: 669883
  Show dependency treegraph
Reported: 2011-04-06 13:46 PDT by Doug Turner (:dougt)
Modified: 2016-07-15 12:13 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

patch v.1 (2.24 KB, patch)
2011-04-06 13:46 PDT, Doug Turner (:dougt)
jmaher: review+
cjones.bugs: review-
Details | Diff | Splinter Review
patch v.2 (3.79 KB, patch)
2011-07-08 10:33 PDT, Brad Lassey [:blassey] (use needinfo?)
jmaher: review+
cjones.bugs: review+
Details | Diff | Splinter Review

Description User image Doug Turner (:dougt) 2011-04-06 13:46:20 PDT
Created attachment 524253 [details] [diff] [review]
patch v.1
Comment 1 User image Joel Maher ( :jmaher) 2011-04-12 17:45:52 PDT
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.
Comment 2 User image Doug Turner (:dougt) 2011-04-14 12:28:21 PDT
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.
Comment 3 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 13:54:12 PDT
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.
Comment 4 User image Doug Turner (:dougt) 2011-04-21 10:29:31 PDT
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?
Comment 5 User image cmtalbert 2011-06-06 12:01:11 PDT
*** Bug 660923 has been marked as a duplicate of this bug. ***
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-15 18:27:26 PDT
The attached patch also breaks the !gBrowserIsRemote case to never use DRAWWINDOW_USE_WIDGET_LAYERS, right?
Comment 7 User image cmtalbert 2011-06-30 15:42:34 PDT
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?
Comment 8 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-07-01 11:52:06 PDT
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.
Comment 9 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-07-01 11:55:05 PDT
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.
Comment 10 User image Joel Maher ( :jmaher) 2011-07-05 12:47:51 PDT
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.
Comment 11 User image Brad Lassey [:blassey] (use needinfo?) 2011-07-08 10:33:58 PDT
Created attachment 544850 [details] [diff] [review]
patch v.2
Comment 12 User image Joel Maher ( :jmaher) 2011-07-08 10:53:19 PDT
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
Comment 13 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-12 10:15:08 PDT
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"
Comment 14 User image Brad Lassey [:blassey] (use needinfo?) 2011-07-12 15:13:01 PDT

Note You need to log in before you can comment on or make changes to this bug.