903112 - Use MOZ_THIS_IN_INITIALIZER_LIST in gfx/

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

MSVC spams the following instances of C4355 in gfx:
> gfx/layers/basic/BasicLayersImpl.h(44) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicCanvasLayer.h(32) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/ipc/AsyncPanZoomController.cpp(174) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/ipc/AsyncPanZoomController.cpp(175) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicLayersImpl.h(44) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicColorLayer.cpp(17) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicLayersImpl.h(44) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicContainerLayer.h(181) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicLayersImpl.h(44) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicImageLayer.cpp(25) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicLayersImpl.h(44) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicLayersImpl.h(44) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicThebesLayer.h(22) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/client/ClientCanvasLayer.h(31) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicLayersImpl.h(44) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicThebesLayer.h(22) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/basic/BasicContainerLayer.h(181) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/client/ClientCanvasLayer.h(31) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/client/ClientColorLayer.cpp(18) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/client/ClientImageLayer.cpp(19) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/client/ClientContainerLayer.h(142) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/client/ClientContainerLayer.h(253) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/client/ClientThebesLayer.h(23) : warning C4355: 'this' : used in base member initializer list
> gfx/layers/client/ClientTiledThebesLayer.cpp(17) : warning C4355: 'this' : used in base member initializer list
> gfx/graphite2/src/Font.cpp(34) : warning C4355: 'this' : used in base member initializer list
> gfx/thebes/gfxPlatform.cpp(250) : warning C4355: 'this' : used in base member initializer list

graphite2 is a third-party library, so we shouldn't patch that locally, but we can suppress the rest of those (assuming they're safe) with MOZ_THIS_IN_INITIALIZER_LIST.

Comment 1

[Daniel Holbert [:dholbert]]

For reference -- comment 0's list is taken from a recent mozilla-central clobber-build on Windows: https://tbpl.mozilla.org/php/getParsedLog.php?id=26306706&tree=Mozilla-Central

Comment 2

[Daniel Holbert [:dholbert]]

Arnaud, any interest in taking this?

(Basically, we just need to replace "this" with "MOZ_THIS_IN_INITIALIZER_LIST()" in all of the above instances, and add #include "mozilla/Attributes.h" in any of those files that fail to compile.)

Comment 3

[Arnaud Sourioux [:Six]]

Daniel, thanks for thinking about me ;)
i'm back on monday, i will work on it

Comment 4

[Arnaud Sourioux [:Six]]

Attachment: replace this with MOZ_THIS_IN_INITIALIZER_LIST()

This should be ok but i request your feedback first before asking to a review peer.
tbpl is there:
https://tbpl.mozilla.org/?tree=Try&rev=8f9de77ba506
all warnings are gone for gfx except the one about graphite2 as you asked.

Comment 5

[Jonathan Kew [:jfkthame]]

FWIW, I just checked the case in graphite2, and confirmed that the usage there is valid - there's no attempt to -use- the incompletely-constructed object through its 'this' pointer here, it's only being stored in a member for later (safe) use.

I did -not- check the other cases to verify that the usage of 'this' is OK in those instances; ISTM that should be checked before we go ahead and suppress the warnings with the MOZ_... macro.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> FWIW, I just checked the case in graphite2, and confirmed that the usage
> there is valid - there's no attempt to -use- the incompletely-constructed
> object through its 'this' pointer here, it's only being stored in a member
> for later (safe) use.

