919022 - CompositableClient.h: 'virtual mozilla::TemporaryRef<mozilla::layers::BufferTextureClient> mozilla::layers::CompositableClient::CreateBufferTextureClient(mozilla::gfx::SurfaceFormat, mozilla::layers::TextureFlags)' was hidden

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Daniel Holbert [:dholbert]]

I noticed this build warning spammed twice during my mozilla-inbound build today (once for each .cpp file that #includes CanvasClient.h, directly or indirectly):

{
layers/CompositableClient.h: 'virtual mozilla::TemporaryRef<mozilla::layers::BufferTextureClient> mozilla::layers::CompositableClient::CreateBufferTextureClient(mozilla::gfx::SurfaceFormat, mozilla::layers::TextureFlags)' was hidden

    CreateBufferTextureClient(gfx::SurfaceFormat aFormat, TextureFlags aFlags);
    ^

layers/CanvasClient.h:86:3: warning:   by 'virtual mozilla::TemporaryRef<mozilla::layers::BufferTextureClient> mozilla::layers::CanvasClient2D::CreateBufferTextureClient(mozilla::gfx::SurfaceFormat)' [-Woverloaded-virtual]

    CreateBufferTextureClient(gfx::SurfaceFormat aFormat) MOZ_OVERRIDE;
    ^
}

I believe the issue is that:

 - The parent class has two functions named CreateBufferTextureClient() with different args.
 - The child class only overrides *one* of them.
 - This "hides" the other one, from the perspective of the child class (i.e. any hypothetical invocation of CreateBufferTextureClient() on the child class will be unable to invoke the other inherited method, though it'd work if you cast the instance to have the parent class's type.)

...and the compiler thinks this is something it should warn about, messily.

I don't think this is a real code bug -- it looks like we intentionally only overrode one method, since the overriding implementation explicitly invokes the other method, using the parent class's name. So that's good. It'd be nice to convince the compiler that we're OK here, though, so that it quiets down.

Comment 1

[Daniel Holbert [:dholbert]]

Meant to say -- the parent's second copy of this method (the 2-arg version of "CompositableClient::CreateBufferTextureClient"), as well as the the child override for the 1-arg version, were both added in this commit for bug 900133:
  http://hg.mozilla.org/mozilla-central/rev/64ab855db547
so I'm pretty sure that's what introduced this warning.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Do not overload CreateBufferTextureClient

Comment 3

[Nick Cameron [:nrc]]

Comment on attachment 810632 [details] [diff] [review]
Do not overload CreateBufferTextureClient

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

::: gfx/layers/client/ImageClient.cpp
@@ -284,5 @@
> -TemporaryRef<BufferTextureClient>
> -ImageClientSingle::CreateBufferTextureClient(gfx::SurfaceFormat aFormat)
> -{
> -  return CompositableClient::CreateBufferTextureClient(aFormat,
> -    mTextureFlags | TEXTURE_FLAGS_DEFAULT);

You seem to be changing the semantics here, a call to CreateBufferTextureClient with one arg will no longer get mTextureFlags. Is that OK?

Comment 4

[Daniel Holbert [:dholbert]]

[adding needinfo=nical, per comment 3]

Comment 5

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #3)
> Comment on attachment 810632 [details] [diff] [review]
> Do not overload CreateBufferTextureClient
> 
> Review of attachment 810632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/ImageClient.cpp
> @@ -284,5 @@
> > -TemporaryRef<BufferTextureClient>
> > -ImageClientSingle::CreateBufferTextureClient(gfx::SurfaceFormat aFormat)
> > -{
> > -  return CompositableClient::CreateBufferTextureClient(aFormat,
> > -    mTextureFlags | TEXTURE_FLAGS_DEFAULT);
> 
> You seem to be changing the semantics here, a call to
> CreateBufferTextureClient with one arg will no longer get mTextureFlags. Is
> that OK?

It does, the one arg version gets mTextureFlags | aFlags
see line 281:

return CompositableClient::CreateBufferTextureClient(aFormat, mTextureFlags | aFlags);

Comment 6

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03dfed609009

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/03dfed609009