997699 - Move CompsoitableChild/Parent to CompositableClient/Host.cpp to not expose them.

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

[Nicolas Silva [:nical]]

Just like we do with TextureChild/Parent.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

+1 !

And by the way, TextureClient.h should not #include "mozilla/layers/PTextureChild.h"

Comment 2

[Nicolas Silva [:nical]]

Attachment: 1) Move CompositableChild to the .cpp

Comment 3

[Nicolas Silva [:nical]]

Attachment: 2) Move CompositableParent to the .cpp

I had to move some of the members of CompositableParent into CompositableHost so that outside code don't chat directly with the ipdl actor anymore (which is trivial since there's a 1-1 relationship between the two) I also found and removed unused members in the process.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8409335 [details] [diff] [review]
1) Move CompositableChild to the .cpp

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +12,5 @@
>  #include "mozilla/Hal.h"
>  #include "mozilla/dom/ScreenOrientation.h"  // for ScreenOrientation
>  #include "mozilla/dom/TabChild.h"       // for TabChild
>  #include "mozilla/hal_sandbox/PHal.h"   // for ScreenConfiguration
> +#include "mozilla/layers/CompositableClient.h"  // for CompositableClient, etc

Rather than to bother updating these stupid IWYU-generated comments, you may as well remove them as you go.

::: gfx/layers/client/CompositableClient.cpp
@@ +47,5 @@
> +      mCompositableClient->mCompositableChild = nullptr;
> +    }
> +  }
> +
> +  CompositableClient* mCompositableClient;

Even though this is local to a cpp file, I still appreciate having data members private. Especially as they used to be private. Up to you.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8409338 [details] [diff] [review]
2) Move CompositableParent to the .cpp

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

::: gfx/layers/composite/CompositableHost.cpp
@@ +58,5 @@
> +      mHost->Detach(nullptr, CompositableHost::FORCE_DETACH);
> +    }
> +  }
> +
> +  RefPtr<CompositableHost> mHost;

Same comment about members being private, as on the previous patch. Up to you.

Comment 6

[Nicolas Silva [:nical]]

landed part 1) https://hg.mozilla.org/integration/mozilla-inbound/rev/dc10c7c748ee

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/241760451193

Comment 9

[Nicolas Silva [:nical]]

Attachment: 3) Fixup #includes

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f727dc726d3d

Comment 12

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

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Benoit, comment 12 landed for 32, but comment 10 landed for 31. Do you think we should land the follow-up on Aurora too? I've confirmed that it uplifts cleanly. Nicolas is away, so if you don't mind, can you please request it if you agree? :)

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

It's not worth requesting uplift for this. Comment 12 is a nice fixup as it prevents slowing down compiles with excessive includes (a #include is more expensive if it is itself in a header file), and it prevents accidentally relying on these #includes that we now consider as implementation details (whence the move to .cpp files) but neither of these benefits is very meaningful on aurora and stable channels. That's mostly beneficial on mozilla-central for active development.

Note, if it compiles cleanly, it's a very safe patch to uplift (the only risk was potentially failing to compile). Just not really worth it.