Closed Bug 870232 Opened 7 years ago Closed 6 years ago

Implement getContext("webgl") for Desktop FF

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Ms2ger, Assigned: jgilbert)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Apparently we don't support that?
We can support this on desktop, but not on mobile or b2g yet.
Why?
The short answer is we don't pass conformance for WebGL on these platforms.
The corresponding WebKit (now Blink) bug also enabled getContext("webgl") only on desktops.
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.
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'.
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'.
Assignee: nobody → jgilbert
Attachment #759519 - Flags: review?(bjacob)
(For the desktop product only)
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.
Attachment #759519 - Flags: review?(bjacob) → review-
(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.
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.
Attachment #760011 - Flags: feedback?(bjacob)
Attachment #760011 - Flags: feedback?(bgirard)
Depends on: 880904
Attachment #759519 - Flags: review- → review?(bjacob)
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.
Attachment #760011 - Flags: feedback?(bjacob) → feedback-
Comment on attachment 759519 [details] [diff] [review]
patch: Enable requesting 'webgl' for our desktop product only

r+ from bjacob over IRC.
Attachment #759519 - Flags: review?(bjacob) → review+
Attachment #760011 - Attachment description: patch: Test to make sure our platform defines are sane. → test patch: Test to make sure our platform defines are sane.
Summary: Implement getContext("webgl") → Implement getContext("webgl") for Desktop FF
https://hg.mozilla.org/mozilla-central/rev/02e3a57b61c5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 881277
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.
Attachment #760011 - Flags: feedback?(bgirard) → feedback-
Depends on: 881492
Blocks: 881997
Attachment #760011 - Attachment is obsolete: true
Depends on: 912606
You need to log in before you can comment on or make changes to this bug.