(Per the end of comment 0, we probably don't want to bother fixing that one anyway, since any fix would get clobbered (or cause merge conflicts?) the next time we update to a new snapshot of that library, right?)

> I did -not- check the other cases to verify that the usage of 'this' is OK
> in those instances; ISTM that should be checked before we go ahead and
> suppress the warnings with the MOZ_... macro.

I fully agree (though I think it's likely that they're fine).

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 789496 [details] [diff] [review]
replace this with MOZ_THIS_IN_INITIALIZER_LIST()

>diff --git a/gfx/layers/basic/BasicCanvasLayer.h b/gfx/layers/basic/BasicCanvasLayer.h
> class BasicCanvasLayer : public CopyableCanvasLayer,
>                          public BasicImplData
> {
> public:
>   BasicCanvasLayer(BasicLayerManager* aLayerManager) :
>-    CopyableCanvasLayer(aLayerManager, static_cast<BasicImplData*>(this))
>+  CopyableCanvasLayer(aLayerManager, \
>+                      static_cast<BasicImplData*>(MOZ_THIS_IN_INITIALIZER_LIST()))

I don't think there's any need for the "\" there, is there?  We should be fine just adding a newline without needing to escape it, I'd expect. (Same elsewhere in the patch.)

Otherwise, looks good to me!

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 789496 [details] [diff] [review]
replace this with MOZ_THIS_IN_INITIALIZER_LIST()

>diff --git a/gfx/layers/basic/BasicCanvasLayer.h b/gfx/layers/basic/BasicCanvasLayer.h
>--- a/gfx/layers/basic/BasicCanvasLayer.h
>+++ b/gfx/layers/basic/BasicCanvasLayer.h
>@@ -8,33 +8,35 @@
> 
> #include "BasicLayersImpl.h"
> #include "nsXULAppAPI.h"
> #include "BasicLayers.h"
> #include "BasicImplData.h"
> #include "mozilla/layers/CanvasClient.h"
> #include "mozilla/Preferences.h"
> #include "CopyableCanvasLayer.h"
>+#include "mozilla/Attributes.h"

One nit: where possible, it's nice to group "mozilla/" includes together. So here, you'd probably want to insert the Attributes.h include right before the Preferences.h include.

>+++ b/gfx/layers/basic/BasicColorLayer.cpp
>@@ -1,25 +1,27 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>  * This Source Code Form is subject to the terms of the Mozilla Public
>  * 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/. */
> 
> #include "mozilla/layers/PLayerTransactionParent.h"
> #include "BasicLayersImpl.h"
>+#include "mozilla/Attributes.h"


Similarly: here, you'd want to stick the new #include next to the other one. (probably doesn't matter which one comes first; it looks like we're not really consistent about whether top-level "mozilla/" headers come before or after "mozilla/layers" headers.)

>--- a/gfx/thebes/gfxPlatform.cpp
>+++ b/gfx/thebes/gfxPlatform.cpp
>@@ -75,16 +75,17 @@
> #endif
> 
> #include "mozilla/Preferences.h"
> #include "mozilla/Assertions.h"
> #include "mozilla/Attributes.h"
> #include "mozilla/Mutex.h"
> 
> #include "nsIGfxInfo.h"
>+#include "mozilla/Attributes.h"

We've already got this #include, 4 lines above where you're adding it, so we don't need another copy of it. :)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 789496 [details] [diff] [review]
replace this with MOZ_THIS_IN_INITIALIZER_LIST()

One more nit:

>diff --git a/gfx/layers/basic/BasicThebesLayer.h b/gfx/layers/basic/BasicThebesLayer.h
> class BasicThebesLayer : public ThebesLayer, public BasicImplData {
> public:
>   typedef ThebesLayerBuffer::PaintState PaintState;
>   typedef ThebesLayerBuffer::ContentType ContentType;
> 
>   BasicThebesLayer(BasicLayerManager* aLayerManager) :
>-    ThebesLayer(aLayerManager, static_cast<BasicImplData*>(this)),
>-    mContentClient(nullptr)
>+  ThebesLayer(aLayerManager, \
>+              static_cast<BasicImplData*>(MOZ_THIS_IN_INITIALIZER_LIST())),
>+  mContentClient(nullptr)

When you modified this chunk, it looks like you (or your editor, on your behalf) reduced the indentation by 2 spaces. (Previously, "ThebesLayer(" was indented 2 spaces further than "BasicThebesLayer("; your changes remove that indentation).

It's better not to do that, to keep patches like this as targeted & mechanical as possible (and to avoid needlessly touching lines like "mContentClient(nullptr)", where you didn't actually modify the code -- only the indentation.)

This applies to most chunks of your patch -- it looks like you reduced the indentation nearly across-the-board for these invocations.

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #5)
> > FWIW, I just checked the case in graphite2, and confirmed that the usage
> > there is valid - there's no attempt to -use- the incompletely-constructed
> > object through its 'this' pointer here, it's only being stored in a member
> > for later (safe) use.
> 
> (Per the end of comment 0, we probably don't want to bother fixing that one
> anyway, since any fix would get clobbered (or cause merge conflicts?) the
> next time we update to a new snapshot of that library, right?)

Right - we shouldn't touch it locally. (And given that the code is correct, I don't think there's any strong reason to pester upstream about it, either.)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> I did -not- check the other cases to verify that the usage of 'this' is OK
> in those instances; ISTM that should be checked before we go ahead and
> suppress the warnings with the MOZ_... macro.

OK -- following up on this -- I did some investigation, and as far as I can tell, all of the "***Layer(aLayerManager, this)" invocations (virtually this entire patch) are fine -- they're passing their "this" pointer as a "void* aImplData" parameter to their superclass's constructor, which ends up passing it (still as a void*) to the top-level Layer() constructor, which just stores it as a void* member-var.

I checked this for the ColorLayer, ContainerLayer, ImageLayer, ReadbackLayer, ThebesLayer, CopyableCanvasLayer (and its superclass "CanvasLayer"), and RefLayer, which I think covers all of the ***Layer() invocations in this patch.

After that, we just have:
 * AsyncPanZoomController() passing "this" to mX/mY, of type AxisX/AxisY, which pass it to their superclass "Axis()", which just stores it as a member-var. (and calls a helper-function that doesn't use that member-var)
 * gfxPlatform passing "this" to mAzureCanvasBackendCollector (of type GfxInfoCollector, which just stores that constructor-arg as a member-var)

So: I'm confident that all of the "this" usages in this patch are safe, and we're OK to suppress the warning about them.

Comment 12

[Arnaud Sourioux [:Six]]

Attachment: replace this with MOZ_THIS_IN_INITIALIZER_LIST()

This one should be okay,
i took care of all Daniel's comments.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 790772 [details] [diff] [review]
replace this with MOZ_THIS_IN_INITIALIZER_LIST()

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

There are a couple of off-by-one whitespace nits; but more importantly, I wonder whether all the additions of #include <mozilla/Attributes.h> are actually necessary. It's a pretty basic header, so in many cases, I suspect it's already being included via another header and needn't be added here. Did you check these to confirm whether they're really needed?

::: gfx/layers/client/ClientContainerLayer.h
@@ +140,5 @@
>  
>  public:
>    ClientContainerLayer(ClientLayerManager* aManager) :
> +    ContainerLayer(aManager,
> +		    static_cast<ClientLayer*>(MOZ_THIS_IN_INITIALIZER_LIST()))

nit: the indent doesn't match here

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +15,5 @@
>  
>  
>  ClientTiledThebesLayer::ClientTiledThebesLayer(ClientLayerManager* const aManager)
> +  : ThebesLayer(aManager,
> +		 static_cast<ClientLayer*>(MOZ_THIS_IN_INITIALIZER_LIST()))

nit: and here

::: gfx/thebes/gfxPlatform.cpp
@@ +2004,5 @@
>  #endif
>  
>    InitLayersAccelerationPrefs();
>    return sComponentAlphaEnabled;
> +}

Looks like you've accidentally lost the final newline here; please restore it.

Comment 14

[Jonathan Kew [:jfkthame]]

Ah, after this was posted I realized that there are actually <tab> characters in those indents, as well as another instance in the patch, I think. Please avoid <tab>, and use spaces instead.

(https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Whitespace)

Comment 15

[Arnaud Sourioux [:Six]]

Attachment: replace this with MOZ_THIS_IN_INITIALIZER_LIST()

This should be better,
this is the first time i'm having troubles with indent using emacs...

tbpl is fine without any #include
https://tbpl.mozilla.org/?tree=Try&rev=155284e1eb24
but before landing it we should try it on other systems too

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +2004,5 @@
> >  #endif
> >  
> >    InitLayersAccelerationPrefs();
> >    return sComponentAlphaEnabled;
> > +}
> 
> Looks like you've accidentally lost the final newline here; please restore
> it.

Nope this means there was no newline at end of file and now there is one but i can remove it if you want.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Sourioux [:Six] from comment #15)
> This should be better,
> this is the first time i'm having troubles with indent using emacs...

That's probably because lots of our source-code files an emacs modeline at the top (in a comment), indicating how they want to be indented; but these files (ClientTiledThebesLayer.cpp at least) are missing that modeline.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Arnaud Sourioux [:Six] from comment #15)
> Created attachment 790886 [details] [diff] [review]
> replace this with MOZ_THIS_IN_INITIALIZER_LIST()
> 
> This should be better,
> this is the first time i'm having troubles with indent using emacs...
> 
> tbpl is fine without any #include
> https://tbpl.mozilla.org/?tree=Try&rev=155284e1eb24
> but before landing it we should try it on other systems too

Thanks. It builds OK for me on OS X, too, so I think this is probably fine.

> (In reply to Jonathan Kew (:jfkthame) from comment #13)
> > ::: gfx/thebes/gfxPlatform.cpp
> > @@ +2004,5 @@
> > >  #endif
> > >  
> > >    InitLayersAccelerationPrefs();
> > >    return sComponentAlphaEnabled;
> > > +}
> > 
> > Looks like you've accidentally lost the final newline here; please restore
> > it.
> 
> Nope this means there was no newline at end of file and now there is one but
> i can remove it if you want.

Ah, so it is - sorry, I interpreted that backwards. OK, although it's unrelated to the rest of this bug, let's accept this change, as I disapprove of lack-of-final-newline. :)

Comment 18

[Arnaud Sourioux [:Six]]

Attachment: replace this with MOZ_THIS_IN_INITIALIZER_LIST()

I didn't see that the bug message was still containing the try syntax...
i let review+ for this patch

In reply to Jonathan Kew (:jfkthame) from comment #17)
> (In reply to Arnaud Sourioux [:Six] from comment #15)
> > but before landing it we should try it on other systems too
>
> Thanks. It builds OK for me on OS X, too, so I think this is probably fine.

Just to be sure i did it anyway, don't want RyanVM to yell on me :)
https://tbpl.mozilla.org/?tree=Try&rev=4409a2df7182

As it's all green i add checkin-needed

(In reply to Jonathan Kew (:jfkthame) from comment #17)
> Ah, so it is - sorry, I interpreted that backwards. OK, although it's
> unrelated to the rest of this bug, let's accept this change, as I disapprove
> of lack-of-final-newline. :)

Yep i had to double check that the first time i saw it for this patch.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

(In reply to Arnaud Sourioux [:Six] from comment #18)
> Just to be sure i did it anyway, don't want RyanVM to yell on me :)

Hah \o/
https://hg.mozilla.org/integration/mozilla-inbound/rev/d959083ba5f6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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