1271478 - Implement webglcontextcreationerror

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

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.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Bug 1271478 - r?jmuizel - Clean up failIfMajorPerfCaveat code.

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/

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: r?smaug - Add WebGLContextEvent.

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/

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: r?jrmuizel - Record context creation failure reason.

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/

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: r?jrmuizel - Add a test.

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/

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: r?jrmuizel - Remove unused file.

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/

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

https://reviewboard.mozilla.org/r/51451/#review48333

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8750551 [details]
MozReview Request: r?jrmuizel - Add a test.

https://reviewboard.mozilla.org/r/51457/#review48335

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

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

https://reviewboard.mozilla.org/r/51451/#review48557

Comment 11

[Jeff Muizelaar [:jrmuizel]]

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 12

[Kelsey Gilbert [:jgilbert]]

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/

Comment 13

[Kelsey Gilbert [:jgilbert]]

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 14

[Kelsey Gilbert [:jgilbert]]

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 15

[Kelsey Gilbert [:jgilbert]]

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 16

[Kelsey Gilbert [:jgilbert]]

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/

Comment 17

[Kelsey Gilbert [:jgilbert]]

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.

Comment 18

[Kelsey Gilbert [:jgilbert]]

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 19

[Kelsey Gilbert [:jgilbert]]

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 20

[Kelsey Gilbert [:jgilbert]]

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 21

[Kelsey Gilbert [:jgilbert]]

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 22

[Kelsey Gilbert [:jgilbert]]

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 23

[Kelsey Gilbert [:jgilbert]]

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/

Comment 24

[Kelsey Gilbert [:jgilbert]]

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 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8750549 [details]
MozReview Request: r?smaug - Add WebGLContextEvent.

Is this done right?

Comment 26

[Jeff Muizelaar [:jrmuizel]]

(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<>?

Comment 27

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: r?jrmuizel - Remove &* from forwarded out-params.

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 29

[Kelsey Gilbert [:jgilbert]]

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 30

[Kelsey Gilbert [:jgilbert]]

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 31

[Kelsey Gilbert [:jgilbert]]

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 32

[Kelsey Gilbert [:jgilbert]]

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 33

[Kelsey Gilbert [:jgilbert]]

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/

Comment 34

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

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

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 36

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8750552 [details]
MozReview Request: r?jrmuizel - Remove unused file.

https://reviewboard.mozilla.org/r/51459/#review48823

Comment 38

[Kelsey Gilbert [:jgilbert]]

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/

Comment 39

[Kelsey Gilbert [:jgilbert]]

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 40

[Kelsey Gilbert [:jgilbert]]

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 41

[Kelsey Gilbert [:jgilbert]]

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 42

[Kelsey Gilbert [:jgilbert]]

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/

Comment 43

[Kelsey Gilbert [:jgilbert]]

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/

Comment 44

[Kelsey Gilbert [:jgilbert]]

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 45

[Kelsey Gilbert [:jgilbert]]

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 46

[Kelsey Gilbert [:jgilbert]]

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 47

[Kelsey Gilbert [:jgilbert]]

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 48

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 49

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8750550 [details]
MozReview Request: r?jrmuizel - Record context creation failure reason.

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

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13e4cb81d4eb

Comment 51

[Jeff Muizelaar [:jrmuizel]]

Can you add a test for nested context creation failure. i.e. create a failing context in the event handler.

Comment 52

[Kelsey Gilbert [: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.

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13e4cb81d4eb

Comment 54

[Florian Scholz (Open Web Docs)]

Mentioned on Firefox 49 for developers: 
https://developer.mozilla.org/en-US/Firefox/Releases/49#WebGL

Reference pages:
https://developer.mozilla.org/en-US/docs/Web/Events/webglcontextcreationerror
https://developer.mozilla.org/en-US/docs/Web/API/WebGLContextEvent
https://developer.mozilla.org/en-US/docs/Web/API/WebGLContextEvent/statusMessage

Comment 55

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8750551 [details]
MozReview Request: r?jrmuizel - Add a test.

https://reviewboard.mozilla.org/r/51457/#review50120

Comment 56

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 57

[Marcia Knous [:marcia]]

49+ for capturing this in the release notes, which is noted in https://developer.mozilla.org/en-US/Firefox/Releases/49#Changes_for_Web_developers