:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 11 years ago

Apparently we don't support that?

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 1 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 2 • 11 years ago

Why?

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 3 • 11 years ago

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

	Masatoshi Kimura [:emk]
	Comment 4 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 5 • 11 years ago

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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 6 • 11 years ago

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

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 7 • 11 years ago

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

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 8 • 11 years ago

(For the desktop product only)

	Benoit Jacob [:bjacob] (mostly away)
	Comment 9 • 11 years ago

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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 10 • 11 years ago

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

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 11 • 11 years ago

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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 12 • 11 years ago

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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 13 • 11 years ago

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

r+ from bjacob over IRC.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 14 • 11 years ago

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

	Ed Morley [:emorley]
	Comment 15 • 11 years ago

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

	Benoit Girard (:BenWa)
	Comment 16 • 11 years ago

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.

	Jean-Yves Perrier [:teoli]
	Comment 17 • 11 years ago

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