Last Comment Bug 870232 - Implement getContext("webgl") for Desktop FF
: Implement getContext("webgl") for Desktop FF
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
Depends on: 880904 881492 912606
Blocks: 881277 881997
  Show dependency treegraph
 
Reported: 2013-05-09 00:24 PDT by :Ms2ger
Modified: 2013-09-05 12:05 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch: Enable requesting 'webgl' for our desktop product only (1.73 KB, patch)
2013-06-06 17:37 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Review
test patch: Test to make sure our platform defines are sane. (2.13 KB, patch)
2013-06-07 16:47 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: feedback-
bgirard: feedback-
Details | Diff | Review

Description :Ms2ger 2013-05-09 00:24:37 PDT
Apparently we don't support that?
Comment 1 Jeff Gilbert [:jgilbert] 2013-06-04 17:37:20 PDT
We can support this on desktop, but not on mobile or b2g yet.
Comment 2 :Ms2ger 2013-06-04 23:55:13 PDT
Why?
Comment 3 Jeff Gilbert [:jgilbert] 2013-06-05 00:51:29 PDT
The short answer is we don't pass conformance for WebGL on these platforms.
Comment 4 Masatoshi Kimura [:emk] 2013-06-05 01:45:34 PDT
The corresponding WebKit (now Blink) bug also enabled getContext("webgl") only on desktops.
Comment 5 :Ms2ger 2013-06-05 02:43:31 PDT
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 Jeff Gilbert [:jgilbert] 2013-06-05 10:28:55 PDT
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 Jeff Gilbert [:jgilbert] 2013-06-06 17:37:13 PDT
Created attachment 759519 [details] [diff] [review]
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 Jeff Gilbert [:jgilbert] 2013-06-06 17:37:39 PDT
(For the desktop product only)
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2013-06-06 18:39:57 PDT
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 Jeff Gilbert [:jgilbert] 2013-06-06 19:08:24 PDT
(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 Jeff Gilbert [:jgilbert] 2013-06-07 16:47:21 PDT
Created attachment 760011 [details] [diff] [review]
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) 2013-06-07 16:54:38 PDT
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 Jeff Gilbert [:jgilbert] 2013-06-07 17:42:01 PDT
Comment on attachment 759519 [details] [diff] [review]
patch: Enable requesting 'webgl' for our desktop product only

r+ from bjacob over IRC.
Comment 15 Ed Morley [:emorley] 2013-06-10 02:12:49 PDT
https://hg.mozilla.org/mozilla-central/rev/02e3a57b61c5
Comment 16 Benoit Girard (:BenWa) 2013-06-10 08:45:13 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.