Clean up the #includes in the WebGL implementation

NEW
Assigned to

Status

()

5 years ago
5 years ago

People

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

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open] webgl-internal)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Changes includes between few content/canvas/src sources files to reduce includes number in WebGLContext.h
(Assignee)

Comment 1

5 years ago
Created attachment 758608 [details] [diff] [review]
patch

patch design to reduce the number of includes in content/canvas/src/WebGLContext.h
Attachment #758608 - Flags: review?(bjacob)
Summary: WebGLContext's includes optimizations → Clean up the #includes in the WebGL implementation
(Assignee)

Comment 2

5 years ago
Created attachment 758803 [details] [diff] [review]
patch with more removed includes

patch removing more useless includes in WebGL.
Attachment #758608 - Attachment is obsolete: true
Attachment #758608 - Flags: review?(bjacob)
Attachment #758803 - Flags: review?(bjacob)
It would be nice if you used hg cp to preserve blame.
Oh, do you mean that when moving _part_ of a file to a separate new file, you would hg cp the _entire_ file and remove the unwanted parts from the copy? Does that trick work? That's nice to know for next time, but I won't r- a patch for this as I had no idea about that trick until now.
(In reply to Benoit Jacob [:bjacob] from comment #4)
> Oh, do you mean that when moving _part_ of a file to a separate new file,
> you would hg cp the _entire_ file and remove the unwanted parts from the
> copy? Does that trick work? That's nice to know for next time, but I won't
> r- a patch for this as I had no idea about that trick until now.

Yes.
Comment on attachment 758803 [details] [diff] [review]
patch with more removed includes

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

r+ with nits below.

::: content/canvas/src/WebGLContext.h
@@ +7,5 @@
>  #define WEBGLCONTEXT_H_
>  
>  #include "mozilla/Attributes.h"
> +#include "GLDefs.h"
> +#include "WebGLActiveInfo.h"

Is this activeinfo #include really needed? I don't see why it would be. Also, we forward-declare WebGLActiveInfo below, so something has to go.

::: content/canvas/src/WebGLMemoryMultiReporterWrapper.h
@@ +23,5 @@
> +    ~WebGLMemoryMultiReporterWrapper();
> +    static WebGLMemoryMultiReporterWrapper* sUniqueInstance;
> +
> +    // here we store plain pointers, not RefPtrs: we don't want the 
> +    // WebGLMemoryMultiReporterWrapper unique instance to keep alive all		

The above two lines have trailing whitespace, please remove.

@@ +46,5 @@
> +    static void RemoveWebGLContext(const WebGLContext* c) {
> +        ContextsArrayType & contexts = Contexts();
> +        contexts.RemoveElement(c);
> +        if (contexts.IsEmpty()) {
> +            delete sUniqueInstance; 

Trailing whitespace above.
Attachment #758803 - Flags: review?(bjacob) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/0e5a7f0d8a4b

Leave open until nits are addressed.
Whiteboard: [leave open]
Assignee: nobody → gabadie
Target Milestone: --- → mozilla24

Comment 10

5 years ago
This patch may have caused some issues, see bug 881681, bug 881264, bug 881805 and bug 881520. Firefox throws "NS_ERROR_FAILURE: Failure" when setting the size of a Canvas with a WebGL context.

Comment 11

5 years ago
(In reply to Conor Boyle from comment #10)
> This patch may have caused some issues, see bug 881681, bug 881264, bug
> 881805 and bug 881520. Firefox throws "NS_ERROR_FAILURE: Failure" when
> setting the size of a Canvas with a WebGL context.

False alarm, this patch falls outside the regression range.
I would suspect bug 798438, FWIW.
Whiteboard: [leave open] → [leave open] webgl-internal
You need to log in before you can comment on or make changes to this bug.