1660470 - IPDL: It should be possible to avoid including *MessageUtils.h in the generated header files

Status: RESOLVED FIXED

(Core :: IPC, task)

Description

[Simon Giesecke [:sg] [he/him]]

In most cases, it is not necessary to include *MessageUtils.h files such as mozilla/widget/WidgetMessageUtils.h in the generate protocol header files, they are only required for the code generated in the child/parent cpp files. However, these are complex to parse and impact build times significantly, so avoiding that would benefit the build.

In the IPDL syntax, this can be achieved by specifying separate includes in using directives, e.g.

using struct LookAndFeelInt from "mozilla/LookAndFeel.h" ipcutil "mozilla/widget/WidgetMessageUtils.h";

instead of

using struct LookAndFeelInt from "mozilla/widget/WidgetMessageUtils.h";

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Introduce separate ipcutil argument for using directives. r=nika

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Add missing include directives/forward declarations. r=nika

Depends on D87864

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Reduce includes for LayersMessageUtils.h. r=nika

Depends on D87865

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Reduce includes for WidgetMessageUtils.h. r=nika

Depends on D87866

Comment 5

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I'll defer to Nika's judgment here, but to me it doesn't make a lot of sense to add this into the existing "using" statement, which like Nika pointed out has the issue that you create super long lines. It seems like what you really want here is a way to include arbitrary headers into the generated CPP file.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #5)

I'll defer to Nika's judgment here, but to me it doesn't make a lot of sense to add this into the existing "using" statement, which like Nika pointed out has the issue that you create super long lines. It seems like what you really want here is a way to include arbitrary headers into the generated CPP file.

I think that could be cleaner. The reasoning I thought of for doing it with the using statements was to have the specific include associated with the type it's used for so that it's easier to remove unnecessary includes, but that's not a super important reason.

I'd be interested in a potentially-simpler cpp_include statement or something like that.

Comment 7

