881997 - Use a confvar for toggling webgl conformance status at compile time

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Use a confvar.

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.

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(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

Comment 4

[(not currently active) Ted Mielczarek]

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?

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

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).

Comment 6

[Kelsey Gilbert [:jgilbert]]

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

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

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

Comment 8

[David Zbarsky (: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...

Comment 9

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08c4cf801fbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbea533f8927

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0dc84a9f2910 Backed out for causing conformance test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=25101769&tree=Mozilla-Inbound

Comment 11

[Masatoshi Kimura [:emk]]

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.

Comment 12

[Ed Morley [:emorley]]

Only one half was backed out, failures were still occurring.

Backed out the rest:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e1eea117b2b0

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: testcase

Updated testcase to use 'Mobile' and 'Tablet' instead of 'Android', as some b2g slaves don't claim to be 'Android'.

Comment 14

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55469eb2cfa6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f39c97b3dfdf

Comment 15

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 16

[David Zbarsky (:dzbarsky)]

(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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55469eb2cfa6
https://hg.mozilla.org/mozilla-central/rev/f39c97b3dfdf