815921 - Split out WebGLTexture into separate files

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This one is a big class living almost entirely inside WebGLContext.h.

Split it into a new separate header, WebGLTexture.h, and try to move most (or all) of its actual code in WebGLTexture.cpp.

Typically WebGLTexture.h would only include WebGLObjectModel.h and not WebGLContext.h. The point is to make it so that we can #include WebGLTexture.h without dragging in too many dependencies, and to bring WebGLContext.h down to a reasonable size.

There are other texture-related files around here. For example, WebGLTexelConversions.h currently includes WebGLContext.h; you could try to see if it could instead just include WebGLTexture.h, and then you can include WebGLContext.h in WebGLTexelConversions.cpp. It's OK to include big headers in a cpp file --- but we must be parcimonious with including big headers into _headers_.

See bug 801499 for the work on splitting WebGLBuffer out (which had to do lots of extra work as WebGLObjectModel.h didn't exist yet).

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Note: when moving its code into WebGLTexture.cpp, use discretion to decide where to stop. If a method is very small, does not require dragging in any dependency, is performance-critical so that the cost of the function call matters, and is used outside of WebGLTexture.cpp, then it can make sense for it to stay in WebGLTexture.h so that the compiler can still inline it without relying on link-time optimization.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Also, you will need to #include some headers in WebGLTexture.h, such as nsTArray.h. That's OK.

Comment 3

[Brendon]

I am new to Mozilla contributions, and was very interested in working on this bug to start.
Is there any other information I should know before working on this bug?

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Nothing that I can think of, but don't hesitate to ask questions. If you get something that builds and passes 1.0.1 conformance tests,
  https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html
you'll know that you're on the right track.

Let me know when you start and I'll assign this to you.

Comment 5

[Brendon]

I have started the work on it.
Thank you for the assistance.

Comment 6

[Brendon]

So I have been referencing bug 801499 for more assistance. Is this a good reference? 
I have completed a good amount of editing in WebGLTexture.cpp and WebGLContext.h (mostly cutting, pasting, then editing). I have also created the new WebGLTexture.h file. Am I headed in the right direction?

Thanks for the help.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Sounds like you are headed in the right direction, and yes, bug 801499 is a good reference. Just know that of all the split-WebGL-stuff-into-separate-file bugs that I've recently filed, you've picked the biggest one. Don't hesitate to start with a smaller one, such as bug 816176, as a warm up, if that helps.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Or rather Bug 816168, now htat bug 816176 is taken.

Comment 9

[Brendon]

Alright, well I seem to be having limited troubles so far, I'll stick to doing this bug for right now. I was wondering if class imageinfo (contained in WebGLContext.h L:1673) is also something needed in the WebGLTexture.cpp/WebGLTexture.h? I am assuming so, but just wanted to check.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Yes. ImageInfo is a nested class inside of class WebGLTexture, so it should move together with it.

Comment 11

[Brendon]

I'm currently correcting for any dependencies/references to WebGLTexture.h.
I just had one question:
Is this acceptable  in WebGLTexture.cpp for the nested class ImageInfo and it's functions:

WebGLTexture :: ImageInfo :: ImageInfo(/*whatever args */){/*whatever else */}?

specifically the "::ImageInfo::" part.
(I am still a novice when it comes to C++)

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

It sure is acceptable! I don't see a problem with that.

Comment 13

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

@Brendon Sorry Brendon, I had to take this bug because it's blocking 816173 and I need to work on it. I am sure you can find some other bug to work on.

@Benoit This patch does 
a) What the other "Split out Foo into separate files" do
b) Includes WebGLTextures.h in WebGLTexelConversions.h rather than WebGLContext.h and includes WebGLContext.h in WebGLTexelConversions.cpp instead.

Comment 14

[Brendon]

@Saurabh Alright, makes sense.

@Benoit Thanks for helping me out through my first attempt at fixing a Mozilla bug, although not finished.

~Brendon

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Brendon from comment #14)
> @Benoit Thanks for helping me out through my first attempt at fixing a
> Mozilla bug, although not finished.

No problem, and apologies for the rough process here. I hope you'll find other bugs you'll like. Maybe http://www.joshmatthews.net/bugsahoy/ can help.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 688034 [details] [diff] [review]
Patch v1

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

Look good.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Saurabh Anand [:sawrubh] from comment #13)
> b) Includes WebGLTextures.h in WebGLTexelConversions.h rather than
> WebGLContext.h and includes WebGLContext.h in WebGLTexelConversions.cpp
> instead.

Yup, it's always good to include a smaller header if you can.

Comment 18

[Mark Capella [:capella]]

Re-based to m-c ... pushing to TRY
https://tbpl.mozilla.org/?tree=Try&rev=10379a54be9b

Comment 19

[Mark Capella [:capella]]

On to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d203a559c0c1

Comment 20

[Ed Morley [:emorley]]

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