890049 - WebGL2 Add WebGL1Context inheriting from WebGLContext

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

Replace WebGLContext by WebGLContextBase with one subclass WebGLContext1

See https://wiki.mozilla.org/Platform/GFX/WebGL2

Comment 1

[Guillaume Abadie]

Attachment: patch revision 1

Comment 2

[Guillaume Abadie]

After discussed offline, I'm okay with WebGL1Context instead of WebGLContext1, but I don't really like that we keep WebGLContext.

With what you propose Jeff, We will have something like that :
class WebGL1Context : public WebGLContext
Therefor if we read it, WebGL1Context is a WebGLContext because it's an webgl context, and I agree that make sense, but sounds redundant. But WebGLContext isn't not any kind of webgl context, this is the core of the entire upcoming webgl API where WebGL1Context will only disable features. That why I don't like this name WebGLContext for our cutting edge context class because it's too near from WebGL1Context. Your surly right about WebGLContextBase is not a good name. I'm ok to change it, but in my mind, kipping WebGLContext is not a good idea either.

Now, let say we call it WebGLCoreContext (only for example for this paragraph). Therefore we will have :
class WebGL1Context : public WebGLCoreContext
That also mean that WebGL1Context is implementing WebGLCoreContext to restrict its features and WebGLCoreContext has absolutely anythings in comment with WebGL1Context because this is the core of our webgl API. Which have much more sense.

If this patch is to big for you, because of renaming WebGLContext to something else, I can split it into two patches :
 - The first one just renames the current WebGLContext to the new name we definitely and publicly agree together, nothing else. So it will only do a str_replace and you just have to trust me I just did that.
 - The second one introduce WebGL1Context inheriting from our webgl cutting edge class with all around changes needed in much smaller patch that you can review as your are used to do.

My propositions are :
WebGLMasterContext
WebGLCoreContext
WebGLTrunkContext (because of the inheritance tree)

Anybody feel free to add other propositions.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Let's definitely do this in two patches, and be careful about find/replace. The last pass had a bunch of collateral changes in comments and other class names. (like WebGLBaseContextOptions, or similar)

One reason I like 'WebGLContext' by itself is that `WebGLContext*` would seem the proper way to refer to either `WebGL1Context` or `WebGL2Context`.

Comment 4

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> Let's definitely do this in two patches, and be careful about find/replace.
> The last pass had a bunch of collateral changes in comments and other class
> names. (like WebGLBaseContextOptions, or similar)
> 
> One reason I like 'WebGLContext' by itself is that `WebGLContext*` would
> seem the proper way to refer to either `WebGL1Context` or `WebGL2Context`.
Yeah it's true about WebGLContextBaseOptions. I chose to do it this way because of this is WebGLContextBase's options after all.

About the reason you like WebGLContext. WebGL1Context or WebGL2Context has only one difference (regardless the name of course) :
WebGL1Context::IsWebGL2() { return false; }
WebGL2Context::IsWebGL2() { return true; }
Therefore I think you won't need to have a pointer to refer either WebGL1Context or WebGL2Context ever because they have any differences. They are only different cutting edge context's profiles. If you would have WebGLContext pointer, it's surly for the WebGL context itself.

What about :
class WebGLContext;
class WebGLContextProfileVersion1 : WebGLContext;
class WebGLContextProfileVersion2 : WebGLContext;

Comment 5

[Guillaume Abadie]

Attachment: patch revision 2

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 774397 [details] [diff] [review]
patch revision 2

Review of attachment 774397 [details] [diff] [review]:
-----------------------------------------------------------------

There are still some find/replace-looking artifacts that need to go. If desired, they can go in another cleanup bug.

Also, I thought we were going to go with WebGLVerNContext instead of WebGLNContext?

::: content/canvas/src/WebGL1Context.cpp
@@ +9,5 @@
> +#include "mozilla/Telemetry.h"
> +
> +using namespace mozilla;
> +
> +// --------------------------------------------------- CONSTRUCTOR & DESTRUCTOR

I think the better way to do this is:
// --------------------------------------------
// Foo

Just make sure to cap the `// ---...' line at 80 chars.

::: content/canvas/src/WebGL1Context.h
@@ +12,5 @@
> +
> +class WebGL1Context
> +    : public WebGLContext
> +{
> +// --------------------------------------------------------------------- PUBLIC

Smaller classes like this don't need such big comment separations.
Just drop this one, and reduce the other two to just `// Foo:`

