Implement getContext("webgl") for Desktop FF

RESOLVED FIXED in mozilla24

Status

()

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

People

(Reporter: Ms2ger, Assigned: jgilbert)

Tracking

({dev-doc-complete})

Trunk
mozilla24
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Apparently we don't support that?
(Assignee)

Comment 1

4 years ago
We can support this on desktop, but not on mobile or b2g yet.
(Reporter)

Comment 2

4 years ago
Why?
(Assignee)

Comment 3

4 years ago
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.
(Reporter)

Comment 5

4 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.
(Assignee)

Comment 6

4 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'.
(Assignee)

Comment 7

4 years ago
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'.
Assignee: nobody → jgilbert
Attachment #759519 - Flags: review?(bjacob)
(Assignee)

Comment 8

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

Comment 10

4 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.
(Assignee)

Comment 11

4 years ago
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.
Attachment #760011 - Flags: feedback?(bjacob)
Attachment #760011 - Flags: feedback?(bgirard)
(Assignee)

Updated

4 years ago
Depends on: 880904
(Assignee)

Updated

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

Comment 13

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

Updated

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

Updated

4 years ago
Summary: Implement getContext("webgl") → Implement getContext("webgl") for Desktop FF
(Assignee)

Comment 14

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/02e3a57b61c5
https://hg.mozilla.org/mozilla-central/rev/02e3a57b61c5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Reporter)

Updated

4 years ago
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-

Updated

4 years ago
Depends on: 881492
(Assignee)

Updated

4 years ago
Blocks: 881997
Keywords: dev-doc-needed
(Assignee)

Updated

4 years ago
Attachment #760011 - Attachment is obsolete: true
Documented: https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/24
Keywords: dev-doc-needed → dev-doc-complete

Updated

4 years ago
Depends on: 912606
You need to log in before you can comment on or make changes to this bug.