Closed Bug 1099427 Opened 10 years ago Closed 9 years ago

Clean up formatting in WebGL files

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

Attachments

(1 file, 2 obsolete files)

We more or less have a format in mind. Let's just apply it to our existing code instead of letting old code remain as a bad example.

We don't need to critically review all the code we touch here, but we should 'review' in the sense that we agree on the general changes.
Attachment #8523245 - Flags: review?(dglastonbury)
Comment on attachment 8523245 [details] [diff] [review]
0001-Improve-formatting-consistency-in-WebGL-files.patch

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

::: dom/canvas/WebGL1Context.cpp
@@ +16,3 @@
>  WebGL1Context::WebGL1Context()
>      : WebGLContext()
>  {

This might be heresy, but I like
{ }
instead of
{
}

Thoughts?

::: dom/canvas/WebGLBuffer.cpp
@@ +71,4 @@
>  }
>  
>  bool
> +WebGLBuffer::Validate(GLenum type, uint32_t max_allowed, size_t first,

maxAllowed

::: dom/canvas/WebGLContext.cpp
@@ +154,2 @@
>  {
> +    if (!mWebGL || strcmp(topic, "memory-pressure")) {

Remove { }s

@@ +163,5 @@
>      {
>          wantToLoseContext = false;
>      }
>  
>      if (wantToLoseContext) {

Remove { }s

@@ +176,4 @@
>  {
>      nsAutoString type;
> +    event->GetType(type);
> +    if (!mWebGL || !type.EqualsLiteral("visibilitychange")) {

Remove { }s

@@ +185,2 @@
>  
> +    if (canvas && !canvas->OwnerDoc()->Hidden()) {

Remove { }s

@@ +406,2 @@
>  {
> +    if (options.isNullOrUndefined() && mOptionsFrozen) {

Remove { }s

::: dom/canvas/WebGLContext.h
@@ +847,5 @@
> +    JS::Value GetQueryObject(JSContext* cx, WebGLQuery* query, GLenum pname);
> +
> +    void GetQueryObject(JSContext* cx, WebGLQuery* query, GLenum pname,
> +                        JS::MutableHandle<JS::Value> retval)
> +    {

I thought this should go on the end of the previous line in a header file?

@@ +1278,2 @@
>      {
> +      return SurfaceFromElement(&element);

4 space indent.

@@ +1542,2 @@
>  {
> +    if (!object) {

Remove { }s
Comment on attachment 8523245 [details] [diff] [review]
0001-Improve-formatting-consistency-in-WebGL-files.patch

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

::: dom/canvas/WebGLContextExtensions.cpp
@@ +16,2 @@
>  
> +/*static*/ const char*

is it important to call out /*static*/?

::: dom/canvas/WebGLElementArrayCache.h
@@ +17,5 @@
>  
>  template<typename T>
>  struct WebGLElementArrayCacheTree;
>  
> +/* WebGLElementArrayCache implements WebGL element array buffer validation for

I don't prefer this. (/* Yada over /*\n* Yada)

@@ +25,2 @@
>   *
> + * * Validate, to be called by WebGLContext::DrawElements, is where we use the

Prefer something other than * to mark bullet point.

::: dom/canvas/WebGLExtensionDrawBuffers.cpp
@@ +52,5 @@
> +    if (mIsLost) {
> +        mContext->ErrorInvalidOperation("%s: Extension is lost.",
> +                                        "drawBuffersWEBGL");
> +        return;
> +    }

This one seems worse to me.
Comment on attachment 8523245 [details] [diff] [review]
0001-Improve-formatting-consistency-in-WebGL-files.patch

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

r+ though I'm not 100% in agreement with everything here. Although, I don't care for a session of bike shedding either.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +437,5 @@
>  
>  void
>  WebGLFramebuffer::DetachAllAttachments()
>  {
> +    for (size_t i = 0; i < mColorAttachments.Length(); i++) {

I don't like this way of calling a function in a loop. But that may be a Pavlovian response from too many years working in gamedev.

"So what if the loop terminate is loaded every loop?  It's not like this is perf. critical.", you say?. To us, it was a matter of principle.

::: dom/canvas/WebGLObjectModel.cpp
@@ +21,1 @@
>      return mContext == other &&

Could you wrap the multi-line && in (). It's a bit silly, but it helps Emacs to indent correctly.

::: dom/canvas/WebGLObjectModel.h
@@ -174,4 @@
>  public:
>      WebGLRefPtr()
>          : mRawPtr(0)
> -    { }

I prefer this one over {}. I don't know why.

::: dom/canvas/WebGLProgram.h
@@ +71,1 @@
>          if (!(mGeneration + 1).isValid())

Why is this implemented in the header?

::: dom/canvas/WebGLQuery.cpp
@@ -37,2 @@
>  {
> -    WebGLRefPtr<WebGLQuery>* targetSlot = mContext->GetQueryTargetSlot(mType, "WebGLQuery::IsActive()");

Sure this is better or are we aiming for < 80 chars per line. (I think 80 chars per line is way too narrow in modern day)

::: dom/canvas/WebGLRenderbuffer.cpp
@@ +185,3 @@
>      if (secondaryFormat) {
> +        gl->fRenderbufferStorage(LOCAL_GL_RENDERBUFFER, secondaryFormat, width,
> +                                 height);

Like this, for example. Is wrapping height really better?

::: dom/canvas/WebGLTexture.h
@@ +20,5 @@
>  namespace mozilla {
>  
>  // Zero is not an integer power of two.
> +inline
> +bool is_pot_assuming_nonnegative(GLsizei x)

bool on previous line with inline.

@@ +243,4 @@
>      }
>  
> +    bool
> +    DoesMipmapHaveAllLevelsConsistentlyDefined(TexImageTarget texImageTarget) const;

Yeah, I don't think this line break after bool achieves anything.

::: dom/canvas/WebGLVertexArrayFake.cpp
@@ +39,5 @@
>          }
>      }
>  
> +    for (size_t i = mAttribs.Length(); i < prevVertexArray->mAttribs.Length();
> +         ++i)

Eewww.  Instead, extract prevVertArray->mAttribs.Length() into a local const variable.

::: dom/canvas/WebGLVertexAttribData.h
@@ +75,1 @@
>  {

Header file, does { go on end of previous line?

@@ +75,2 @@
>  {
> +  field.buf = nullptr;

4 space indent.
Attachment #8523245 - Flags: review?(dglastonbury) → review+
Comment on attachment 8523245 [details] [diff] [review]
0001-Improve-formatting-consistency-in-WebGL-files.patch

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

::: dom/canvas/WebGL1Context.cpp
@@ +16,3 @@
>  WebGL1Context::WebGL1Context()
>      : WebGLContext()
>  {

I would lean towards `{}` as opposed to `{ }`.
Elsewhere I see you support `{ }` over `{}`. I don't really care, so I'd be fine with this. I suppose this can come out of a general "curly brackets should be surrounded by spaces" rule, a la:
Foo foo = { bar, bat }
And:
bool Foo() const { return mFoo; }

I'm not going to fix that in this patch, though, I think.

::: dom/canvas/WebGLBuffer.cpp
@@ +71,4 @@
>  }
>  
>  bool
> +WebGLBuffer::Validate(GLenum type, uint32_t max_allowed, size_t first,

ok

::: dom/canvas/WebGLContext.h
@@ +847,5 @@
> +    JS::Value GetQueryObject(JSContext* cx, WebGLQuery* query, GLenum pname);
> +
> +    void GetQueryObject(JSContext* cx, WebGLQuery* query, GLenum pname,
> +                        JS::MutableHandle<JS::Value> retval)
> +    {

Not if the args have started wrapping.

::: dom/canvas/WebGLContextExtensions.cpp
@@ +16,2 @@
>  
> +/*static*/ const char*

Probably not. It's a little weird that you can't otherwise tell if a function definition is static otherwise, though.

::: dom/canvas/WebGLElementArrayCache.h
@@ +17,5 @@
>  
>  template<typename T>
>  struct WebGLElementArrayCacheTree;
>  
> +/* WebGLElementArrayCache implements WebGL element array buffer validation for

It seems wasteful, though if we're doing a block comment, it's because it's going to be long already. I could go either way.

Trying it both ways, maybe /*\n looks cleaner? I'll try a follow-up patch.

@@ +25,2 @@
>   *
> + * * Validate, to be called by WebGLContext::DrawElements, is where we use the

Fair. I was trying to follow markdown.

::: dom/canvas/WebGLExtensionDrawBuffers.cpp
@@ +52,5 @@
> +    if (mIsLost) {
> +        mContext->ErrorInvalidOperation("%s: Extension is lost.",
> +                                        "drawBuffersWEBGL");
> +        return;
> +    }

Yeah, I was copy-pasting this, since most extensions' names are too long to fit on one line here. This one barely fits.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +437,5 @@
>  
>  void
>  WebGLFramebuffer::DetachAllAttachments()
>  {
> +    for (size_t i = 0; i < mColorAttachments.Length(); i++) {

If `Length()` is non-trivial for an array, something weird's up.

::: dom/canvas/WebGLProgram.h
@@ +71,1 @@
>          if (!(mGeneration + 1).isValid())

I agree, but I had to draw the line between reformatting and refactoring somewhere.

::: dom/canvas/WebGLQuery.cpp
@@ -37,2 @@
>  {
> -    WebGLRefPtr<WebGLQuery>* targetSlot = mContext->GetQueryTargetSlot(mType, "WebGLQuery::IsActive()");

It's 104 chars if I don't split it. It's 97 if I split at the comma. Since just the declaration for refpointer vars is 35ish chars, it's pretty common for them to spill at least somewhat. Splitting the declaration from the definition is a relatively clean way to keep the function call on one line.

::: dom/canvas/WebGLRenderbuffer.cpp
@@ +185,3 @@
>      if (secondaryFormat) {
> +        gl->fRenderbufferStorage(LOCAL_GL_RENDERBUFFER, secondaryFormat, width,
> +                                 height);

There's a simplicity in keeping it more mechanical. Being more mechanical also comes with comparatively harsh edge cases, but it makes maintenance straight-forward.

If we added one more arg here, should we wrap then? What if we add an indent level? When does it become too long?

::: dom/canvas/WebGLTexture.h
@@ +243,4 @@
>      }
>  
> +    bool
> +    DoesMipmapHaveAllLevelsConsistentlyDefined(TexImageTarget texImageTarget) const;

80chars is a harsh mistress.
I don't care too much about this in these cases, though.

::: dom/canvas/WebGLVertexArrayFake.cpp
@@ +39,5 @@
>          }
>      }
>  
> +    for (size_t i = mAttribs.Length(); i < prevVertexArray->mAttribs.Length();
> +         ++i)

Hah, fair.

::: dom/canvas/WebGLVertexAttribData.h
@@ +75,1 @@
>  {

It's not whether it's in a header file, it's whether it's at the global scope, vs. nested.
(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> Fair. I was trying to follow markdown.

Oh. Well then leave it then.
 
> If `Length()` is non-trivial for an array, something weird's up.

Yeah, strict aliasing rules and C++ compilers are dumb.

> ::: dom/canvas/WebGLProgram.h
> @@ +71,1 @@
> >          if (!(mGeneration + 1).isValid())
> 
> I agree, but I had to draw the line between reformatting and refactoring
> somewhere.

Follow up TODO?
 
> There's a simplicity in keeping it more mechanical. Being more mechanical
> also comes with comparatively harsh edge cases, but it makes maintenance
> straight-forward.

Well can I configure emacs with the setting you used to wrap?

> If we added one more arg here, should we wrap then? What if we add an indent
> level? When does it become too long?
> 
> 80chars is a harsh mistress.
> I don't care too much about this in these cases, though.
 
My personal opinion is that we should break the 80 column limit. It's not like it's 1975 anymore. We have large, bitmap displays.
 
> It's not whether it's in a header file, it's whether it's at the global
> scope, vs. nested.
 
Ah, OK
Attached patch 0002-More-formatting-fixes.patch (obsolete) — Splinter Review
Alright, nits from previous, a couple more files, and all instances of `foo {*,&}bar` instead of `foo{*,&} bar` in dom/canvas/WebGL*.
Attachment #8524970 - Flags: review?(dglastonbury)
(In reply to Dan Glastonbury :djg :kamidphish from comment #5)
> (In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > Fair. I was trying to follow markdown.
> 
> Oh. Well then leave it then.
TBH, I think it looks better with `-`, so I reverted it.
>  
> > If `Length()` is non-trivial for an array, something weird's up.
> 
> Yeah, strict aliasing rules and C++ compilers are dumb.
> 
> > ::: dom/canvas/WebGLProgram.h
> > @@ +71,1 @@
> > >          if (!(mGeneration + 1).isValid())
> > 
> > I agree, but I had to draw the line between reformatting and refactoring
> > somewhere.
> 
> Follow up TODO?
I think I change this in some patches I'm working on to update our program linking validation and shader bypass stuff. (starting from yours)
>  
> > There's a simplicity in keeping it more mechanical. Being more mechanical
> > also comes with comparatively harsh edge cases, but it makes maintenance
> > straight-forward.
> 
> Well can I configure emacs with the setting you used to wrap?
I don't know emacs. :(
> 
> > If we added one more arg here, should we wrap then? What if we add an indent
> > level? When does it become too long?
> > 
> > 80chars is a harsh mistress.
> > I don't care too much about this in these cases, though.
>  
> My personal opinion is that we should break the 80 column limit. It's not
> like it's 1975 anymore. We have large, bitmap displays.
It's true, but now I want to see things side-by-side, both for diffs and for editing. With my setup, I get about 100 chars when editing on just one side of my screen.

Maybe we should raise the limit, but I do think we should have a limit for 99% of code.
Maybe 90 chars? It would be cool to get stats on the un-split line lengths in our code.
>  
> > It's not whether it's in a header file, it's whether it's at the global
> > scope, vs. nested.
>  
> Ah, OK
Comment on attachment 8524970 [details] [diff] [review]
0002-More-formatting-fixes.patch

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

::: dom/canvas/WebGLContextValidate.cpp
@@ +48,4 @@
>      case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
>      case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
>      case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:
> +        *out_blockWidth = 4;

So in release this will crash if we happened to get a NULL pointer. Is that acceptable?

@@ +79,5 @@
>      const char* name = WebGLContext::EnumName(glenum);
>      if (name)
>          ctx->ErrorInvalidEnum("%s: %s %s", InfoFrom(func, dims), msg, name);
> +    else {
> +        ctx->ErrorInvalidEnum("%s: %s 0x%04x", InfoFrom(func, dims), msg,

I think if you put { } around one clause then also put them on the other clause.

::: dom/canvas/WebGLObjectModel.h
@@ +303,4 @@
>          mHeight = height;
>      }
>  
> +    void setDimensions(WebGLRectangleObject* rect) {

SetDimensions?
Attachment #8524970 - Flags: review?(dglastonbury) → review+
Current revision.
Attachment #8523245 - Attachment is obsolete: true
Attachment #8524970 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/10a7d6c3fa97
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Target Milestone: mozilla37 → mozilla36
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.