Add IPC serialization support for EnumSet

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: botond, Assigned: zielas.daniel, Mentored)

Tracking

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

Reporter

Description

a year ago
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
Reporter

Updated

a year ago
Mentor: botond
Whiteboard: [lang=c++]
Assignee

Comment 1

a year ago
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"
Flags: needinfo?(botond)
Reporter

Comment 2

a year ago
(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
Assignee: nobody → szefoski22
Flags: needinfo?(botond)
Assignee

Comment 3

a year ago
I hope it fulfil expectations, if not please point what should be improved.
Flags: needinfo?(botond)
Attachment #8938966 - Flags: review+
Reporter

Comment 4

a year ago
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.)
Attachment #8938966 - Flags: review+

Comment 5

a year ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c9d1491515
Add IPC serialization support for EnumSet. r=botond
Reporter

Comment 6

a year ago
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
Flags: needinfo?(botond)
Reporter

Comment 7

a year ago
(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.
Assignee

Comment 8

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

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/03c9d1491515
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Reporter

Updated

a year ago
Blocks: 1427229
You need to log in before you can comment on or make changes to this bug.