957499 - Fix instances of "warning: 'mozilla::layers::[something]::SetCompositor' hides overloaded virtual function [-Woverloaded-virtual]"

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Daniel Holbert [:dholbert]]

Filing bug on these two related build warnings (which I noticed when building m-i with clang 3.4 today):

First warning:
{
TextureHostOGL.h:272:8: warning: 
      'mozilla::layers::SharedTextureSourceOGL::SetCompositor' hides overloaded virtual function
      [-Woverloaded-virtual]
  void SetCompositor(CompositorOGL* aCompositor);
       ^
../../dist/include/mozilla/layers/TextureHost.h:152:16: note: hidden overloaded virtual function
      'mozilla::layers::NewTextureSource::SetCompositor' declared here: type mismatch at 1st parameter
      ('mozilla::layers::Compositor *' vs 'mozilla::layers::CompositorOGL *')
  virtual void SetCompositor(Compositor* aCompositor) {}
               ^
}

Second warning:
{
MacIOSurfaceTextureHostBasic.h:41:8: warning: 
      'mozilla::layers::MacIOSurfaceTextureSourceBasic::SetCompositor' hides overloaded virtual function
      [-Woverloaded-virtual]
  void SetCompositor(BasicCompositor* aCompositor) {
       ^
../../dist/include/mozilla/layers/TextureHost.h:152:16: note: hidden overloaded virtual function
      'mozilla::layers::NewTextureSource::SetCompositor' declared here: type mismatch at 1st parameter
      ('mozilla::layers::Compositor *' vs 'mozilla::layers::BasicCompositor *')
  virtual void SetCompositor(Compositor* aCompositor) {}
               ^
}

Based on skimming the same function implementation in other classes, I'm pretty sure these functions want to take a Compositor* (to match the superclass signaature) and static_cast it to their own custom type. (CompositorOGL* or BasicCompositor*)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> Based on skimming the same function implementation in other classes, I'm
> pretty sure these functions want to take a Compositor* (to match the
> superclass signaature) and static_cast it to their own custom type.

Actually, the other classes I was referring to appear to be subclasses of TextureHost (which also has a SetCompositor method), whereas the classes here are subclasses of NewTextureSource. So maybe the semantics are different.

Still, I'll post a strawman patch.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1: make SetCompositor impls take a Compositor* and static_cast it

nical, does this make sense / is this what was intended?

(The patch fixes GrallocTextureSourceOGL, which I don't get a build warning for (maybe because it's not used on my platform?) but which I discovered by looking for other NewTextureSource subclasses.)

Comment 3

[Daniel Holbert [:dholbert]]

Alternately, if we really do intend for these methods to take their own custom Compositor type, then maybe we should remove the (no-op) NewTextureSource method [1]?

(I tried removing the NewTextureSource method locally, but it wouldn't compile because we invoke that method on objects of (abstract) type NewTextureSource, inside of BufferTextureHost::SetCompositor().  [2]

(Though I suspect that invocation might be a no-op currently, because the NewTextureSource subclasses don't provide a SetCompositor implementation, so they just get the empty impl from NewTextureSource.)

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.h?rev=033f40f8414d&mark=152-152#134
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.cpp?rev=033f40f8414d&mark=384-386#378

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8356992 [details] [diff] [review]
fix v1: make SetCompositor impls take a Compositor* and static_cast it

FWIW, I did end up finding some other NewTextureSource classes that use static_cast like "fix v1" does. They're all subclasses of DataTextureSource (which itself is a subclass of NewTextureSource):

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/TextureD3D9.cpp?rev=dd1d484f6d22#1210
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.cpp?rev=033f40f8414d#392
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp?rev=ff328828dfda#271

So I think the approach is probably correct. Hence, upgrading feedback request to review request.

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 8356992 [details] [diff] [review]
fix v1: make SetCompositor impls take a Compositor* and static_cast it

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

You are right, this is much better.

::: gfx/layers/basic/MacIOSurfaceTextureHostBasic.h
@@ +6,5 @@
>  #ifndef MOZILLA_GFX_MACIOSURFACETEXTUREHOST_BASIC_H
>  #define MOZILLA_GFX_MACIOSURFACETEXTUREHOST_BASIC_H
>  
>  #include "mozilla/layers/TextureHostBasic.h"
> +#include "BasicCompositor.h"

nit: I would prefer to move the implemenation of SetCompositor to the .cpp and keep the forward declaration instead adding an #include in the header.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2 [r=nical]

(In reply to Nicolas Silva [:nical] from comment #5)
> nit: I would prefer to move the implemenation of SetCompositor to the .cpp
> and keep the forward declaration instead adding an #include in the header.

Addressed that in this version.

Two other things that I changed here:
 - I fixed some missing parens in my static_cast in GrallocTextureHost.cpp (which caused a compile error on B2G)
 - I fixed this bug's SetCompositor issue in one other NewTextureSource subclass: MacIOSurfaceTextureHostOGL.

Comment 7

[Daniel Holbert [:dholbert]]

Try run with the final patch, except without the MacIOSurfaceTextureHostOGL tweak (which I found&addressed after starting this Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=42a4e305de80

And here's a sanity-check mac-only Try run with the MacIOSurfaceTextureHostOGL tweak as well:
 https://tbpl.mozilla.org/?tree=Try&rev=9f0f54cf2a88

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2a [r=nical]

Updated patch to adjust the parameter's type (s/CompositorOGL/Compositor/) in its decl within MacIOSurfaceTextureHostOGL, to fix build bustage on Try:
 https://tbpl.mozilla.org/?tree=Try&rev=8bd8a70d4575

Comment 9

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b150c4d305a

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2b150c4d305a