Use a confvar for toggling webgl conformance status at compile time

RESOLVED FIXED in mozilla25

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla25
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
Created attachment 761256 [details] [diff] [review]
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.
Attachment #761256 - Flags: review?(bjacob)
(Assignee)

Updated

5 years ago
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).
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 7

5 years ago
Created attachment 771110 [details] [diff] [review]
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
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
(Assignee)

Comment 13

5 years ago
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'.
Attachment #774256 - Flags: review+
(Assignee)

Comment 15

5 years ago
(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
Last Resolved: 5 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.