Closed
Bug 648104
Opened 13 years ago
Closed 13 years ago
reftest failures on desktop - can't drawWindow remote content fails
Categories
(Core Graveyard :: Tracking, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla8
People
(Reporter: dougt, Assigned: blassey)
References
Details
Attachments
(1 file, 1 obsolete file)
3.79 KB,
patch
|
jmaher
:
review+
cjones
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•13 years ago
|
Attachment #524253 -
Flags: review?(jmaher)
Comment 1•13 years ago
|
||
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•13 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•13 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?
Reporter | ||
Updated•13 years ago
|
Assignee: doug.turner → nobody
Component: General → Tracking
OS: Mac OS X → All
QA Contact: general → chofmann
Hardware: x86 → All
Comment 6•13 years ago
|
||
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
Comment 8•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 11•13 years ago
|
||
Assignee: nobody → blassey.bugs
Attachment #524253 -
Attachment is obsolete: true
Attachment #544850 -
Flags: review?(jones.chris.g)
Attachment #544850 -
Flags: review?(jmaher)
Comment 12•13 years ago
|
||
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+
Assignee | ||
Comment 14•13 years ago
|
||
pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/afa0db2e1985
Whiteboard: [inbound]
Comment 15•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/afa0db2e1985
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•