Closed Bug 881997 Opened 6 years ago Closed 6 years ago

Use a confvar for toggling webgl conformance status at compile time

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(3 files)

The fix to 870232 works, but isn't a good way to do this.

Instead, we should have a conf var that we set for the products which have reached conformance.
Attachment #761256 - Flags: review?(bjacob)
Attachment #761256 - Flags: review?(ted)
Comment on attachment 761256 [details] [diff] [review]
patch: Use a confvar.

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

Technically this is fine. I'm still weirded out by the fact that we consider this a per-*product* decision. So if you build Firefox for a platform that's not officially supported (like, say, OpenBSD), we'd still consider WebGL conformant? That doesn't really make sense.
Attachment #761256 - Flags: review?(ted) → review+
Assignee: nobody → jgilbert
Comment on attachment 761256 [details] [diff] [review]
patch: Use a confvar.

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

::: configure.in
@@ +5901,5 @@
>  fi
>  
> +if test -n "$MOZ_WEBGL_CONFORMANT"; then
> +  AC_DEFINE(MOZ_WEBGL_CONFORMANT)
> +fi

Is that all? Other confvars seem to have some more code:

dnl ========================================================
dnl = Enable getUserMedia support
dnl ========================================================
MOZ_ARG_ENABLE_BOOL(media-navigator,
[  --enable-media-navigator  Enable support for getUserMedia],
    MOZ_MEDIA_NAVIGATOR=1,
    MOZ_MEDIA_NAVIGATOR=)

if test -n "$MOZ_MEDIA_NAVIGATOR"; then
  AC_DEFINE(MOZ_MEDIA_NAVIGATOR)
fi


I'm only r+ing nsLayoutModule, letting Ted handle the build system changes.
Attachment #761256 - Flags: review?(bjacob) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> Comment on attachment 761256 [details] [diff] [review]
> patch: Use a confvar.
> 
> Review of attachment 761256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Technically this is fine. I'm still weirded out by the fact that we consider
> this a per-*product* decision. So if you build Firefox for a platform that's
> not officially supported (like, say, OpenBSD), we'd still consider WebGL
> conformant? That doesn't really make sense.

If a platform is not officially supported, then in particular we don't bear responsibility for our code correctly reporting its conformance status there ;-)

For better or worse, there are formal rules about when a browser can claim to be WebGL conformant and expose the "webgl" context ID as opposed to "experimental-webgl", see
  https://www.khronos.org/registry/webgl/sdk/tests/CONFORMANCE_RULES.txt
What happens if we start shipping on a new platform (say Windows x64)? Are we automatically conformant there because we're conformant on our other platforms?
Short answer: yes, because we are conformant on Win32.

On a theoretical level: the process doesn't distinguish between 32bit and 64bit versions of the same OS, or application.

On a practical level: we have rarely seen 32bit vs 64bit make any impact on whether a browser passes a conformance test.

By far the most meaningful factors here are the browser engine, the GPU driver and the OS (but again 32bit vs 64bit rarely makes a difference).

The process requires us to pass on 2 different drivers on each OS that our product support. Because of this "all OSes we support" rule, we insist that FennecAndroid is a different product than desktop Firefox, so that we're not forced to wait for mobile GPU drivers to improve before we can at least claim conformance on the desktop. Of course, FennecAndroid is very much a different product anyway (Java vs XUL, etc).
Some try runs for a mochitest to test this. (but first we need to test the test!)
https://tbpl.mozilla.org/?tree=Try&rev=e07ec1f29642
https://tbpl.mozilla.org/?tree=Try&rev=5bf5721fc99e
OS: Linux → All
Hardware: x86_64 → All
Attached patch patchSplinter Review
I need a reviewer, but bjacob is on PTO. dzbarsky is the lucky winner of the 'made the mistake of touching WebGL code once' raffle.

This passes currectly without this bug's fix patch, and we should expect it to work afterwards, too.
https://tbpl.mozilla.org/?tree=Try&rev=d54fccbd3f81
Attachment #771110 - Flags: review?(dzbarsky)
Comment on attachment 771110 [details] [diff] [review]
patch

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

LGMT. I knew touching WebGL would come back to haunt me...
Attachment #771110 - Flags: review?(dzbarsky) → review+
Comment on attachment 771110 [details] [diff] [review]
patch

::: content/canvas/test/webgl/non-conf-tests/test_webgl_conformance.html
>+var isAndroid = /Android/.test(navigator.userAgent);
>+var shouldBeConformant = !isAndroid;

This test should exclude not only Android but also B2G.
Only one half was backed out, failures were still occurring.

Backed out the rest:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e1eea117b2b0
Attached patch patch: testcaseSplinter Review
Updated testcase to use 'Mobile' and 'Tablet' instead of 'Android', as some b2g slaves don't claim to be 'Android'.
Attachment #774256 - Flags: review+
(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> Created attachment 774256 [details] [diff] [review]
> patch: testcase
> 
> Updated testcase to use 'Mobile' and 'Tablet' instead of 'Android', as some
> b2g slaves don't claim to be 'Android'.

Apparently, b2g just says 'Mobile' and not 'Android' after bug 777710.
(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> (In reply to Jeff Gilbert [:jgilbert] from comment #13)
> > Created attachment 774256 [details] [diff] [review]
> > patch: testcase
> > 
> > Updated testcase to use 'Mobile' and 'Tablet' instead of 'Android', as some
> > b2g slaves don't claim to be 'Android'.
> 
> Apparently, b2g just says 'Mobile' and not 'Android' after bug 777710.

Shoot, I should have caught that.  I spent a few days last year debugging a failure that was due to us no longer being served mobile content.
https://hg.mozilla.org/mozilla-central/rev/55469eb2cfa6
https://hg.mozilla.org/mozilla-central/rev/f39c97b3dfdf
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.