::: content/canvas/src/WebGLContext.cpp
@@ +1382,5 @@
>  // 3) If we are using ANGLE, or anything that supports ARB_robustness, query the
>  //    GPU periodically to see if the reset status bit has been set.
>  // In all of these situations, we use this timer to send the script context lost
>  // and restored events asynchronously. For example, if it triggers a context loss,
> +// the WebGLContextlost event will be sent to it the next time the robustness timer

This looks like the result of an over-zealous find/replace. It'd be nice if we could clean this up to use capitals, but not in this patch. Dupes marked with [1].

@@ +1387,5 @@
>  // fires.
>  // Note that this timer mechanism is not used unless one of these 3 criteria
>  // are met.
>  // At a bare minimum, from context lost to context restores, it would take 3
> +// full timer iterations: detection, WebGLContextlost, WebGLContextrestored.

[1]

@@ +1405,5 @@
>      if (mContextStatus == ContextLostAwaitingEvent) {
>          bool defaultAction;
>          nsContentUtils::DispatchTrustedEvent(mCanvasElement->OwnerDoc(),
>                                               static_cast<nsIDOMHTMLCanvasElement*>(mCanvasElement),
> +                                             NS_LITERAL_STRING("WebGLContextlost"),

[1]

@@ +1434,5 @@
>          }
>          mContextStatus = ContextStable;
>          nsContentUtils::DispatchTrustedEvent(mCanvasElement->OwnerDoc(),
>                                               static_cast<nsIDOMHTMLCanvasElement*>(mCanvasElement),
> +                                             NS_LITERAL_STRING("WebGLContextrestored"),

[1]

::: content/canvas/src/WebGLContext.h
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef WebGLContext_H_
> +#define WebGLContext_H_

[1], but style dictates that these should stay all-caps. Dupes marked as [2].

::: content/canvas/src/WebGLContextUtils.h
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef WebGLContextUTILS_H_
> +#define WebGLContextUTILS_H_

[2]

@@ +42,5 @@
>  }
>  
>  } // namespace mozilla
>  
> +#endif // WebGLContextUTILS_H_

[2]

Comment 7

[Guillaume Abadie]

Attachment: patch revision 1

Fix problems, but about the // ---- , I thought that would be a good idea to introduce those in WebGLContext.h , to get more structure in the class. Therefore I added here on this tiny class to introduce a guideline to structure the whole WebGL API implementation, where this files (WebGL1Context.h/cpp) would become the example.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 776084 [details] [diff] [review]
patch revision 1

Review of attachment 776084 [details] [diff] [review]:
-----------------------------------------------------------------

Remove the OOM check we'll never hit, and fix the find/replace artifact.

::: content/canvas/src/WebGL1Context.cpp
@@ +44,5 @@
> +    Telemetry::Accumulate(Telemetry::CANVAS_WEBGL_USED, 1);
> +    nsIDOMWebGLRenderingContext* ctx = new WebGL1Context();
> +
> +    if (!ctx)
> +        return NS_ERROR_OUT_OF_MEMORY;

I believe that this can't happen anymore, as IIRC we have infallible `new`. (crash on OOM)

::: content/canvas/src/WebGLContextUtils.h
@@ +42,5 @@
>  }
>  
>  } // namespace mozilla
>  
> +#endif // WebGLContextUTILS_H_

Find/replace artifact.

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 776084 [details] [diff] [review]
patch revision 1

::: content/canvas/src/WebGL1Context.cpp
>@@ -0,0 +1,53 @@
>+// -----------------------------------------------------------------------------
>+// IMPLEMENT nsWrapperCache
>+
>+nsresult
>+NS_NewCanvasRenderingContextWebGL(nsIDOMWebGLRenderingContext** aResult)

Is this really implementing nsWrapperCache?

Comment 10

[Guillaume Abadie]

Attachment: patch for landing

This patch is exactly attachment 776084 [details] [diff] [review] which as a r+ from jgilbert, but with just theses differences :
 - remove if (!ctx) return NS_ERROR_OUT_OF_MEMORY;
 - remove the #endif // WebGLContextUTILS_H_ artifact
 - replace comment "// IMPLEMENT nsWrapperCache" by "// INSTANCING nsIDOMWebGLRenderingContext"

Asking for landing

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a266b677b5fb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a266b677b5fb