870232 - Implement getContext("webgl") for Desktop FF

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Apparently we don't support that?

Comment 1

[Kelsey Gilbert [:jgilbert]]

We can support this on desktop, but not on mobile or b2g yet.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Why?

Comment 3

[Kelsey Gilbert [:jgilbert]]

The short answer is we don't pass conformance for WebGL on these platforms.

Comment 4

[Masatoshi Kimura [:emk]]

The corresponding WebKit (now Blink) bug also enabled getContext("webgl") only on desktops.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We don't pass all tests for the 2d context either. I don't think that's sufficient reason to make it harder to use.

Comment 6

[Kelsey Gilbert [:jgilbert]]

It's a spec conformance issue. Since we don't pass the tests on the requisite number of machines for our non-desktop products, we are not allowed to claim conformance. We don't have a choice in the matter.

This doesn't make it 'harder to use', either. Every tutorial either uses 'experimental-webgl' or falls back to it. There is practically no reason for an app to only support 'webgl'.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Enable requesting 'webgl' for our desktop product only

We have the requisite conformance submissions to claim conformance to WebGL 1.0.1, and as such can expose contexts from both 'webgl' and 'experimental-webgl'.

Comment 8

[Kelsey Gilbert [:jgilbert]]

(For the desktop product only)

Comment 9

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

Comment on attachment 759519 [details] [diff] [review]
patch: Enable requesting 'webgl' for our desktop product only

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

::: layout/build/nsLayoutModule.cpp
@@ +1173,5 @@
>    { "@mozilla.org/content/element/html;1?name=img", &kNS_HTMLIMAGEELEMENT_CID },
>    { "@mozilla.org/content/element/html;1?name=option", &kNS_HTMLOPTIONELEMENT_CID },
>    { "@mozilla.org/content/canvas-rendering-context;1?id=moz-webgl", &kNS_CANVASRENDERINGCONTEXTWEBGL_CID },
>    { "@mozilla.org/content/canvas-rendering-context;1?id=experimental-webgl", &kNS_CANVASRENDERINGCONTEXTWEBGL_CID },
> +#ifdef MOZ_PHOENIX // Not MOZ_FENNEC or MOZ_B2G yet.

Woohoo, I didn't know that we still had a MOZ_PHOENIX. But looking at configure.in, it seems that MOZ_PHOENIX is defined on Android:

case "$MOZ_BUILD_APP" in
browser)
  AC_DEFINE(MOZ_PHOENIX)
  ;;

You can use just ANDROID to detect both Android and B2G, I think.

Comment 10

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #9)
> Comment on attachment 759519 [details] [diff] [review]
> patch: Enable requesting 'webgl' for our desktop product only
> 
> Review of attachment 759519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/build/nsLayoutModule.cpp
> @@ +1173,5 @@
> >    { "@mozilla.org/content/element/html;1?name=img", &kNS_HTMLIMAGEELEMENT_CID },
> >    { "@mozilla.org/content/element/html;1?name=option", &kNS_HTMLOPTIONELEMENT_CID },
> >    { "@mozilla.org/content/canvas-rendering-context;1?id=moz-webgl", &kNS_CANVASRENDERINGCONTEXTWEBGL_CID },
> >    { "@mozilla.org/content/canvas-rendering-context;1?id=experimental-webgl", &kNS_CANVASRENDERINGCONTEXTWEBGL_CID },
> > +#ifdef MOZ_PHOENIX // Not MOZ_FENNEC or MOZ_B2G yet.
> 
> Woohoo, I didn't know that we still had a MOZ_PHOENIX. But looking at
> configure.in, it seems that MOZ_PHOENIX is defined on Android:
> 
> case "$MOZ_BUILD_APP" in
> browser)
>   AC_DEFINE(MOZ_PHOENIX)
>   ;;
> 
> You can use just ANDROID to detect both Android and B2G, I think.

I want to do this correctly, by product. MOZ_FENNEC and MOZ_B2G should work, but it'd be best if I can say what I mean, so I can mean what I say.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: test patch: Test to make sure our platform defines are sane.

We shouldn't add these tests here, but rather add them elsewhere in an actual test file. Maybe BenWa has a better idea of where to put this.

bjacob: This shows that MOZ_PHOENIX is not defined on android (which I have tested manually) or b2g.

Comment 12

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

Comment on attachment 760011 [details] [diff] [review]
test patch: Test to make sure our platform defines are sane.

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

Thanks for checking this, but I don't think that we want to put tests like this in code like that. You could do a separate gtest, and yes BenWa is the right person to ask about that.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 759519 [details] [diff] [review]
patch: Enable requesting 'webgl' for our desktop product only

r+ from bjacob over IRC.

Comment 14

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02e3a57b61c5

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/02e3a57b61c5

Comment 16

[Benoit Girard (:BenWa)]

Comment on attachment 760011 [details] [diff] [review]
test patch: Test to make sure our platform defines are sane.

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

IMO this patch is indicative of code smell.

::: layout/build/nsLayoutModule.cpp
@@ +1173,5 @@
>    { "@mozilla.org/content/element/html;1?name=img", &kNS_HTMLIMAGEELEMENT_CID },
>    { "@mozilla.org/content/element/html;1?name=option", &kNS_HTMLOPTIONELEMENT_CID },
>    { "@mozilla.org/content/canvas-rendering-context;1?id=moz-webgl", &kNS_CANVASRENDERINGCONTEXTWEBGL_CID },
>    { "@mozilla.org/content/canvas-rendering-context;1?id=experimental-webgl", &kNS_CANVASRENDERINGCONTEXTWEBGL_CID },
> +#if defined(MOZ_FENNEC) != defined(MOZ_WIDGET_ANDROID)

I don't think it's up to a webgl bug to decide that fennec <=> widget_android and b2g <=> gonk. Your patch probably shouldn't make these assumptions. If so I'd suggest getting review from these modules to introduce a check like this.

@@ +1187,3 @@
>  #ifdef MOZ_PHOENIX // Not MOZ_FENNEC or MOZ_B2G yet.
> +#if defined(ANDROID) || defined(MOZ_FENNEC) || defined(MOZ_B2G)
> +#error But MOZ_PHOENIX should only be defined for the desktop product!

MOZ_PHOENIX should be considered deprecated. Even if it happens to be defined to what you need doesn't mean it's ok to use it. If you need a define to do something I'd suggest introducing one (renaming MOZ_PHOENIX would work too) and getting a proper review.

Comment 17

[Jean-Yves Perrier [:teoli]]

Documented: https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/24