1425926 - Add IPC serialization support for EnumSet

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Botond Ballo [:botond]]

MFBT contains an EnumSet<T> utility class [1] which represents a set of flags defined by an enumeration.

In bug 1420996, we are replacing an enumeration CompositorHitTestInfo which handled the combination of flags manually, with an EnumSet. Since CompositorHitTestInfo is sent over IPC [2], we need to add support for EnumSet to be sent over IPC.

[1] https://searchfox.org/mozilla-central/source/mfbt/EnumSet.h
[2] https://searchfox.org/mozilla-central/rev/b1e0ae2573e88391a7ed92407568be77994c9317/gfx/ipc/GfxMessageUtils.h#927

Comment 1

[zielas.daniel]

Hi Botond, I would like to take care of this improvement.

I have already followed "https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation" and "./mach run" starts Firefox on my Fedora.

I want to be double sure if I understand the need correct. So it looks like I just have to add a "struct "template<typename T>
struct ParamTraits<mozilla::EnumSet<T>>" and correct "write" and "read" methods in "GfxMessageUtils.h"?

I also see that "EnumSet" has "serialize" and "deserialize" methods so I want to use them in "ParamTraits" "write" and "read" methods.

I'm wondering if "GfxMessageUtils.h" is a correct place, "EnumSet.h" is from "mfbt" subdirectory similar like "Variant.h" but ParamTraits for mentioned Variant is defined inside "ipc/glue/IPCMessageUtils.h"

Comment 2

[Botond Ballo [:botond]]

(In reply to szefoski22 from comment #1)
> Hi Botond, I would like to take care of this improvement.
> 
> I have already followed
> "https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation"
> and "./mach run" starts Firefox on my Fedora.

Great, thanks for you interest! I assigned the bug to you.

> I want to be double sure if I understand the need correct. So it looks like
> I just have to add a "struct "template<typename T>
> struct ParamTraits<mozilla::EnumSet<T>>" and correct "write" and "read"
> methods in "GfxMessageUtils.h"?
> 
> I also see that "EnumSet" has "serialize" and "deserialize" methods so I
> want to use them in "ParamTraits" "write" and "read" methods.

Yes, that's the idea.

One thing that's worth noting is that the motivation for making this change is to use an EnumSet in place of CompositorHitTestInfo (bug 1420996). The current code for serializing CompositorHitTestInfo [1] uses BitFlagsEnumSerializer, which performs validation, by checking that the enum value being serialized/deserialized is "within bounds". The validation is possible because the ParamTraits specialization provides a value specific to the enumeration (in this case, CompositorHitTestInfo::ALL_BITS). For a generic implementation (ParamTraits<EnumSet<T>> for any T), we can't do such validation without some additional machinery to obtain the correct "ALL_BITS" value for T. That's fine for now, though - let's do it as you described, and we can add validation in a follow-up bug.

> I'm wondering if "GfxMessageUtils.h" is a correct place, "EnumSet.h" is from
> "mfbt" subdirectory similar like "Variant.h" but ParamTraits for mentioned
> Variant is defined inside "ipc/glue/IPCMessageUtils.h"

I think IPCMessageUtils.h is more appropriate, as EnumSet is a generic utility type, not specific to graphics.

Let me know if you have any other questions!

[1] https://searchfox.org/mozilla-central/rev/8ed13df845595dcb50c381aadde678706fd22332/gfx/ipc/GfxMessageUtils.h#954

Comment 3

[zielas.daniel]

Attachment: Bug 1425926 - Add IPC serialization support for EnumSet

I hope it fulfil expectations, if not please point what should be improved.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8938966 [details] [diff] [review]
Bug 1425926 - Add IPC serialization support for EnumSet

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

Looks great, thanks!

(Note that the intended use of the 'review' flag is to set it to '?' when requesting review (a field will come up to enter the name of the desired reviewer). The reviewer then sets the flag to '+' if the patch looks good.)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c9d1491515
Add IPC serialization support for EnumSet. r=botond

Comment 6

[Botond Ballo [:botond]]

I pushed the patch to mozilla-inbound, our integration branch, when some automated builds and tests are run on it. You can follow its progress here [1]. Assuming it's passing tests, it will automatically be merged to mozilla-central.

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=03c9d149151518d8b3b4312379dc783fca36585a

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> One thing that's worth noting is that the motivation for making this change
> is to use an EnumSet in place of CompositorHitTestInfo (bug 1420996). The
> current code for serializing CompositorHitTestInfo [1] uses
> BitFlagsEnumSerializer, which performs validation, by checking that the enum
> value being serialized/deserialized is "within bounds". The validation is
> possible because the ParamTraits specialization provides a value specific to
> the enumeration (in this case, CompositorHitTestInfo::ALL_BITS). For a
> generic implementation (ParamTraits<EnumSet<T>> for any T), we can't do such
> validation without some additional machinery to obtain the correct
> "ALL_BITS" value for T. That's fine for now, though - let's do it as you
> described, and we can add validation in a follow-up bug.

I filed bug 1427229 for performing validation. Let me know if you're interested in working on it.

Comment 8

[zielas.daniel]

Sounds Great! I will be happy to work on a next issue bug 1427229.

Comment 9

[Cosmin Sabou [:CosminS]]

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