WebGL2 Add WebGL1Context inheriting from WebGLContext

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: guillaume.abadie, Assigned: guillaume.abadie)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Replace WebGLContext by WebGLContextBase with one subclass WebGLContext1

See https://wiki.mozilla.org/Platform/GFX/WebGL2
Blocks: webgl2
No longer blocks: webgl2
Depends on: webgl2
Posted patch patch revision 1 (obsolete) — Splinter Review
Attachment #771357 - Flags: review?(jgilbert)
Blocks: webgl2
No longer depends on: webgl2
Assignee: nobody → gabadie
Blocks: 890311
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.
Flags: needinfo?(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`.
Flags: needinfo?(jgilbert)
(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;
Summary: WebGL2 Replace WebGLContext by WebGLContextBase with one subclass WebGLContext1 → WebGL2 Add WebGL1Context inheriting from WebGLContext
Posted patch patch revision 2 (obsolete) — Splinter Review
Attachment #771357 - Attachment is obsolete: true
Attachment #771357 - Flags: review?(jgilbert)
Attachment #774397 - Flags: review?(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]
Attachment #774397 - Flags: review?(jgilbert) → review-
Posted patch patch revision 1 (obsolete) — Splinter Review
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.
Attachment #774397 - Attachment is obsolete: true
Attachment #776084 - Flags: review?(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.
Attachment #776084 - Flags: review?(jgilbert) → review+
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?
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
Attachment #776084 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a266b677b5fb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.