914826 - clean up generated protocol headers a little bit

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - use static_assert instead of COMPILE_ASSERT in IPCMessageStart.h

COMPILE_ASSERT was chromium's static_assert.  Use the new hotness.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - rename builtinIncludes to builtinHeaderIncludes

We're going to add includes for only cpp files in later patches.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - forward-declare nsIFile for GetMinidump() declaration

No need to include nsIFile in headers just so we can declare things for parent
processes.  The condition is copied from the condition guarding the declaration
of GetMinidump further down; I wanted to factor out the condition, but the
latter condition also guards other things, so I left as-is.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - provide for cpp-only include files, starting with nsIFile.h and GeckoProfiler.h

These files aren't actually needed in the generated headers; let's just include
them in the source files.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 5 - fix source files that were bootlegging XPCOM do_* functions via generated ipdl headers

Various bits of Gecko bootleg do_CreateInstance and do_GetService from the ipdl headers.
Make them stop so we can sanely refactor the generated headers.

I see that this patch doesn't fully compile on x86-64 Linux, though it does on Android.
I'd like to request a blanket rs+ on similar changes to these that are required to fix
up compile errors I discover when pushing this to try or similar.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 6 - don't include basictypes.h or nscore.h in ipdl headers

We don't use anything from these headers in the generated headers.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 802573 [details] [diff] [review]
part 5 - fix source files that were bootlegging XPCOM do_* functions via generated ipdl headers

Please see patch comment or comment 5.

Comment 8

[(no longer active)]

Comment on attachment 802566 [details] [diff] [review]
part 1 - use static_assert instead of COMPILE_ASSERT in IPCMessageStart.h

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

This is hot stuff!  ;-)

Comment 9

[(no longer active)]

Comment on attachment 802573 [details] [diff] [review]
part 5 - fix source files that were bootlegging XPCOM do_* functions via generated ipdl headers

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

r+rs=me

Comment 10

[(no longer active)]

Comment on attachment 802572 [details] [diff] [review]
part 4 - provide for cpp-only include files, starting with nsIFile.h and GeckoProfiler.h

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

::: ipc/ipdl/ipdl/lower.py
@@ +2575,4 @@
>          cf.addthings((
>              [ Whitespace.NL ]
> +            + cppheaders
> +            + [ Whitespace.NL ]

Please #include these at the very end of the #include list, to make sure that they won't declare names used by the other headers later in the list.

Comment 11

[(no longer active)]

Comment on attachment 802574 [details] [diff] [review]
part 6 - don't include basictypes.h or nscore.h in ipdl headers

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

I've seen basictypes.h provide PrintfIncludeMacros.h stuff which is sometimes used in b2g specific code.  r=me if this builds.  :-)

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 5 - provide for cpp-only include files, starting with nsIFile.h and GeckoProfiler.h

OK, let's put them at the end, then!

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 4 - fix source files that were bootlegging XPCOM do_* functions via generated ipdl headers

This seems to compile on Try.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #11)
> I've seen basictypes.h provide PrintfIncludeMacros.h stuff which is
> sometimes used in b2g specific code.  r=me if this builds.  :-)

Should start using whatever's in mfbt...but it builds:

https://tbpl.mozilla.org/?tree=Try&rev=c6debde04d98

Comment 15

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52b319afe965
https://hg.mozilla.org/integration/mozilla-inbound/rev/58d96448609a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4ce1d4d09b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b28cb0ae1853
https://hg.mozilla.org/integration/mozilla-inbound/rev/d16460fc0518
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a029c13179

Pushed after green try: https://tbpl.mozilla.org/?tree=Try&rev=7cc165920e0f  Ideally there won't have been too many header switcharounds since this morning...

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Interestingly, this was busted on ASAN builds. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e16e1dae3315

https://tbpl.mozilla.org/php/getParsedLog.php?id=27845091&tree=Mozilla-Inbound

Comment 17

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/011a9b6b8457
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0bf7e9815ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/19f9a0020632
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7667dd91087
https://hg.mozilla.org/integration/mozilla-inbound/rev/50719d4df698
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dde02a8bc25

Green builds on try, unlike last time: https://tbpl.mozilla.org/?tree=Try&rev=b86ad5197f8c

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/011a9b6b8457
https://hg.mozilla.org/mozilla-central/rev/f0bf7e9815ff
https://hg.mozilla.org/mozilla-central/rev/19f9a0020632
https://hg.mozilla.org/mozilla-central/rev/b7667dd91087
https://hg.mozilla.org/mozilla-central/rev/50719d4df698
https://hg.mozilla.org/mozilla-central/rev/0dde02a8bc25

Comment 19

[Jason Smith [:jsmith]]

This broke YouTube playback on Firefox OS on trunk, which breaks a daily smoketest. See bug 922334 a followup.