Closed Bug 742781 Opened 12 years ago Closed 12 years ago

Implement WEBGL_debug_renderer_info extension

Categories

(Core :: Graphics, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: kbrussel, Assigned: bjacob)

References

Details

(Keywords: dev-doc-complete, Whiteboard: webgl-extension)

Attachments

(3 files, 2 obsolete files)

      No description provided.
Oops, hitting 'return' accidentally submitted the bug.

This is an RFE to please expose the "WEBGL_debug_renderer_info" WebGL extension to privileged contexts in Firefox; namely, in WebGL contexts instantiated from Firefox extensions. The MapsGL team at Google, in particular, is interested in gathering more detailed information for any problem reports, and being able to query this information is important. They are considering writing a Chrome extension for this purpose in that browser, and it would be good if the same were possible in Firefox.
Blocks: webgl-ext
Summary: Expose WEBGL_debug_renderer_info extension to privileged code (Firefox extensions) → Implement WEBGL_debug_renderer_info extension
Blocks: 742798
No longer blocks: webgl-ext
Whiteboard: webgl-extension
Blocks: 795701
Taking it; need it for bug 795701.
Assignee: nobody → bjacob
Boris for bindings aspects; Jeff for WebGL aspects.
Attachment #670582 - Flags: review?(jgilbert)
Attachment #670582 - Flags: review?(bzbarsky)
Forgot new file.
Attachment #670582 - Attachment is obsolete: true
Attachment #670582 - Flags: review?(jgilbert)
Attachment #670582 - Flags: review?(bzbarsky)
Attachment #670584 - Flags: review?(jgilbert)
Attachment #670584 - Flags: review?(bzbarsky)
Attachment #670584 - Flags: review?(jgilbert) → review+
Comment on attachment 670584 [details] [diff] [review]
implement WEBGL_debug_renderer_info

This needs a test that ensures that this debug extension cannot be gotten from content.  Ideally one that ensures that this is still true after chrome code _has_ gotten it from the same context.

r=me with that.
Attachment #670584 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #5)
> Comment on attachment 670584 [details] [diff] [review]
> implement WEBGL_debug_renderer_info
> 
> This needs a test that ensures that this debug extension cannot be gotten
> from content.  Ideally one that ensures that this is still true after chrome
> code _has_ gotten it from the same context.
> 
> r=me with that.

Hm. How do you enable/disable chrome privileges in a mochitest? How do you test what privileges you currently have?
> Hm. How do you enable/disable chrome privileges in a mochitest?

You write a mochitest-chrome test that runs part of itself as chrome and part as not....
Thanks, I guess 'mochitest-chrome' was the keyword I was missing.
Attached patch Chrome mochitest (obsolete) — Splinter Review
This works here.

It relies on the nontrivial assumption that messages will be received in the same order as they are posted. Is that OK?

If not, what approach would you recommend?
 - is it safe to assume that message handlers won't run before the current JS callback returns? If yes, a simple counter-based  solution is possible.
 - Otherwise we can do more postMessage, a form of SYN/ACK protocol if you want.
Attachment #671989 - Flags: review?(bzbarsky)
Note, the rationale for using postMessage here is that apparently the non-Chrome code may not call the Chrome mochitest ok() function directly.
Attached patch Chrome mochitestSplinter Review
Forgot a hg add.
Attachment #671989 - Attachment is obsolete: true
Attachment #671989 - Flags: review?(bzbarsky)
Attachment #671991 - Flags: review?(bzbarsky)
> It relies on the nontrivial assumption that messages will be received in the same order
> as they are posted. Is that OK?

As long as the posting is only happening from a single frame and the receiving is only happening in a single frame, that assumption is correct.
Comment on attachment 671991 [details] [diff] [review]
Chrome mochitest

This is good, but I would like it to test one more thing.  Specifically, in the iframe.onload, I would like the chrome code to test that it can get the WEBGL_debug_renderer_info on the subframe's WebGL context.  And then trigger the subframe part of the test _after_ that.

Also, I'm not sure why "chrome" is capitalized all over in the subframe.  ;)

r=me with those fixed.
Attachment #671991 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/756d555fc3bf
http://hg.mozilla.org/integration/mozilla-inbound/rev/e78db739d589

(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 671991 [details] [diff] [review]
> Chrome mochitest
> 
> This is good, but I would like it to test one more thing.  Specifically, in
> the iframe.onload, I would like the chrome code to test that it can get the
> WEBGL_debug_renderer_info on the subframe's WebGL context.  And then trigger
> the subframe part of the test _after_ that.

OK, done -- repeating the whole chrome test on that second canvas, factored into a helper function.

> 
> Also, I'm not sure why "chrome" is capitalized all over in the subframe.  ;)

thanks, fixed.

Ken & Zhenyao --- I don't see a good way to share such privileged extension tests at Khronos in a systematic way, but if you are interested, this code should be rather easy to adapt as WebKit/Chromium tests. You'd just have to replace the boilerplate code to create a privileged-code test, and the non-privileged domain for your test harness (replace http://mochi.test:8888/... by whatever gives non-privileged pages in your harness).
https://hg.mozilla.org/mozilla-central/rev/756d555fc3bf
https://hg.mozilla.org/mozilla-central/rev/e78db739d589
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 671555 [details] [diff] [review]
updated --- GetSupportedExtensions didn't list the new extension

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression. But blocker for the fix for bug 795701 which is a regression caused by WebIDL port (bug 779611).
User impact if declined: Bug 795701 will ship to users, causing about:support to miss WebGL renderer info. WebGL will still work unaffected, only about:support will miss information.
Testing completed (on m-c, etc.): on m-c for several days now
Risk to taking this patch (and alternatives if risky): medium risk. It's a new feature that must be exposed to privileged code only, but it's covered by an extensive mochitest-chrome test.
String or UUID changes made by this patch: none
Attachment #671555 - Flags: approval-mozilla-aurora?
Comment on attachment 671991 [details] [diff] [review]
Chrome mochitest

[Approval Request Comment]
See previous comment.
Attachment #671991 - Flags: approval-mozilla-aurora?
(In reply to Benoit Jacob [:bjacob] from comment #18)
> Risk to taking this patch (and alternatives if risky): medium risk. It's a
> new feature that must be exposed to privileged code only, but it's covered
> by an extensive mochitest-chrome test.

Do we have any lower risk alternatives. Obviously backing out bug 779611 wouldn't fly, but we're concerned with the medium risk evaluation. If this is the only path forward, where could we find regressions?
The only options are:
 - either backport this
 - or ship one Firefox version with no 'WebGL renderer' info in about:support.

If we are going to backport this, we should then backport the chrome mochitest as well and verify that it's run. It's really extensive so as long as it succeeds I'm not afraid of regressions.
Also, if this caused regressions in other WebGL extensions it would cause mochitest-1 to go orange.
Comment on attachment 671555 [details] [diff] [review]
updated --- GetSupportedExtensions didn't list the new extension

Given testing coverage here, where we are in the cycle, and the possible support impact if declined, we'll take a fix for FF18.
Attachment #671555 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #671991 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1171228
You need to log in before you can comment on or make changes to this bug.