Closed Bug 420084 Opened 12 years ago Closed 11 years ago

reftest should use nsIXULRuntime.widgetToolkit instead of autoconf.js


(Testing :: Reftest, defect)

Not set


(Not tracked)



(Reporter: ted, Assigned: ted)




(1 file, 1 obsolete file)

I don't think reftest needs anything other than the OS for skipping tests, am I right? It should just use nsIXULRuntime.OS instead, then we could get rid of autoconf.js. I'd like to see reftest packaged as an extension at some point, and this would be a necessary step.

xr = Components.classes[";1"].getService(Components.interfaces.nsIXULRuntime);
Attached patch use nsIXULRuntime.OS instead (obsolete) — Splinter Review
This isn't fantastic, since the values don't match, so you have to map them, but it works. Using this patch, I was able to manually package a reftest extension:
Assignee: nobody → ted.mielczarek
At some point there's a pretty good chance we'll have a qt port, so we may well have tests that need to be marked failing on one of qt or gtk2.  Likewise for conditioning failures on things like some of the new accelerated gfx backends.  So we probably ought to have a way to query for a few of those things at runtime.

See also the patch in bug 448121, where I added the HTTP handler's variables.
The patch I mentioned in the previous comment is attachment 332597 [details] [diff] [review].
Component: Layout: Misc Code → Reftest
Product: Core → Testing
Version: Trunk → unspecified
So I think we probably ought to add xr.OS and xr.XPCOMABI like in attachment 332597 [details] [diff] [review], but also find a way to add the correct widget toolkit.

Then I think we should mass-change the manifests to switch to the new way, and then remove the autoconf.js stuff.
We could add a cut-down that just copies the values we need from, and either have configure generate it at configure time, or the preprocessor at build time.
Duplicate of this bug: 431457
Depends on: 475811
QA Contact: layout.misc-code → reftest
I'm adding nsIXULRuntime.widgetToolkit in bug 475811, so we can use that and not have to change any test manifests.
Summary: reftest should use nsIXULRuntime.OS instead of autoconf.js → reftest should use nsIXULRuntime.widgetToolkit instead of autoconf.js
We should probably expose OS, widgetToolkit, and XPCOMABI, since we might have things that depend on them (e.g., tests that fail in the x86-assembly version only).  And I think we probably want to do it in a way such that we do change manifiests, since MOZ_WIDGET_TOOLKIT is really the Makefile-ish way of looking at what we want.  So maybe just add widgetToolkit to what I said in comment 4?
Ok, this gets rid of autoconf.js completely, and uses the widgetToolkit property I added to nsIXULRuntime to populate MOZ_WIDGET_TOOLKIT in the sandbox. It also adds (as dbaron suggested), a xulRuntime object in the sandbox, with OS, XPCOMABI, and widgetToolkit populated directly from the xul runtime object. I've added sanity tests for xulRuntime.OS. They're not super pretty, but they were the simplest thing I could think of.
Attachment #306286 - Attachment is obsolete: true
Attachment #359524 - Flags: review?(dbaron)
Blocks: 468913
Comment on attachment 359524 [details] [diff] [review]
ditch autoconf.js, use nsIXULRuntime.widgetToolkit

r=dbaron, although I think we need to come up with something better than fails-if() for tests like this (i.e., something that doesn't count as failing for the other platforms).
Attachment #359524 - Flags: review?(dbaron) → review+
What's the impetus?
Not adding to the "known fails" total when it's actually the right result.
Ah. Should I be using skip-if instead, or do you really want something semantically better?
I want something semantically better, and where we still run the test when it's supposed to fail.
Filed bug 476642 on sorting that out.
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
This isn't going to land on 1.9.1, as it depends on the nsIXULRuntime patch, which isn't going to land on 1.9.1. It's not really necessary for the patch, it's just necessary for enabling tests on the build machines.
Depends on: 481911
Whiteboard: [needs 1.9.1 landing]
You need to log in before you can comment on or make changes to this bug.