Closed Bug 997699 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(3 files)

Just like we do with TextureChild/Parent.
+1 !

And by the way, TextureClient.h should not #include "mozilla/layers/PTextureChild.h"
Attachment #8409335 - Flags: review?(bjacob)
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.
Attachment #8409338 - Flags: review?(bjacob)
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.
Attachment #8409335 - Flags: review?(bjacob) → review+
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.
Attachment #8409338 - Flags: review?(bjacob) → review+
landed part 1) https://hg.mozilla.org/integration/mozilla-inbound/rev/dc10c7c748ee
Whiteboard: [leave open]
Attachment #8412689 - Flags: review?(bjacob)
Attachment #8412689 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/mozilla-central/rev/f727dc726d3d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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? :)
Flags: needinfo?(bjacob)
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.
Flags: needinfo?(bjacob)
You need to log in before you can comment on or make changes to this bug.