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"

(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 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.)

(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.