Closed Bug 1271478 Opened 8 years ago Closed 8 years ago

Implement webglcontextcreationerror

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed
relnote-firefox --- 49+

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: [games:p1][platform-rel-Games])

Attachments

(5 files, 1 obsolete file)

MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.
58 bytes, text/x-review-board-request
jrmuizel
: review+
mtseng
: review+
Details
MozReview Request: r?smaug - Add WebGLContextEvent.
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: r?jrmuizel - Record context creation failure reason.
58 bytes, text/x-review-board-request
mtseng
: review+
jrmuizel
: review+
Details
MozReview Request: r?jrmuizel - Add a test.
58 bytes, text/x-review-board-request
jrmuizel
: review+
mtseng
: review+
Details
MozReview Request: r?jrmuizel - Remove unused file.
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
This will allow us to communicate why a context creation attempt failed.

This has been part of the spec for a while, but we haven't implemented it.
Chrome implements it, but Edge does not. Safari may or may not.

This will also lay some groundwork for getting better context creation failure info via Telemetry.
From ecccc21b18dd31549d546f1450ebfa29e9b8335e Mon Sep 17 00:00:00 2001
---
 dom/canvas/WebGLContext.cpp | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/51451/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51451/
From 6fbb2dec967b1190db1ce6b4659d36914a41acd5 Mon Sep 17 00:00:00 2001
---
 dom/webidl/WebGLContextEvent.webidl     | 18 ++++++++++++++++++
 dom/webidl/WebGLRenderingContext.webidl | 16 +++-------------
 dom/webidl/moz.build                    |  1 +
 3 files changed, 22 insertions(+), 13 deletions(-)
 create mode 100644 dom/webidl/WebGLContextEvent.webidl

Review commit: https://reviewboard.mozilla.org/r/51453/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51453/
From 6345713e38826d1adc1eb935555a8a284fb7bab9 Mon Sep 17 00:00:00 2001
---
 dom/canvas/WebGL2Context.cpp        |  17 ++++--
 dom/canvas/WebGLContext.cpp         | 119 +++++++++++++++++++++++++-----------
 dom/canvas/WebGLContext.h           |  13 ++--
 dom/canvas/WebGLContextValidate.cpp |  35 ++++++-----
 4 files changed, 123 insertions(+), 61 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/51455/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51455/
From 2578e50d692459c368ec39a6861aed538415e16d Mon Sep 17 00:00:00 2001
---
 dom/canvas/moz.build                               |  4 +-
 dom/canvas/test/webgl-mochitest/mochitest.ini      |  1 +
 .../test_webglcontextcreationerror.html            | 66 ++++++++++++++++++++++
 3 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 dom/canvas/test/webgl-mochitest/test_webglcontextcreationerror.html

Review commit: https://reviewboard.mozilla.org/r/51457/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51457/
From e51e6cc0d106bf92c39e3630c287f671f186bb89 Mon Sep 17 00:00:00 2001
---
 dom/canvas/test/mochitest-subsuite-webgl.ini | 5 -----
 1 file changed, 5 deletions(-)
 delete mode 100644 dom/canvas/test/mochitest-subsuite-webgl.ini

Review commit: https://reviewboard.mozilla.org/r/51459/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51459/
Attachment #8750548 - Flags: review?(mtseng)
Attachment #8750548 - Flags: review?(jmuizelaar)
Attachment #8750549 - Flags: review?(bugs)
Attachment #8750550 - Flags: review?(mtseng)
Attachment #8750550 - Flags: review?(jmuizelaar)
Attachment #8750551 - Flags: review?(mtseng)
Attachment #8750551 - Flags: review?(jmuizelaar)
Attachment #8750552 - Flags: review?(jmuizelaar)
Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

https://reviewboard.mozilla.org/r/51453/#review48325

Those fixed, r+.