[Simon Giesecke [:sg] [he/him]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #6)

(In reply to Andrew McCreight [:mccr8] from comment #5)

I'll defer to Nika's judgment here, but to me it doesn't make a lot of sense to add this into the existing "using" statement, which like Nika pointed out has the issue that you create super long lines. It seems like what you really want here is a way to include arbitrary headers into the generated CPP file.

I think that could be cleaner.
The reasoning I thought of for doing it with the using statements was to have the specific include associated with the type it's used for

Yes, exactly, that's was one major reason for me to add it as an argument to using, the other being consistency with how the include files are specified otherwise.

so that it's easier to remove unnecessary includes, but that's not a super important reason.

Yes, that's one specific benefit. In general, it makes it clearer why some particular file needs to be included. C++ doesn't have that for it's include directives though, so maybe it's a futile attempt to improve on that here. However, then you could argue that specifying any include file in using doesn't make sense, since you could always replace:

using struct LookAndFeelInt from "mozilla/widget/WidgetMessageUtils.h";

by

include  "mozilla/widget/WidgetMessageUtils.h";
using struct LookAndFeelInt;

I'd be interested in a potentially-simpler cpp_include statement or something like that.

That's an alternative.

However, I think I remember that nika told me at some point the the existence of include is only a workaround for rare cases, and cpp_include would then add something in line with that.

I think overall I would do one the following alternatives:

• Do it like suggest in comment #0, and keep include as a workaround for rare cases
• Get rid of specifying includes in using completely, and change everything to include & cpp_include

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

If I understand correctly, the core problem here has to do with cases where one (small) header file is needed to define the type, but another (larger) one is needed for a manual ParamTraits instance. So, another approach would be to work on decreasing the number of manual ParamTraits instances, which is something we'd like to do anyway at some point to support languages other than C++.

For example, a lot of these are just plain structs; if the reason for that is because IPDL structs require accessor methods, we could expose the fields directly. (The () noise I had to add for bug 1657401 isn't great.) There are also instances of ContiguousEnumSerializer, but that shouldn't need to require a lot of headers besides the type itself.

Also, the headers we generate from .ipdlh have a lot of includes they may not need, and it's not possible to get just the types and not the ParamTraits declarations and their IPC header dependencies (which is basically the same problem as this bug but in the other direction); I don't know if that's a factor discouraging or preventing people from using IPDL structs, but if it is, it would be possible to generate separate headers and/or prune the includes.

I also noticed the IMPL_PARAMTRAITS_BY_SERDE macro — apparently we're exporting serde trait impls as C functions and calling them from ParamTraits in C++, which seems… convoluted.

Comment 9

[Simon Giesecke [:sg] [he/him]]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #8)

If I understand correctly, the core problem here has to do with cases where one (small) header file is needed to define the type, but another (larger) one is needed for a manual ParamTraits instance.

Yes, that's also my understanding. I don't know exactly why, but it seems that parsing the ParamTraits specializations (or maybe some of their dependent headers; they don't look that complex by themselves, but I am not a compiler) takes a significant amount of time, as my clang-build-analyzer analyses showed.

So, another approach would be to work on decreasing the number of manual ParamTraits instances, which is something we'd like to do anyway at some point to support languages other than C++.

I think both approaches could be combined. IIUC, the ParamTraits specializations could be reduced but not eliminated, so that approach wouldn't solve the problem on its own completely, right?

Also, the headers we generate from .ipdlh have a lot of includes they may not need, and it's not possible to get just the types and not the ParamTraits declarations and their IPC header dependencies (which is basically the same problem as this bug but in the other direction);

Ah, yes, that's another factor. I guess that can be split up quite easily into separate headers being generated and the ParamTraits included only where necessary.

Comment 10

[Simon Giesecke [:sg] [he/him]]

nika, what do you think? Should I pursue either of the alternatives mentioned at the end of https://bugzilla.mozilla.org/show_bug.cgi?id=1660470#c7, and if yes, which one? Should we file additional bugs for the aspects mentioned by jld in https://bugzilla.mozilla.org/show_bug.cgi?id=1660470#c8?

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Currently the include keyword includes the specified header into the .h file for the protocol, rather than the .cpp file. I don't really see the purpose of that, as the using declarations ought to contain the information needed to forward-declare or import each type we're using.

Perhaps we could avoid adding new headers or making substantial changes at all, and make the include keyword import those files in cpps rather than headers?

In general, I think I'd prefer to avoid doing a full on refactoring where we make everything be attached to a using statement, or move everything to instead use include statements. An intermediate ground where using lets you import types (which potentially will lead to an #include in the header if it's not forward-declareable due to using class or using struct), and include lets you import extra types required for serializing, seems appealing to me.

Comment 12

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Include C++ header files only from cpp file. r=nika

Comment 13

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including GfxMessageUtils.h from header files. r=nika

Comment 14

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including IPCMessageUtils.h from header files. r=nika

Comment 15

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including GamepadMessageUtils.h from header files. r=nika

Comment 16

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including VRMessageUtils.h from header files. r=nika

Comment 17

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including WidgetMessageUtils.h from header files. r=nika

Comment 18

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including DocShellMessageUtils.h from header files. r=nika

Comment 19

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including NeckoMessageUtils.h from header files. r=nika

Comment 20

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including ClientIPCUtils.h from header files. r=nika

Comment 21

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including dom/cache/IPCUtils.h from header files. r=nika

Comment 22

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including ErrorIPCUtils.h from header files. r=nika

Comment 23

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including DataStorageIPCUtils.h from header files. r=nika

Comment 24

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including PermissionDelegateIPCUtils.h from header files. r=nika

Comment 25

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including nsGUIEventIPC.h from header files. r=nika

Comment 26

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including LayersMessageUtils.h from header files. r=nika

Comment 27

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including FetchIPCUtils.h from header files. r=nika

Comment 28

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Move IPDLParamTraits for FileDescriptor out of FileDescriptor.h. r=nika

Depends on D93248

Comment 29

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split TelemetryTypes.h from TelemetryComms.h. r=nika

Depends on D93321

Comment 30

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split ShmemMessageUtils.h from Shmem.h. r=nika

Depends on D93542

Comment 31

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including ProtocolUtils.h from header files. r=nika

Depends on D93543

Comment 32

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Move void_t/null_t to a new IPCCore.h header. r=nika

Depends on D93544

Comment 33

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split BackgroundMessageUtils.h from BackgroundUtils.h. r=nika

Depends on D93546

Comment 34

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split RegistryTypes.h from RegistryMessageUtils.h. r=nika

Depends on D93548

Comment 35

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split SerializedStructuredCloneBuffer.h from IPCMessageUtils.h. r=nika

Depends on D93549

Comment 36

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including PermissionMessageUtils.h from header files. r=nika

Depends on D93550

Comment 37

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split D3DTypes.h from D3DMessageUtils.h. r=nika

Depends on D93552

Comment 38

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split HalSensorMessageUtils.h from HalSensor.h. r=nika

Depends on D93553

Comment 39

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including ServiceWorkerIPCUtils.h from header files. r=nika

Depends on D93554

Comment 40

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including BindingIPCUtils.h from header files. r=nika

Depends on D93555

Comment 41

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including CSPMessageUtils.h from header files. r=nika

Depends on D93556

Comment 42

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including ReferrerInfoUtils.h from header files. r=nika

Depends on D93557

Comment 43

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including PropertyBagUtils.h from header files. r=nika

Depends on D93558

Comment 44

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including URIUtils.h from header files. r=nika

Depends on D93559

Comment 45

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split SurfaceDescriptor.h from ShadowLayerUtils.h. r=nika

Depends on D93560

Comment 46

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split VideoBridgeSource.h from VideoBridgeUtils.h. r=nika

Depends on D93561

Comment 47

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including MediaSessionIPCUtils.h from header files. r=nika

Depends on D93562

Comment 48

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Avoid including MediaControlIPC.h from header files. r=nika

Depends on D93563

Comment 49

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split JSActorMessageUtils.h from JSActor.h. r=nika

Depends on D93564

Comment 50

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split ProcessLink.h from MessageLink.h. r=nika

Depends on D93565

Comment 51

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Remove unnecessary includes from MessageChannel.h and MessageLink.h. r=nika

Depends on D93566

Comment 52

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Split Endpoint.h and ProtocolMessageUtils.h from ProtocolUtils.h. r=nika

Depends on D93567

Comment 53

[Simon Giesecke [:sg] [he/him]]

I have some numbers for two scenarios doing the following to show the benefits for incremental builds:

• full build
• touch ipc/glue/IPCMesssageUtils.h (scenario 1) resp. gfx/layers/ipc/LayersMessageUtils.h (scenario 2)
• clear ccache (otherwise we would have to actually modify the touched files to see a meaningful effect)
• build again (incrementally)

Scenario 1:

metric        | before | after | delta
------------- | ------ | ----- | -----
wall time     |   465s |  393s | -15%
rebuilt TUs   |   1025 |   831 | -19%
frontend time |  4824s | 4003s | -17%
backend time  |  2523s | 2169s | -14%

Scenario 2:

metric        | before | after | delta
------------- | ------ | ----- | -----
wall time     |   239s |  143s | -40%
rebuilt TUs   |    425 |   219 | -48%
frontend time |  2421s | 1313s | -46%
backend time  |  1203s |  657s | -45%

Comment 54

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Move ParamTraits specializations with extra dependencies out of IPCMessageUtils.h. r=nika

Depends on D93568

Comment 55

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Remove unnecessary includes from chromium ipc headers. r=nika

Depends on D94765

Comment 56

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Remove unnecessary includes from xpcpublic.h. r=nika

Depends on D94866

Comment 57

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Remove unnecessary includes from nsIWidget.h. r=jhorak

Depends on D94867

Comment 58

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Nika doesn't have enough time to review these questions, so I said I'd take over. I'd like it if you would please answer some high level questions to orient myself as to what is happening here, as I haven't been paying attention to this bug since I first looked at it at the end of August. My apologies if this is retreading ground you've already gone over with Nika. Thanks.

What is the goal of these patches? Is it to reduce build times or are there other additional goals?

What is the unifying principle for these patches? As initially filed, you are talking about changing how things are structured so that serialization code isn't included in headers, but now I'm looking at, say, "Remove unnecessary includes from xpcpublic.h" and it isn't obviously related. Are these cleanups related to the initial goal or has the scope of the bug expanded? Could some of these be split into a separate bug? The first of the patches that support your main goal (say, one of the "Avoid including FooUtils.h" patches") should describe that goal and how these patches are supporting it. For the other patches that are not obviously related to the apparent goal of these patches of not including IPC serialization functions in generated headers, the bug summary should say how they support the initial goal. If they are not really related, it would be nice if they got moved into a new bug to reduce the cognitive overhead of trying to understand what the body of patches here is achieving.

How were these patches created? Did you write them entirely by hand or are they generated by some kind of tool?

The summary of the bug says you are "removing unnecessary includes", but some of these patches are doing a lot more than that. They are reordering the existing includes and adding new includes. What guidelines are you following for deciding on the order for the includes? Why are you adding new includes? Presumably the goal is that if you add one header, you can remove another one, but how did you decide when that would be beneficial? Similarly, how did you decide that a class could be forward declared instead of needing the full declaration from a header file?

Comment 59

[Simon Giesecke [:sg] [he/him]]

(In reply to Andrew McCreight [:mccr8] from comment #58)

Nika doesn't have enough time to review these questions, so I said I'd take over.

Thanks a lot!

I'd like it if you would please answer some high level questions to orient myself as to what is happening here, as I haven't been paying attention to this bug since I first looked at it at the end of August. My apologies if this is retreading ground you've already gone over with Nika. Thanks.

Well, my apologies that a lot of the conversations around that were private conversations with Nika on Matrix, without wrapping them up here.

What is the goal of these patches? Is it to reduce build times or are there other additional goals?

The immediate goal is to reduce build times as described by the metrics in https://bugzilla.mozilla.org/show_bug.cgi?id=1676346#c0, but see my comment below.

What is the unifying principle for these patches? As initially filed, you are talking about changing how things are structured so that serialization code isn't included in headers, but now I'm looking at, say, "Remove unnecessary includes from xpcpublic.h" and it isn't obviously related. Are these cleanups related to the initial goal or has the scope of the bug expanded?

Yes, that's right, the scope has evolved. While my original goal was to reduce (re)build times, I increasingly found the include dependencies to be very messy in a wider sense. So a related goal of more patches is also to clean up (include) dependencies for the sake of better understandability and maintainability. And one step for that is to make sure that the declared include dependencies correspond to the actual dependencies. That I should probably elaborate more on this maybe on another meta-bug to file.

Could some of these be split into a separate bug? The first of the patches that support your main goal (say, one of the "Avoid including FooUtils.h" patches") should describe that goal and how these patches are supporting it. For the other patches that are not obviously related to the apparent goal of these patches of not including IPC serialization functions in generated headers, the bug summary should say how they support the initial goal. If they are not really related, it would be nice if they got moved into a new bug to reduce the cognitive overhead of trying to understand what the body of patches here is achieving.

Some of the patches indeed should better move to a separate bug blocking the same meta bug 1676346. I'll take care of that, but maybe only next week.

How were these patches created? Did you write them entirely by hand or are they generated by some kind of tool? The summary of the bug says you are "removing unnecessary includes", but some of these patches are doing a lot more than that. They are reordering the existing includes and adding new includes. What guidelines are you following for deciding on the order for the includes? Why are you adding new includes?

"Removing unnecessary includes" is probably not accurate, "fix includes" is usually more appropriate. If an include is really just an extra include, it could be simply removed, but usually something included indirectly is used and must be included instead. In a number of cases, and :andi can tell a tale of that, it's even relying on includes from another translation unit that happens to be in the same unified translation unit. So, it's about fixing the includes in the sense of what include-what-you-use intends to do. However, include-what-you-use is not perfect. So for some of the fixes, I used the output from include-what-you-use as a basis (as others seem to have done before as it seems, as existing // for gfxFontVariation style comments show).

Presumably the goal is that if you add one header, you can remove another one, but how did you decide when that would be beneficial?

It's more the other way round: If I remove a header, I oftentimes need to add another one or two or three or ... And removing a header is, in my understanding, the right thing, if nothing is used from that header. There may be exceptions to that where an "umbrella header" is included, whose sole purpose is to pull in a number of other related headers. (FWIW, include-what-you-use can be told to recognize that via some pragmas, which, again, are already used in some parts of the code base).

Similarly, how did you decide that a class could be forward declared instead of needing the full declaration from a header file?

A bit of trial and error combined with some pattern recognition. In most cases, include-what-you-use is right when it says a forward declaration is sufficient. There are some exceptions that I can't grasp fully yet. In case the build broke, I re-added the header. But in quite some cases, it wrongly assumes that the full definition is needed, e.g. in connection with nsCOMPtr<T> and RefPtr<T>. And indeed, a full definition is needed to instantiate the constructors and destructors of those, so these are typical cases where I moved methods, in particular constructors/destructors of classes that have such data members, to the cpp file to avoid pulling in another header.

As a final note right now, one thing include-what-you-use systematically gets "wrong" is that symbols referenced in the definition of the macro don't count as "used" where the macro is defined, but where the macro is used. But it's arguable if that's actually wrong, I guess there are cases where either thing is desirable or even possible. There may certainly be some disputable decisions I made in this regard, be it by oversight or by opinion.

Comment 60

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Thanks for the explanations. I think I've reviewed all of the patches for the IPDL message utils related patches, or asked for revisions, or Nika's asked for revisions. The remaining patches look like they should be in another bug, so I'll wait to look at them in depth until you have a chance to move them to another bug.

Comment 61

[Simon Giesecke [:sg] [he/him]]

(In reply to Andrew McCreight [:mccr8] from comment #60)

Thanks for the explanations. I think I've reviewed all of the patches for the IPDL message utils related patches, or asked for revisions, or Nika's asked for revisions. The remaining patches look like they should be in another bug, so I'll wait to look at them in depth until you have a chance to move them to another bug.

Thanks a lot! I moved the three open revisions to Bug 1677466 now. I'll address yours and Nika's comments this week.

Comment 62

[Phabricator Automation]

Comment on attachment 9181631 [details]
Bug 1660470 - Remove unnecessary includes from MessageChannel.h and MessageLink.h. r=nika

Revision D93567 was moved to bug 1677466. Setting attachment 9181631 [details] to obsolete.

Comment 63

[Phabricator Automation]

Comment on attachment 9181632 [details]
Bug 1660470 - Split Endpoint.h and ProtocolMessageUtils.h from ProtocolUtils.h. r=nika

Revision D93568 was moved to bug 1677466. Setting attachment 9181632 [details] to obsolete.

Comment 64

[Phabricator Automation]

Comment on attachment 9183207 [details]
Bug 1660470 - Move ParamTraits specializations with extra dependencies out of IPCMessageUtils.h. r=nika

Revision D94459 was moved to bug 1677466. Setting attachment 9183207 [details] to obsolete.

Comment 65

[Phabricator Automation]

Comment on attachment 9184042 [details]
Bug 1660470 - Remove unnecessary includes from chromium ipc headers. r=nika

Revision D94866 was moved to bug 1677466. Setting attachment 9184042 [details] to obsolete.

Comment 66

[Phabricator Automation]

Comment on attachment 9184043 [details]
Bug 1660470 - Remove unnecessary includes from xpcpublic.h. r=nika

Revision D94867 was moved to bug 1677541. Setting attachment 9184043 [details] to obsolete.

Comment 67

[Phabricator Automation]

Comment on attachment 9184045 [details]
Bug 1660470 - Remove unnecessary includes from nsIWidget.h. r=jhorak

Revision D94869 was moved to bug 1677542. Setting attachment 9184045 [details] to obsolete.

Comment 68

[Andrew McCreight [:mccr8] (PTO until 12-2)]

It would be good if you posted some kind of mini-guide to the proper way to do using and include for IPDL files to dev-platform. That way we'll have something to refer people to.

Also, what do these changes from "using foo" to "using class foo" accomplish?

Comment 69

[Simon Giesecke [:sg] [he/him]]

(In reply to Andrew McCreight [:mccr8] from comment #68)

It would be good if you posted some kind of mini-guide to the proper way to do using and include for IPDL files to dev-platform. That way we'll have something to refer people to.

Well, I think a guide should better be in-tree documentation?

More generally, I think the coding style should have more comprehensive guidelines on when/how/where to include and/or forward declare things rather than just saying Forward-declare classes in your header files, instead of including them, whenever possible. (There's one small hint beyond that right now, but that's only the tip of the iceberg)

I'll file another bug for that. When that's done, a heads-up to dev-platform is definitely a good idea (and we can iterate based on that if necessary).

Also, what do these changes from "using foo" to "using class foo" accomplish?

I think I might have misunderstood something here. I originally thought that using foo would include the header in the generated header file, while using class foo would only place a forward declaration in the generated header file. But there seems to be more logic involved deciding that. Any hints on that are appreciated :)

Comment 70

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Simon Giesecke [:sg] [he/him] from comment #69)

Well, I think a guide should better be in-tree documentation?

Yes, that would certainly be better. I just wasn't sure where it would live. I was thinking more of a guide for IPDL which doesn't quite fit with the C++ style guide.

I think I might have misunderstood something here. I originally thought that using foo would include the header in the generated header file, while using class foo would only place a forward declaration in the generated header file. But there seems to be more logic involved deciding that. Any hints on that are appreciated :)

I didn't know that was even a thing. I guess I should look over how that might work.

Comment 71

[Andrew McCreight [:mccr8] (PTO until 12-2)]

It looks like the logic for using a forward declaration or an include is in the visitUsingStmt method of _GenerateProtocolActorCode, which seems to come down to the canBeForwardDeclared method of UsingStmt, which... just seems to check if it is a struct or class. Weird. visitUsingStmt also requires that you specify a header.

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93fabad45659
Add missing include directives/forward declarations. r=nika
https://hg.mozilla.org/integration/autoland/rev/be2a7a17d162
Include C++ header files only from cpp file. r=nika
https://hg.mozilla.org/integration/autoland/rev/8335f089e0d9
Avoid including GfxMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/48b0a7ccbe8a
Avoid including IPCMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/1091038d01df
Avoid including GamepadMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/024ec63a0cba
Avoid including VRMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/e6bd12dabc99
Avoid including WidgetMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/d324ca922186
Avoid including DocShellMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/b35f6638db0c
Avoid including NeckoMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/a2b72f7ed2a5
Avoid including ClientIPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/c3d87b114e56
Avoid including dom/cache/IPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/1f7590a3c06c
Avoid including ErrorIPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/b189a72b54a7
Avoid including DataStorageIPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/666727f2d0b2
Avoid including PermissionDelegateIPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/c587df43720e
Avoid including nsGUIEventIPC.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/c8b64b1dcaf0
Avoid including FetchIPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/73a763fb7428
Move IPDLParamTraits for FileDescriptor out of FileDescriptor.h. r=nika
https://hg.mozilla.org/integration/autoland/rev/d43822cff2f3
Split ShmemMessageUtils.h from Shmem.h. r=nika
https://hg.mozilla.org/integration/autoland/rev/b0ea0cb1d07a
Avoid including ProtocolUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/10d165db0694
Move void_t/null_t to a new IPCCore.h header. r=nika
https://hg.mozilla.org/integration/autoland/rev/0d3b1360e5e8
Split SerializedStructuredCloneBuffer.h from IPCMessageUtils.h. r=nika
https://hg.mozilla.org/integration/autoland/rev/c27dc74a06b3
Avoid including PermissionMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/79a76b7cbd44
Avoid including ServiceWorkerIPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/387c34582c02
Avoid including BindingIPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/69e2c95ff92b
Avoid including CSPMessageUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/59d9e21b9126
Avoid including ReferrerInfoUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/049348b7e0d8
Avoid including PropertyBagUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/b3f5b41533e2
Avoid including URIUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/f5374b13d619
Avoid including MediaSessionIPCUtils.h from header files. r=nika
https://hg.mozilla.org/integration/autoland/rev/f83b80d1aa07
Avoid including MediaControlIPC.h from header files. r=nika

Comment 73

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1660470 - Fix Linux AArch64 bustage.

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ce6d78452a1
Fix Linux AArch64 bustage. r=bustage-fix

Comment 75

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/93fabad45659
https://hg.mozilla.org/mozilla-central/rev/be2a7a17d162
https://hg.mozilla.org/mozilla-central/rev/8335f089e0d9
https://hg.mozilla.org/mozilla-central/rev/48b0a7ccbe8a
https://hg.mozilla.org/mozilla-central/rev/1091038d01df
https://hg.mozilla.org/mozilla-central/rev/024ec63a0cba
https://hg.mozilla.org/mozilla-central/rev/e6bd12dabc99
https://hg.mozilla.org/mozilla-central/rev/d324ca922186
https://hg.mozilla.org/mozilla-central/rev/b35f6638db0c
https://hg.mozilla.org/mozilla-central/rev/a2b72f7ed2a5
https://hg.mozilla.org/mozilla-central/rev/c3d87b114e56
https://hg.mozilla.org/mozilla-central/rev/1f7590a3c06c
https://hg.mozilla.org/mozilla-central/rev/b189a72b54a7
https://hg.mozilla.org/mozilla-central/rev/666727f2d0b2
https://hg.mozilla.org/mozilla-central/rev/c587df43720e
https://hg.mozilla.org/mozilla-central/rev/c8b64b1dcaf0
https://hg.mozilla.org/mozilla-central/rev/73a763fb7428
https://hg.mozilla.org/mozilla-central/rev/d43822cff2f3
https://hg.mozilla.org/mozilla-central/rev/b0ea0cb1d07a
https://hg.mozilla.org/mozilla-central/rev/10d165db0694
https://hg.mozilla.org/mozilla-central/rev/0d3b1360e5e8
https://hg.mozilla.org/mozilla-central/rev/c27dc74a06b3
https://hg.mozilla.org/mozilla-central/rev/79a76b7cbd44
https://hg.mozilla.org/mozilla-central/rev/387c34582c02
https://hg.mozilla.org/mozilla-central/rev/69e2c95ff92b
https://hg.mozilla.org/mozilla-central/rev/59d9e21b9126
https://hg.mozilla.org/mozilla-central/rev/049348b7e0d8
https://hg.mozilla.org/mozilla-central/rev/b3f5b41533e2
https://hg.mozilla.org/mozilla-central/rev/f5374b13d619
https://hg.mozilla.org/mozilla-central/rev/f83b80d1aa07
https://hg.mozilla.org/mozilla-central/rev/6ce6d78452a1

Comment 76

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc4168b58a13
Split SurfaceDescriptor.h from ShadowLayerUtils.h. r=mattwoodrow

Comment 77

[Andreea Pavel [:apavel]]

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