Closed Bug 516213 Opened 10 years ago Closed 10 years ago

Freshen WebGL implementation and enable on trunk

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: mwsteele, Assigned: mwsteele)

References

Details

Attachments

(4 files, 2 obsolete files)

Need to add constructors for arrays and align method names & types with the spec.
Attachment #400334 - Flags: review?(vladimir)
Assignee: nobody → mwsteele
Comment on attachment 400334 [details] [diff] [review]
Add constructors for arrays, update idl

Looks fine to me; want me to land on the trunk?
Attached patch additional patchSplinter Review
Additional patch.  Mainly enables WebGL by default, except for a few platforms where it explicitly disables it (but still compiles the IDL and bits, just returns errors from the xpcom constructors).

Also changes the pref to webgl.enabled_for_all_sites.
Attachment #401372 - Flags: review?(mwsteele)
Comment on attachment 401372 [details] [diff] [review]
additional patch

+# only allow on platforms/toolkits we know are good
+ifneq (,$(NS_OSSO)$(WINCE)$(filter-out windows cocoa gtk2,$(MOZ_WIDGET_TOOLKIT)))
+MOZ_ENABLE_CANVAS3D=
+endif

This belongs in configure.in. Please do a followup patch to move this to configure.in and remove MOZ_ENABLE_CANVAS3D conditionals from the IDL etc.
Attachment #401372 - Flags: review+
Attachment #401372 - Flags: review?(mwsteele) → review+
Summary: Freshen WebGL implementation → Freshen WebGL implementation and enable on trunk
Looks like the attachment labeled "additional patch" landed earlier tonight:
http://hg.mozilla.org/mozilla-central/rev/be2a05a9e4ef

After I pulled this checkin to my local Ubuntu Linux machine, I got build errors building WebGLContext.cpp.  I was able to fix the bustage by installing the "libglitz-glx1-dev" package.  Before I had that package, I get errors like this when building WebGLContext.cpp
{
In file included from /mozilla/content/canvas/src/WebGLContext.h:59,
                 from /mozilla/content/canvas/src/WebGLContext.cpp:2:
/mozilla/content/canvas/src/nsGLPbuffer.h:61:20: error: GL/glx.h: No such file or directory
In file included from /mozilla/content/canvas/src/WebGLContext.h:59,
                 from /mozilla/content/canvas/src/WebGLContext.cpp:2:
/mozilla/content/canvas/src/nsGLPbuffer.h:183: error: ISO C++ forbids declaration of ‘Display’ with no type
/mozilla/content/canvas/src/nsGLPbuffer.h:183: error: expected ‘;’ before ‘*’ token
/mozilla/content/canvas/src/nsGLPbuffer.h:184: error: ‘GLXFBConfig’ does not name a type
/mozilla/content/canvas/src/nsGLPbuffer.h:185: error: ‘GLXPbuffer’ does not name a type
/mozilla/content/canvas/src/nsGLPbuffer.h:186: error: ‘GLXContext’ does not name a type
make[2]: *** [WebGLContext.o] Error 1
}

When I first started getting these errors, I tried adding "ac_add_options --disable-webgl" to my .mozconfig as a workaround, but that gave me a different error:
{
/mozilla/content/canvas/src/WebGLContextNotSupported.cpp:39:44: error: nsICanvasRenderingContextWebGL.h: No such file or directory
In file included from /mozilla/content/canvas/src/WebGLContextNotSupported.cpp:40:
../../../dist/include/WebGLArray.h:41: error: ‘nsresult’ does not name a type
}
with 8 copies of that last line -- one for each line of WebGLContextNotSupported.cpp.

That error looks more like a bug in a Makefile or an #include ordering, rather than a library dependency...
(In reply to comment #4)
> That error looks more like a bug in a Makefile or an #include ordering, rather
> than a library dependency...

Yeah -- we only build nsICanvasRenderingContextWebGL.idl if webgl (MOZ_ENABLE_CANVAS3D) is enabled[1], and we only build WebGLContextNotSupported.cpp if webgl is disabled[2].  And WebGLContextNotSupported.cpp tries to #include nsICanvasRenderingContextWebGL.h [3], which makes us fail because the header wasn't built, since webgl is disabled.

So, I don't think we build successfully with --disable-webgl at all right now -- Vlad/Mark, can you post a followup to fix this?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/canvas/Makefile.in?mark=51-53#51
[2] http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/Makefile.in?mark=63-63,98-102#63
[3] http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContextNotSupported.cpp?mark=39-39#39
(In reply to comment #5)
> since webgl is
> disabled.
Sorry, in case that wasn't clear -- of course webgl is enabled by default now -- I meant that if we're building WebGLContextNotSupported.cpp, that means it's disabled.
Trunk doesn't compile now on my linux system.
I get the error
/usr/include/GL/glxext.h:773: error: ‘GLboolean’ has not been declared
Attached patch a patchSplinter Review
I had to add GLX_GLXEXT_LEGACY. No idea if that is the right thing to do, 
but without that I couldn't compile.
Here's a bustage fix for the compile error when building with --disable-webgl (mentioned in the second half of comment 4 & comment 5).

I've verified that this lets me compile (with --disable-webgl) on my linux box, and nieder in IRC confirmed that the same change fixes it for him on darwin.
Attachment #401517 - Flags: review?(vladimir)
Comment on attachment 401418 [details] [diff] [review]
a patch

Pushed this fix for older Linux.
Attachment #401418 - Flags: review+
Comment on attachment 401517 [details] [diff] [review]
fix for bustage with --disable-webgl

This was obsoleted by bug 517557.
Attachment #401517 - Attachment is obsolete: true
Attachment #401517 - Flags: review?(vladimir)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 517612
Blocks: 517608
Depends on: 517566
Blocks: 516858
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
Attached patch Fixed compilation on mingw. (obsolete) — Splinter Review
This broke compilation on mingw. There were two reasons:

- NO_ERROR is defined as macro in MinGW and older MSC, so its use as a identifier in nsICanvasRenderingContextWebGL.idl caused parse error. I've removed it as it's not used anywhere. An alternative solution would be to protect it by #ifndef

- mingw 3.4.6 (and probably older GCC versions) don't lice constructions like this:
nsAutoArrayPtr<int> formats = new int[numFormats];
the correct syntax is:
nsAutoArrayPtr<int> formats(new int[numFormats]);
I've changed it to regular table as the pointer doesn't change its value, so we might avoid dynamic allocation. If you think that 1KB stack use is worth avoiding, I will change the patch.
Attachment #405474 - Flags: review?(vladimir)
Comment on attachment 405474 [details] [diff] [review]
Fixed compilation on mingw.

NO_ERROR is part of the API; it's the value returned by getError and similar.  A potential workaround is to just #undef NO_ERROR.
Attachment #405474 - Flags: review?(vladimir) → review-
Blocks: 523562
Attached patch Fix using #undefSplinter Review
Thanks for the review. Updated patch is attached.
Attachment #405474 - Attachment is obsolete: true
Attachment #413943 - Flags: review?(vladimir)
Keywords: checkin-needed
What needs landing where?
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.