::: dom/webidl/WebGLContextEvent.webidl:11
(Diff revision 1)
> + * The origin of this IDL file is
> + * https://www.khronos.org/registry/webgl/specs/latest/1.0/#fire-a-webgl-context-event
> + */
> +
> +[Constructor(DOMString type, optional WebGLContextEventInit eventInit)]
> +interface WebGLContextEvent : Event {

Do we need the event also in Workers, like do we dispatch the event also in case CanvasContext is used in workers? If so, the interface should have  Exposed=(Window,Worker).
Looking at other interfaces, looks like
Exposed=(Window,Worker), Func="mozilla::dom::OffscreenCanvas::PrefEnabledOnWorkerThread"
would be the right thing to do for this new interface.


You'll need to add the new interface to
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_interfaces.html?force=1

and depending on whether we want to expose to workers also http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/test_worker_interfaces.js
and http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/test_serviceworker_interfaces.js
similar way as other other canvas related interfaces have been exposed there.

::: dom/webidl/WebGLContextEvent.webidl:17
(Diff revision 1)
> +  readonly attribute DOMString statusMessage;
> +};
> +
> +// EventInit is defined in the DOM4 specification.
> +dictionary WebGLContextEventInit : EventInit {
> +  DOMString statusMessage = "";

Please file a spec bug about the default value.
FWIW, Blink seems to default to "" too.
Attachment #8750549 - Flags: review?(bugs) → review+
Comment on attachment 8750548 [details]
MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.

https://reviewboard.mozilla.org/r/51451/#review48333
Attachment #8750548 - Flags: review?(mtseng) → review+
Attachment #8750551 - Flags: review?(mtseng) → review+
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

https://reviewboard.mozilla.org/r/51455/#review48337

::: dom/canvas/WebGLContext.cpp:673
(Diff revision 1)
>      MOZ_RELEASE_ASSERT(!gl);
>      gl = nullptr;
>      while (!fallbackCaps.empty()) {
>          gl::SurfaceCaps& caps = fallbackCaps.front();
>  
> -        gl = fnCreateGL(caps, flags, this);
> +        gl = fnCreateGL(caps, flags, this, &*out_failReason);

Why not just passing out_failReason here and following code?

::: dom/canvas/WebGLContext.cpp:782
(Diff revision 1)
> +    event->SetTrusted(true);
> +
> +    bool didPreventDefault;
> +    target->DispatchEvent(event, &didPreventDefault);
> +
> +    //////

Meaningless comment?
Attachment #8750550 - Flags: review?(mtseng)
Attachment #8750548 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8750548 [details]
MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.

https://reviewboard.mozilla.org/r/51451/#review48557
Attachment #8750550 - Flags: review?(jmuizelaar)
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

https://reviewboard.mozilla.org/r/51455/#review48559

::: dom/canvas/WebGLContext.cpp:853
(Diff revision 1)
>      // we are initializing a new context.
>  
> +    const auto fnFailContextCreation = [this](const nsACString& text) {
> +        this->ThrowEvent_WebGLContextCreationError(text);
> +        return NS_ERROR_FAILURE;
> +    };

I'm not sure having a helper function for this is really worth it. It requires the reader to lookup what the function does when it could nearly as easily just be inline.
Comment on attachment 8750548 [details]
MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51451/diff/1-2/
Attachment #8750550 - Flags: review?(mtseng)
Attachment #8750550 - Flags: review?(jmuizelaar)
Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51453/diff/1-2/
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51455/diff/1-2/
Comment on attachment 8750551 [details]
MozReview Request: r?jrmuizel - Add a test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51457/diff/1-2/
Comment on attachment 8750552 [details]
MozReview Request: r?jrmuizel - Remove unused file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51459/diff/1-2/
https://reviewboard.mozilla.org/r/51455/#review48337

> Why not just passing out_failReason here and following code?

The logic here is that it explicitly passes the outparam to the callee.

Compare:
bool foo;
Bar(&foo);

void Bar(bool* const out_foo) {
  Qux(out_foo);
vs
  Qux(&*out_foo);
}

It maintains the &xxxx pattern for passing an outparam, and that we're expecting to get something out of it.

> Meaningless comment?

This is for visually splitting up the code flow a bit into logical blocks.
https://reviewboard.mozilla.org/r/51455/#review48559

> I'm not sure having a helper function for this is really worth it. It requires the reader to lookup what the function does when it could nearly as easily just be inline.

Fixed.
Comment on attachment 8750548 [details]
MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51451/diff/2-3/
Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51453/diff/2-3/
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51455/diff/2-3/
Comment on attachment 8750551 [details]
MozReview Request: r?jrmuizel - Add a test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51457/diff/2-3/
Comment on attachment 8750552 [details]
MozReview Request: r?jrmuizel - Remove unused file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51459/diff/2-3/
https://reviewboard.mozilla.org/r/51453/#review48325

> Do we need the event also in Workers, like do we dispatch the event also in case CanvasContext is used in workers? If so, the interface should have  Exposed=(Window,Worker).
> Looking at other interfaces, looks like
> Exposed=(Window,Worker), Func="mozilla::dom::OffscreenCanvas::PrefEnabledOnWorkerThread"
> would be the right thing to do for this new interface.
> 
> 
> You'll need to add the new interface to
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_interfaces.html?force=1
> 
> and depending on whether we want to expose to workers also http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/test_worker_interfaces.js
> and http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/test_serviceworker_interfaces.js
> similar way as other other canvas related interfaces have been exposed there.

Like this?

> Please file a spec bug about the default value.
> FWIW, Blink seems to default to "" too.

https://github.com/KhronosGroup/WebGL/pull/1669
Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

Is this done right?
Attachment #8750549 - Flags: review+ → review?(bugs)
(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> https://reviewboard.mozilla.org/r/51455/#review48337
> 
> > Why not just passing out_failReason here and following code?
> 
> The logic here is that it explicitly passes the outparam to the callee.
> 
> Compare:
> bool foo;
> Bar(&foo);
> 
> void Bar(bool* const out_foo) {
>   Qux(out_foo);
> vs
>   Qux(&*out_foo);
> }
> 
> It maintains the &xxxx pattern for passing an outparam, and that we're
> expecting to get something out of it.

While, I agree that it's valuable to convey this information, I think it creates a weird enough pattern that it's not worth it. I was confused when I read this code.

A better way to deal with outparams is to not have them at all. Wouldn't it be better just to return a struct with bool and a string or even just a pair<>?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #26)
> (In reply to Jeff Gilbert [:jgilbert] from comment #17)
> > https://reviewboard.mozilla.org/r/51455/#review48337
> > 
> > > Why not just passing out_failReason here and following code?
> > 
> > The logic here is that it explicitly passes the outparam to the callee.
> > 
> > Compare:
> > bool foo;
> > Bar(&foo);
> > 
> > void Bar(bool* const out_foo) {
> >   Qux(out_foo);
> > vs
> >   Qux(&*out_foo);
> > }
> > 
> > It maintains the &xxxx pattern for passing an outparam, and that we're
> > expecting to get something out of it.
> 
> While, I agree that it's valuable to convey this information, I think it
> creates a weird enough pattern that it's not worth it. I was confused when I
> read this code.
It's only weird until you get used to it. :)
Can revert. It's still out_-prefixed, so it's not too mysterious.

> 
> A better way to deal with outparams is to not have them at all. Wouldn't it
> be better just to return a struct with bool and a string or even just a
> pair<>?
Succinct tuple returns might be better, but it's traditionally been a huge mess to do well in C++.
Let's not sidetrack on this here.
From 5ed59c005653f404a164cc9b57d822fb8e0c4b37 Mon Sep 17 00:00:00 2001
---
 dom/canvas/WebGLContext.cpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/51775/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51775/
Comment on attachment 8750548 [details]
MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51451/diff/3-4/
Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51453/diff/3-4/
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51455/diff/3-4/
Comment on attachment 8750551 [details]
MozReview Request: r?jrmuizel - Add a test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51457/diff/3-4/
Comment on attachment 8750552 [details]
MozReview Request: r?jrmuizel - Remove unused file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51459/diff/3-4/
Attachment #8751045 - Flags: review?(jmuizelaar)
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

https://reviewboard.mozilla.org/r/51455/#review48615
Attachment #8750550 - Flags: review?(mtseng) → review+
Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

https://reviewboard.mozilla.org/r/51453/#review48695

You're still missing the change to dom/workers/test/serviceworkers/test_serviceworker_interfaces.js
Attachment #8750549 - Flags: review?(bugs) → review+
Comment on attachment 8751045 [details]
MozReview Request: r?jrmuizel - Remove &* from forwarded out-params.

https://reviewboard.mozilla.org/r/51775/#review48803

Can you just fold this into the previous patch?
Attachment #8751045 - Flags: review?(jmuizelaar)
Attachment #8750552 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8750552 [details]
MozReview Request: r?jrmuizel - Remove unused file.

https://reviewboard.mozilla.org/r/51459/#review48823
Comment on attachment 8750548 [details]
MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51451/diff/4-5/
Attachment #8750551 - Attachment description: MozReview Request: r?jrmuizel - Add test for webglcontextcreationerror. → MozReview Request: r?jrmuizel - Remove &* from forwarded out-params.
Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51453/diff/4-5/
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51455/diff/4-5/
Comment on attachment 8750551 [details]
MozReview Request: r?jrmuizel - Add a test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51457/diff/4-5/
Comment on attachment 8750552 [details]
MozReview Request: r?jrmuizel - Remove unused file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51459/diff/4-5/
Attachment #8751045 - Attachment is obsolete: true
Comment on attachment 8750548 [details]
MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51451/diff/5-6/
Attachment #8750551 - Attachment description: MozReview Request: r?jrmuizel - Remove &* from forwarded out-params. → MozReview Request: r?jrmuizel - Add a test.
Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51453/diff/5-6/
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51455/diff/5-6/
Comment on attachment 8750551 [details]
MozReview Request: r?jrmuizel - Add a test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51457/diff/5-6/
Comment on attachment 8750552 [details]
MozReview Request: r?jrmuizel - Remove unused file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51459/diff/5-6/
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

https://reviewboard.mozilla.org/r/51455/#review48865

The if (Length) { Append("\n") } idiom is sort of ugly, but whatevs.

::: dom/canvas/WebGLContextValidate.cpp:950
(Diff revision 6)
> +        out_failReason->Assign(reason);
>          return false;
>      }
>  
>      if (IsWebGL2() &&
> -        !InitWebGL2())
> +        !InitWebGL2(&*out_failReason))

Ah &* slipped in here.
Attachment #8750550 - Flags: review?(jmuizelaar)
Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

https://reviewboard.mozilla.org/r/51455/#review48869
Attachment #8750550 - Flags: review+
Can you add a test for nested context creation failure. i.e. create a failing context in the event handler.
Flags: needinfo?(jgilbert)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #51)
> Can you add a test for nested context creation failure. i.e. create a
> failing context in the event handler.

Good catch. Let's handle this is a separate bug.
Flags: needinfo?(jgilbert)
Blocks: 1272193
https://hg.mozilla.org/mozilla-central/rev/13e4cb81d4eb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Attachment #8750551 - Flags: review?(jmuizelaar) → review+
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: The webglcontextcreationerror event, which allows to get better information on why a WebGL context creation failed, has been implemented.
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/Firefox/Releases/49#WebGL
relnote-firefox: --- → ?
Whiteboard: [games:p1] → [games:p1][platform-rel-Games]
Blocks: 1289901
Depends on: 1303301
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: