1420996 - Replace CompositorHitTestInfo with an EnumSet

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement, P3)

Description

[Botond Ballo [:botond]]

We have an enumeration called CompositorHitTestInfo [1] that's whose enumerators are basically bit flags, which we use in combination.

We have a library facility called EnumSet [2] for expressing this pattern in an easier-to-use way.

The general idea would be to replace the enum CompositorHitTestInfo with an enum CompositorHitTestFlags, which would contain each bit-flag but with the values just being 1, 2, 3, ... instead of 2^1, 2^2, 2^3, ..., and then use an EnumSet<CompositorHitTestFlags> wherever we use CompositorHitTestInfo today.

I'm going to make this a mentored bug because it's straightforward enough for someone new to the codebase to play around with, and not high-priority.

[1] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/gfx/src/CompositorHitTestInfo.h#20
[2] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/mfbt/EnumSet.h

Comment 1

[Kartikaya Gupta (email:[e-mail])]

One problem with this is that it seems to be implemented using a 32-bit value underneath, whereas CompositorHitTestInfo is a uint16_t. This is important because we pass this value to rust and it needs to fit into the u16 on the webrender side.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

For the record the conversion happens via a rust function with a C binding, at [1]. It's not hard to take the 32 bit value and trim it to the bottom 16 bits, we just need to make sure that the underlying flgas enum doesn't extend into the top 16 bits of the 32-bit value. (Or we need to increase the size of the rust-side field).

[1] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/gfx/webrender_bindings/WebRenderAPI.cpp#1274

Comment 3

[spencerb]

I've been trying to work on this but I think I'm a little confused.  EnumSet makes perfect sense to me, but I'm not sure how the new enum is supposed to work.
If we make the values of the new enum just 0, 1, 2, 3, ..., (which is how I interpreted the description) wouldn't it make the values ambiguous?
Like, if
eInvisibleToHitTest = 0,
eVisibleToHitTest = 1,
eDispatchToContent = 2,
eTouchActionPanXDisabled = 3,
etc.
Then eVisibleToHitTest | eDispatchToContent would result in the same as eTouchActionPanXDisabled.
Have I got the wrong idea of what I should be doing.
Thanks.

Comment 4

[Botond Ballo [:botond]]

You would no longer write "eVisibleToHitTest | eDispatchToContent" to express a combination of values. You would write something like:

EnumSet<CompositorHitTestFlags> flags;
flags += eVisibleToHitTest;
flags += eDispatchToContent;
// use "flags"

Internally, the EnumSet stores an integer, and each time you add an enumerator with value N to the EnumSet, it bitwise-ors (1 << N) into that integer, so there will be no conflict.

Does that make sense?

Comment 5

[spencerb]

Ah yes, now I see.  The bitFor method.  Thanks.

Comment 6

[spencerb]

What about the special values like eTouchActionMask.  There isn't a value it can be assigned that would bitshift to 120, which would be the value of the EnumSet if the 4 eTouch values are set.  If the current functionality of eTouchActionMask can't be duplicated, should I just omit it and change usages to check all 4 eTouch values?

Comment 7

[Botond Ballo [:botond]]

Since eTouchActionMask is a combination of values, it would be represented as an EnumSet<CompositorHitTestFlags>. We could have a global constant of type EnumSet<CompositorHitTestFlags> for it.

Uses of eTouchActionMask should map to corresponding operations on EnumSet. For example, here [1] we are taking the bitwise-and of a CompositorHitTestInfo variable and eTouchActionMask. After this change, we would be taking the bitwise-and of two EnumSet<CompositorHitTestFlags> variables (EnumSet has an operator for that [2]).

[1] https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/layout/painting/nsDisplayList.cpp#5142
[2] https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/mfbt/EnumSet.h#170

Comment 8

[spencerb]

Attachment: CompositorHitTestFlags.h file containing new enum and constants

Hopefully I'm on the right track.

Comment 9

[Botond Ballo [:botond]]

Thanks for the patch! I assigned the bug to you to reflect that you're working on it.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8937465 [details] [diff] [review]
CompositorHitTestFlags.h file containing new enum and constants

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

Thanks, this is a good start!

Note that, in addition to these changes, the patch will also need to update places in the code that use CompositorHitTestInfo accordingly. You can use Searchfox [1] to find where those places are.

[1] https://searchfox.org/mozilla-central/search?q=CompositorHitTestInfo&path=

::: gfx/src/CompositorHitTestFlags.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MOZILLA_GFX_COMPOSITORHITTESTINFO_H_

Instead of creating a new file, the changes can go into gfx/src/CompositorHitTestInfo.h.

Even though we are changing the name of the enum, we can typedef EnumSet<CompositorHitTestFlags> to CompositorHitTestInfo, so from the point of view of calling code, the name of the type representing a combination of flags doesn't change.

@@ +25,5 @@
> +                                      EnumSet<CompositorHitTestFlags>(eTouchActionDoubleTapZoomDisabled)
> +
> +// Used for IPDL serialization. This bitmask should include all the bits
> +// that are defined in the enum.
> +const EnumSet<CompositorHitTestFlags> ALL_BITS = EnumSet<CompositorHitTestFlags>().deserialize((1 << 10) - 1);

We'll need to do a bit of extra work to support serialization of EnumSet.

Instead of doing it specifically for CompositorHitTestFlags, we should just support serialization of EnumSet<T> in general. I'll file a separate bug for that and mark this as depending on that one.

@@ +29,5 @@
> +const EnumSet<CompositorHitTestFlags> ALL_BITS = EnumSet<CompositorHitTestFlags>().deserialize((1 << 10) - 1);
> +
> +enum class CompositorHitTestFlags : uint16_t {
> +  // Shortcut for checking that none of the flags are set
> +  eInvisibleToHitTest = 0,

We don't need this flag. "None of the flags are set" will be represented as an EnumSet with none of the subsequent flags set in it.

@@ +55,5 @@
> +  // one (if set) or a horizontal one (if not set)
> +  eScrollbarVertical = 9,
> +};
> +
> +MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(CompositorHitTestFlags)

We don't need this any more (and in fact don't want it, because users should no longer be combining the flags with bitwise operators manually).

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> We'll need to do a bit of extra work to support serialization of EnumSet.
> 
> Instead of doing it specifically for CompositorHitTestFlags, we should just
> support serialization of EnumSet<T> in general. I'll file a separate bug for
> that and mark this as depending on that one.

I filed bug 1425926. Let me know if you're interested in working on that as well.

Comment 12

[Botond Ballo [:botond]]

Note, bug 1425926 has been fixed, so work here can proceed.

Comment 13

[spencerb]

Attachment: CompositorHitTestInfo.h

Thanks.
Here is CompositorHitTestInfo.h.
I am currently working on changing all the usages of CompositorHitTestInfo.

Comment 14

[spencerb]

Hmm.  I guess I am still getting used to the workflow.  That was definitely not the right diff.

Comment 15

[spencerb]

Attachment: Correct CompositorHitTestInfo.h with new enum and constants

I think this is correct.

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8945245 [details] [diff] [review]
Correct CompositorHitTestInfo.h with new enum and constants

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

Looks like you're on the right track. There are a couple of compiler errors related to order of declarations that you'll run into once you finish the patch and try to compile it.

::: gfx/src/CompositorHitTestInfo.h
@@ +19,5 @@
>  // sent to the compositor.
> +
> +// Used for IPDL serialization. This bitmask includes all the bits defined in
> +// the enum.
> +const EnumSet<CompositorHitTestFlags> ALL_BITS = EnumSet<CompositorHitTestFlags>().deserialize((1 << 9) - 1);

Given the way we did serialization of EnumSet in bug 1425926, we don't need this ALL_BITS variable any more.

However, once bug 1427229 lands, we'll need a specialization of the MaxEnumValue trait being added in that bug for CompositorHitTestFlags. It probably makes sense to add that in a separate patch, since we don't know which order the bugs will be ready to land in.

@@ +24,3 @@
>  
> +// Mask to check for all the touch-action flags at once
> +const EnumSet<CompositorHitTestFlags> eTouchActionMask = EnumSet<CompositorHitTestFlags>(eTouchActionPanXDisabled) + EnumSet<CompositorHitTestFlags>(eTouchActionPanYDisabled) + EnumSet<CompositorHitTestFlags>(eTouchActionPinchZoomDisabled) + EnumSet<CompositorHitTestFlags>(eTouchActionDoubleTapZoomDisabled);

Please format like so:

const EnumSet<CompositorHitTestFlags> eTouchActionMask = 
  EnumSet<CompositorHitTestFlags>(eTouchActionPanXDisabled) + 
  EnumSet<CompositorHitTestFlags>(eTouchActionPanYDisabled) + 
  EnumSet<CompositorHitTestFlags>(eTouchActionPinchZoomDisabled) + 
  EnumSet<CompositorHitTestFlags>(eTouchActionDoubleTapZoomDisabled);

Also, you can use "CompositorHitTestInfo" instead of "EnumSet<CompositorHitTestFlags>" to make it a bit shorter.

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #16)
> once bug 1427229 lands, we'll need a specialization of the
> MaxEnumValue trait being added in that bug for CompositorHitTestFlags. It
> probably makes sense to add that in a separate patch, since we don't know
> which order the bugs will be ready to land in.

Note that bug 1427229 has landed, so please disregard the patch about a separate patch: just add the MaxEnumValue specialization in the same patch as the rest of the changes.

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #17)
> Note that bug 1427229 has landed, so please disregard the patch about a
> separate patch

I meant "please disregard the part about a separate patch"

Comment 19

[Botond Ballo [:botond]]

Hey spencerb, how is the rest of the patch here coming? Anything I can help with?

Comment 20

[spencerb]

Attachment: In-Progress Patch

I think it is progressing nicely, thanks.
I'm posting the diff of my current changes.
The only one of the usages of CompositorHitTestInfo that I haven't changed is https://searchfox.org/mozilla-central/source/gfx/ipc/GfxMessageUtils.h#954.
Am I correct in assuming that this, like the ALL_BITS constant, is deprecated in favor of serializing the EnumSet itself?

Comment 21

[spencerb]

Scratch that, there are a few more usages besides that I still haven't adjusted yet, although that doesn't affect my question.

Comment 22

[spencerb]

I also noticed that I need to change the usage in WebRenderAPI.cpp as was mentioned at the beginning of this bug.  I believe I will wait until after I make the MaxEnumValue and possibly use that there as well.

Comment 23

[Botond Ballo [:botond]]

(In reply to spencerb from comment #20)
> The only one of the usages of CompositorHitTestInfo that I haven't changed
> is
> https://searchfox.org/mozilla-central/source/gfx/ipc/GfxMessageUtils.h#954.
> Am I correct in assuming that this, like the ALL_BITS constant, is
> deprecated in favor of serializing the EnumSet itself?

Yep, this ParamTraits specialization can be removed. You just need to specialize MaxEnumValue trait added in bug 1427229 for CompositorHitTestFlags, and serialization of EnumSet<CompositorHitTestFlags> will just work.

Comment 24

[Botond Ballo [:botond]]

(In reply to spencerb from comment #22)
> I also noticed that I need to change the usage in WebRenderAPI.cpp as was
> mentioned at the beginning of this bug.  I believe I will wait until after I
> make the MaxEnumValue and possibly use that there as well.

Yeah, a check like "MaxEnumValue<CompositorHitTestFlags>::value < 16" in the static_assert should be enough.

Comment 25

[Botond Ballo [:botond]]

(In reply to spencerb from comment #20)
> I'm posting the diff of my current changes.

Have you tried building with these changes? They contain a number of compiler errors.

I'll leave it up to you to discover and fix them, but I'll mention the use of "CompositorHitTestFlags::eInvisibleToHitTest" in particular.

Recall from comment 10 that we removed eInvisibleToHitTest:

(In reply to Botond Ballo [:botond] from comment #10)
> @@ +29,5 @@
> > +const EnumSet<CompositorHitTestFlags> ALL_BITS = EnumSet<CompositorHitTestFlags>().deserialize((1 << 10) - 1);
> > +
> > +enum class CompositorHitTestFlags : uint16_t {
> > +  // Shortcut for checking that none of the flags are set
> > +  eInvisibleToHitTest = 0,
> 
> We don't need this flag. "None of the flags are set" will be represented as
> an EnumSet with none of the subsequent flags set in it.

We can use a default-constructed CompositorHitTetInfo variable to represent this value. (We can create a named constant for it, similar to eTouchActionMask).

Comment 26

[Botond Ballo [:botond]]

Hey spencerb, just wanted to check in to see how things are going with this bug. If there's anything you're stuck on or that I can help clarify, let me know!

Comment 27

[Botond Ballo [:botond]]

Hi spencerb, are you still working on this?

Comment 28

[spencerb]

Yes, sorry for the slow progress.  I am pretty much finished, just going over the changes and cleaning up some stuff.  I am away from home atm, but will upload the finished patch this weekend.
Thanks for your patience, and your help getting used to the FF codebase.

Comment 29

[Botond Ballo [:botond]]

Cool, thanks for the update!

Comment 30

[Botond Ballo [:botond]]

Hi spencerb, just wanted to check in again. How are things going with this patch?

Comment 31

[Botond Ballo [:botond]]

As we have not heard back from spencerb, I'm going to clear the bug's assignee to make it available for others to work on.

spencerb, if you'd like to resume working on this, please let me know. Even if you don't, if you want to just upload the latest version of your local patch for someone else to pick up, that would be appreciated :)

Comment 32

[zielas.daniel]

Hello, I would like to take this one.

Comment 33

[Botond Ballo [:botond]]

(In reply to zielas.daniel from comment #32)
> Hello, I would like to take this one.

Great! I assigned it to you.

I think the bug description, the attached work-in-progress patch, and the comments so far should give you a pretty good idea of what to do. If you have any questions, feel free to ask!

Comment 34

[zielas.daniel]

Attachment: bug_1420996.patch

Hello,

Please review my patch. Below is a Try Server check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f043436c3d6eb13a47653b684e33eeab9645978d

Comment 35

[zielas.daniel]

Now I see ...

"bool operator==(const EnumSet<T> aEnumSet) const" and added by me "bool contains_only(const EnumSet<T> &aEnumSet) const" is the same. I will remove it after review.

Comment 36

[Botond Ballo [:botond]]

Your Try push is showing some test failures related to the patch.

Do you know how to run/debug a test to investigate the issue?

The easiest one to start with is probably the GTest (and you might find that the other failures have the same root cause).

Comment 37

[Botond Ballo [:botond]]

Here are some docs on running/debugging GTests:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/GTest

Comment 38

[zielas.daniel]

Hello Botond,

Thanks for GTests links.

So I have found a root cause. It was a little bit tricky. The conversion from Enum into EnumSet is not always straightforward.
In HitTestingTreeNode::HitTest(const LayerPoint& aPoint) const [1]

There is:
> CompositorHitTestInfo result = CompositorHitTestInfo::eInvisibleToHitTest; // result has value 0
> ...
> ...
> result |= CompositorHitTestInfo::eVisibleToHitTest; // result has value 1

In legacy Enum solution converting it to Integer gives a number 1. Because CompositorHitTestInfo::eInvisibleToHitTest has value 0 and CompositorHitTestInfo::eVisibleToHitTest has value 1.

But in a EnumSet solution:
> CompositorHitTestInfo result = CompositorHitTestFlags::eInvisibleToHitTest; // serialized value is 1
> ...
> ...
> result += CompositorHitTestFlags::eVisibleToHitTest; // serialized value is (1 LOGICAL OR 2) what gives 3

Serializing it gives value 3. What is logically more accurate, because we turin on bit 0 and turn on bit 1. In the original legacy solution any logical OR performs a reset value of CompositorHitTestInfo::eInvisibleToHitTest (what is some kind of shortcut). In EnumSet solution we have to reset it explicitly:
> CompositorHitTestInfo result = CompositorHitTestFlags::eInvisibleToHitTest; // serialized value is 1
> ...
> ...
> result = CompositorHitTestFlags::eVisibleToHitTest; // serialized value is 2

So it looks like when in legacy Enum solution is in use an Enum with value 0 we have to be very careful converting it to EnumSet.

Serialized EnumSet values are shifted by 1 according to legacy Enum values, are we sure it will not brake anything when we pass it to Rust? :
(
/*enum*/                        eInvisibleToHitTest = 0,         // it's 0
EnumSet<CompositorHitTestFlags>(eInvisibleToHitTest).serialize() // it's 1

/*enum*/                        eVisibleToHitTest = 1 << 0,      // it's 1
EnumSet<CompositorHitTestFlags>(eVisibleToHitTest).serialize()   // it's 2

/*enum*/                        eDispatchToContent = 1 << 1,     // it's 2
EnumSet<CompositorHitTestFlags>(eDispatchToContent).serialize()  // it's 4

)


I will upload a new patch soon.


[1] https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/HitTestingTreeNode.cpp#292

Comment 39

[zielas.daniel]

More complete picture of values passed to Rust:
(
/*old enum*/                        eInvisibleToHitTest = 0,          // it's 0
/*enum prepared for EnumSet*/       eInvisibleToHitTest = 0,          // it's 0
EnumSet<CompositorHitTestFlags>(eInvisibleToHitTest).serialize()      // it's 1

/*old enum*/                    eVisibleToHitTest = 1 << 0,           // it's 1
/*enum prepared for EnumSet*/   eVisibleToHitTest = 1,                // it's 1
EnumSet<CompositorHitTestFlags>(eVisibleToHitTest).serialize()        // it's 2

/*old enum*/                    eDispatchToContent = 1 << 1,          // it's 2
/*enum prepared for EnumSet*/   eDispatchToContent = 2,               // it's 2
EnumSet<CompositorHitTestFlags>(eDispatchToContent).serialize()       // it's 4

/*old enum*/                    eTouchActionPanXDisabled = 1 << 2,    // it's 2
/*enum prepared for EnumSet*/   eTouchActionPanXDisabled = 3,         // it's 3
EnumSet<CompositorHitTestFlags>(eTouchActionPanXDisabled).serialize() // it's 8
)

Source code implemented in Rust will handle our switch to EnumSet without any improvements?

Comment 40

[Botond Ballo [:botond]]

This actually came up earlier, and I addressed it in comment 10:

(In reply to Botond Ballo [:botond] from comment #10)
> @@ +29,5 @@
> > +const EnumSet<CompositorHitTestFlags> ALL_BITS = EnumSet<CompositorHitTestFlags>().deserialize((1 << 10) - 1);
> > +
> > +enum class CompositorHitTestFlags : uint16_t {
> > +  // Shortcut for checking that none of the flags are set
> > +  eInvisibleToHitTest = 0,
> 
> We don't need this flag. "None of the flags are set" will be represented as
> an EnumSet with none of the subsequent flags set in it.

Following this advice should resolve the issue you describe.

Comment 41

[Botond Ballo [:botond]]

(In reply to zielas.daniel from comment #39)
> Source code implemented in Rust will handle our switch to EnumSet without
> any improvements?

The interface with Rust code happens in DisplayListBuilder::SetHitTestInfo() and WebRenderAPI::HitTest(). Your patch already changes those locations to call serialize() on the EnumSet. As long as that results in the same numerical value as the CompositorHitTestInfo had before this patch (which I expect to be the case), no changes on the Rust side are required.

Comment 42

[zielas.daniel]

(In reply to Botond Ballo [:botond] from comment #40)
> This actually came up earlier, and I addressed it in comment 10:
> 
> (In reply to Botond Ballo [:botond] from comment #10)
> > @@ +29,5 @@
> > > +const EnumSet<CompositorHitTestFlags> ALL_BITS = EnumSet<CompositorHitTestFlags>().deserialize((1 << 10) - 1);
> > > +
> > > +enum class CompositorHitTestFlags : uint16_t {
> > > +  // Shortcut for checking that none of the flags are set
> > > +  eInvisibleToHitTest = 0,
> > 
> > We don't need this flag. "None of the flags are set" will be represented as
> > an EnumSet with none of the subsequent flags set in it.
> 
> Following this advice should resolve the issue you describe.

Thanks I have missed it.

Comment 43

[zielas.daniel]

(In reply to Botond Ballo [:botond] from comment #41)
> (In reply to zielas.daniel from comment #39)
> > Source code implemented in Rust will handle our switch to EnumSet without
> > any improvements?
> 
> The interface with Rust code happens in DisplayListBuilder::SetHitTestInfo()
> and WebRenderAPI::HitTest(). Your patch already changes those locations to
> call serialize() on the EnumSet. As long as that results in the same
> numerical value as the CompositorHitTestInfo had before this patch (which I
> expect to be the case), no changes on the Rust side are required.

OK. I have just found something suspicious [1]. Maybe just the comment is wrong now when I have removed 'CompositorHitTestInfo::eInvisibleToHitTest'.

[1] https://searchfox.org/mozilla-central/source/gfx/webrender_bindings/src/bindings.rs#2362

Comment 44

[zielas.daniel]

Attachment: bug_1420996_2.patch

Please review the new patch.

Here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a952eca9b4008e9472018d1dbd0eba81b3a4a8a

Comment 45

[Botond Ballo [:botond]]

Comment on attachment 8988556 [details] [diff] [review]
bug_1420996_2.patch

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1515,5 @@
>    return result;
>  }
>  
>  static TouchBehaviorFlags
> +ConvertToTouchBehavior(const CompositorHitTestInfo &info)

nit: as per style guide, & goes beside the type

::: gfx/layers/apz/src/APZUtils.h
@@ +87,5 @@
>      : mTargetConfirmed(aTargetConfirmed)
>      , mRequiresTargetConfirmation(false)
>    {}
>  
> +  explicit TargetConfirmationFlags(const gfx::CompositorHitTestInfo &aHitTestInfo)

& beside type

::: gfx/layers/apz/testutil/APZTestData.cpp
@@ +68,5 @@
>    static void ConvertHitResult(const APZTestData::HitResult& aResult,
>                                 dom::APZHitResult& aOutHitResult) {
>      aOutHitResult.mScreenX.Construct() = aResult.point.x;
>      aOutHitResult.mScreenY.Construct() = aResult.point.y;
> +    static_assert(MaxEnumValue<std::remove_reference<decltype(aResult.result)>::type::valueType>::value

I don't really see the point in the remove_reference and the decltype here. By using ::valueType, we're effectively assuming the type of |aResult.result| is an EnumSet, so might as well just do:

  MaxEnumValue<CompositorHitTestInfo::valueType>::value

@@ +70,5 @@
>      aOutHitResult.mScreenX.Construct() = aResult.point.x;
>      aOutHitResult.mScreenY.Construct() = aResult.point.y;
> +    static_assert(MaxEnumValue<std::remove_reference<decltype(aResult.result)>::type::valueType>::value
> +                  < std::numeric_limits<uint16_t>::digits,
> +                  "CompositorHitTestFlags MAX value have to be less than number of digits in uint16_t");

I would say "number of bits", since "number of digits" might be misinterpreted as "number of decimal digits".

::: gfx/src/CompositorHitTestInfo.h
@@ +17,5 @@
>  // that is relevant to hit-testing in the compositor. The flags are
>  // intentionally set up so that if all of them are 0 the item is effectively
>  // invisible to hit-testing, and no information for this frame needs to be
>  // sent to the compositor.
> +enum class CompositorHitTestFlags : uint32_t {

I would actually make this a uint8_t, since the allowed values now range from 0 to 9 (with possible future extension up to 15, but no more, since the values, raised to a power of 2, must fit into a 16-bit integer).

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +357,5 @@
>                        wr::WrPipelineId& aOutPipelineId,
>                        layers::FrameMetrics::ViewID& aOutScrollId,
>                        gfx::CompositorHitTestInfo& aOutHitInfo)
>  {
> +  static_assert(MaxEnumValue<std::remove_reference<decltype(aOutHitInfo)>::type::valueType>::value

Instead of repeating this static_assert in multiple places, perhaps we could just have it once, in CompositorHitTestInfo.h?

@@ +362,5 @@
> +                < std::numeric_limits<uint16_t>::digits,
> +                "CompositorHitTestFlags MAX value have to be less than number of digits in uint16_t");
> +
> +  uint16_t serialized = static_cast<uint16_t>(aOutHitInfo.serialize());
> +  const auto result = wr_api_hit_test(mDocHandle, aPoint,

s/auto/bool

::: gfx/webrender_bindings/src/bindings.rs
@@ +2358,5 @@
>                                    out_hit_info: &mut u16) -> bool {
>      let result = dh.api.hit_test(dh.document_id, None, point, HitTestFlags::empty());
>      for item in &result.items {
>          // For now we should never be getting results back for which the tag is
> +        // 0. In the future if we allow this, we'll want to |continue| on the loop

We can keep the parenthetical remark and say "(CompositorHitTestInfo with no flags set)".

::: layout/generic/nsFrame.cpp
@@ +11273,5 @@
>      // encounter an element that disables it that's inside the scrollframe.
>      // This is equivalent to the |considerPanning| variable in
>      // TouchActionHelper.cpp, but for a top-down traversal.
> +    CompositorHitTestInfo panMask(CompositorHitTestFlags::eTouchActionPanXDisabled);
> +    panMask += CompositorHitTestFlags::eTouchActionPanYDisabled;

You can use the two-argument EnumSet constructor here, like you did in WebRenderCommandBuilder.

::: mfbt/EnumSet.h
@@ +27,5 @@
>  class EnumSet
>  {
>  public:
>    typedef uint32_t serializedType;
> +  typedef T valueType;

Please split out the changes to EnumSet.h into a separate patch, as they will need to be reviewed separately by an MFBT peer.

Comment 46

[Botond Ballo [:botond]]

Comment on attachment 8988556 [details] [diff] [review]
bug_1420996_2.patch

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

Kats, I would like your opinion on one thing. This patch changes checks that currently look like this:

  MOZ_ASSERT(hitResult != CompositorHitTestInfo::eInvisibleToHitTest);

to look like this:

  MOZ_ASSERT(!hitResult.isEmpty());

Does that seem all right to you, or would you prefer introducing a named constant for an empty CompositorHitTestInfo (that keeps the name "invisible to hit test") to make such checks more explicit?

Comment 47

[Botond Ballo [:botond]]

Comment on attachment 8988556 [details] [diff] [review]
bug_1420996_2.patch

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

::: gfx/layers/apz/test/gtest/TestEventRegions.cpp
@@ +242,5 @@
>  
>    gfx::CompositorHitTestInfo result;
>    RefPtr<AsyncPanZoomController> hit = manager->GetTargetAPZC(ScreenPoint(50, 75), &result);
>    EXPECT_EQ(child, hit.get());
> +  EXPECT_TRUE(result == CompositorHitTestFlags::eVisibleToHitTest);

These should still work in EXPECT_EQ form, and I would prefer to keep them in that form, because then the error messages that are printed are more descriptive (they include the two values).

Comment 48

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #45)
> >  {
> > +  static_assert(MaxEnumValue<std::remove_reference<decltype(aOutHitInfo)>::type::valueType>::value
> 
> Instead of repeating this static_assert in multiple places, perhaps we could
> just have it once, in CompositorHitTestInfo.h?

I actually slightly prefer having the asserts at the point in the code that they're relevant (i.e. at the FFI boundary where the assumption comes into play) even if that means it gets repeated.

(In reply to Botond Ballo [:botond] from comment #46)
> Kats, I would like your opinion on one thing. This patch changes checks that
> currently look like this:
> 
>   MOZ_ASSERT(hitResult != CompositorHitTestInfo::eInvisibleToHitTest);
> 
> to look like this:
> 
>   MOZ_ASSERT(!hitResult.isEmpty());
> 
> Does that seem all right to you, or would you prefer introducing a named
> constant for an empty CompositorHitTestInfo (that keeps the name "invisible
> to hit test") to make such checks more explicit?

I would prefer introducing the named constant, yes.

Comment 49

[zielas.daniel]

Thank you for the review!


(In reply to Kartikaya Gupta (email:[e-mail]) from comment #48)
> 
> (In reply to Botond Ballo [:botond] from comment #46)
> > Kats, I would like your opinion on one thing. This patch changes checks that
> > currently look like this:
> > 
> >   MOZ_ASSERT(hitResult != CompositorHitTestInfo::eInvisibleToHitTest);
> > 
> > to look like this:
> > 
> >   MOZ_ASSERT(!hitResult.isEmpty());
> > 
> > Does that seem all right to you, or would you prefer introducing a named
> > constant for an empty CompositorHitTestInfo (that keeps the name "invisible
> > to hit test") to make such checks more explicit?
> 
> I would prefer introducing the named constant, yes.

Does it mean I should restore 'CompositorHitTestInfo::eInvisibleToHitTest'?

Comment 50

[zielas.daniel]

(In reply to zielas.daniel from comment #49)
> Thank you for the review!
> 
> 
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #48)
> > 
> > (In reply to Botond Ballo [:botond] from comment #46)
> > > Kats, I would like your opinion on one thing. This patch changes checks that
> > > currently look like this:
> > > 
> > >   MOZ_ASSERT(hitResult != CompositorHitTestInfo::eInvisibleToHitTest);
> > > 
> > > to look like this:
> > > 
> > >   MOZ_ASSERT(!hitResult.isEmpty());
> > > 
> > > Does that seem all right to you, or would you prefer introducing a named
> > > constant for an empty CompositorHitTestInfo (that keeps the name "invisible
> > > to hit test") to make such checks more explicit?
> > 
> > I would prefer introducing the named constant, yes.
> 
> Does it mean I should restore 'CompositorHitTestInfo::eInvisibleToHitTest'?

Or just create a 'const CompositorHitTestInfo CompositorHitTestInvisibleToHit;' which has no turned on bits?

Comment 51

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #48)
> (In reply to Botond Ballo [:botond] from comment #45)
> > >  {
> > > +  static_assert(MaxEnumValue<std::remove_reference<decltype(aOutHitInfo)>::type::valueType>::value
> > 
> > Instead of repeating this static_assert in multiple places, perhaps we could
> > just have it once, in CompositorHitTestInfo.h?
> 
> I actually slightly prefer having the asserts at the point in the code that
> they're relevant (i.e. at the FFI boundary where the assumption comes into
> play) even if that means it gets repeated.

Would you be OK with reudcing repetition by having a helper function that performs the assert that's called at each FFI boundary point? Something like this:

  AssertCompositorHitTestInfoFitsIntoBits<16>();

(In reply to zielas.daniel from comment #50)
> (In reply to zielas.daniel from comment #49)
> > Thank you for the review!
> > 
> > 
> > (In reply to Kartikaya Gupta (email:[e-mail]) from comment #48)
> > > 
> > > (In reply to Botond Ballo [:botond] from comment #46)
> > > > Kats, I would like your opinion on one thing. This patch changes checks that
> > > > currently look like this:
> > > > 
> > > >   MOZ_ASSERT(hitResult != CompositorHitTestInfo::eInvisibleToHitTest);
> > > > 
> > > > to look like this:
> > > > 
> > > >   MOZ_ASSERT(!hitResult.isEmpty());
> > > > 
> > > > Does that seem all right to you, or would you prefer introducing a named
> > > > constant for an empty CompositorHitTestInfo (that keeps the name "invisible
> > > > to hit test") to make such checks more explicit?
> > > 
> > > I would prefer introducing the named constant, yes.
> > 
> > Does it mean I should restore 'CompositorHitTestInfo::eInvisibleToHitTest'?
> 
> Or just create a 'const CompositorHitTestInfo
> CompositorHitTestInvisibleToHit;' which has no turned on bits?

The latter, please.

Comment 52

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #51)
> Would you be OK with reudcing repetition by having a helper function that
> performs the assert that's called at each FFI boundary point? Something like
> this:
> 
>   AssertCompositorHitTestInfoFitsIntoBits<16>();

Yup, that's fine.

Comment 53

[zielas.daniel]

(In reply to Botond Ballo [:botond] from comment #45)
> Comment on attachment 8988556 [details] [diff] [review]
> bug_1420996_2.patch
> ::: gfx/layers/apz/testutil/APZTestData.cpp
> @@ +68,5 @@
> >    static void ConvertHitResult(const APZTestData::HitResult& aResult,
> >                                 dom::APZHitResult& aOutHitResult) {
> >      aOutHitResult.mScreenX.Construct() = aResult.point.x;
> >      aOutHitResult.mScreenY.Construct() = aResult.point.y;
> > +    static_assert(MaxEnumValue<std::remove_reference<decltype(aResult.result)>::type::valueType>::value
> 
> I don't really see the point in the remove_reference and the decltype here.
> By using ::valueType, we're effectively assuming the type of
> |aResult.result| is an EnumSet, so might as well just do:
> 
>   MaxEnumValue<CompositorHitTestInfo::valueType>::value

The motivation was to have tight coupling between argument and assert. So when someone will remove or change type of 'aResult' then the source code will not compile so will have to remove not needed assert also.

Comment 54

[zielas.daniel]

Attachment: bug_1420996_3-EnumSet.patch

Comment 55

[zielas.daniel]

Attachment: bug_1420996_3-Replace_CompositorHitTestInfo.patch

Please review the patches.
Try Server report:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e9bf5235015a9d1ef7411b90aef90da7f2baaff&selectedJob=189869089

Comment 56

[Botond Ballo [:botond]]

Comment on attachment 8994604 [details] [diff] [review]
bug_1420996_3-Replace_CompositorHitTestInfo.patch

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

Thanks, looks pretty good! A few minor comments:

::: gfx/layers/apz/testutil/APZTestData.cpp
@@ +70,5 @@
>      aOutHitResult.mScreenX.Construct() = aResult.point.x;
>      aOutHitResult.mScreenY.Construct() = aResult.point.y;
> +    static_assert(MaxEnumValue<CompositorHitTestInfo::valueType>::value
> +                  < std::numeric_limits<uint16_t>::digits,
> +                  "CompositorHitTestFlags MAX value have to be less than number of bits in uint16_t");

have -> has

::: gfx/src/CompositorHitTestInfo.h
@@ +50,5 @@
>  };
>  
> +using CompositorHitTestInfo = EnumSet<CompositorHitTestFlags>;
> +
> +// Helper for checking that none of the flags are set

I would just say "A CompositorHitTestInfo with none of the flags set".

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +1277,5 @@
>                                     gfx::CompositorHitTestInfo aHitInfo)
>  {
> +  static_assert(MaxEnumValue<CompositorHitTestInfo::valueType>::value
> +                < std::numeric_limits<uint16_t>::digits,
> +                "CompositorHitTestInfo MAX value have to be less than number of bits in uint16_t");

CompositorHitTestInfo -> CompositorHitTestFlags

By the way, have you seen my suggestion in comment 51 for avoiding the repetition between these static_asserts?

===
Would you be OK with reudcing repetition by having a helper function that performs the assert that's called at each FFI boundary point? Something like this:

  AssertCompositorHitTestInfoFitsIntoBits<16>();
===

Comment 57

[Botond Ballo [:botond]]

Comment on attachment 8994603 [details] [diff] [review]
bug_1420996_3-EnumSet.patch

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

Looks good to me, but let's ask Waldo to review this as an MFBT peer.

::: mfbt/EnumSet.h
@@ +224,5 @@
>      return mBitField & bitFor(aEnum);
>    }
>  
>    /**
> +   * Test if an set is contained in the set.

an -> a

Comment 58

[Jeff Walden [:Waldo]]

Comment on attachment 8994603 [details] [diff] [review]
bug_1420996_3-EnumSet.patch

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

Looks good -- just fix the one issue here, please?

If we'd ever interacted before I'd be happy to r+ and assume you can make the obvious fix, but for the first couple times, I want to have looked at the final patch.  Hope you understand.  :-)

Or if Botond wants to just make this fix before landing, he can consider it an r+.  Either way's fine with me.

::: mfbt/EnumSet.h
@@ +67,5 @@
>        (*this) += value;
>      }
>    }
>  
> +  EnumSet<T>& operator=(T aEnum)

Make this return void and remove the return-statement.  Actually depending on assign-chaining is not desirable -- basically it's just confusing :-) -- so we might as well just forbid it.

Comment 59

[zielas.daniel]

Thanks all for the comments.

(In reply to Botond Ballo [:botond] [on PTO, back Aug 27] from comment #56)
> Comment on attachment 8994604 [details] [diff] [review]
> bug_1420996_3-Replace_CompositorHitTestInfo.patch
> 
> Review of attachment 8994604 [details] [diff] [review]:
> -----------------------------------------------------------------
> ===
> Would you be OK with reudcing repetition by having a helper function that
> performs the assert that's called at each FFI boundary point? Something like
> this:
> 
>   AssertCompositorHitTestInfoFitsIntoBits<16>();
> ===

Sure, so it should looks like below?

// A Helper fucntion for static asserts
template <int N>
static constexpr bool AssertCompositorHitTestInfoFitsIntoBits()
{
    if (MaxEnumValue<CompositorHitTestInfo::valueType>::value < N)
    {
        return true;
    }

    return false;
}

  static_assert(AssertCompositorHitTestInfoFitsIntoBits<std::numeric_limits<uint16_t>::digits>(),
                "CompositorHitTestFlags MAX value has to be less than number of bits in uint16_t");

Comment 60

[Botond Ballo [:botond]]

Yup, this works.

I would drop the "Assert" from the function name in this formulation. (When I suggested that name, I had in mind a variation where the static_assert goes inside the function, hence the "Assert" in the function name.)

(I would also take the liberty of assuming that the number of bits in uint16_t is 16 :))

Comment 61

[zielas.daniel]

Attachment: bug_1420996_4-EnumSet.patch

Comment 62

[zielas.daniel]

Attachment: bug_1420996_4-Replace_CompositorHitTestInfo.patch

Hello

Please review the patches.
Try Server report:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eddc152ac809aa1bf4981913748fab8936e7f60

Comment 63

[Botond Ballo [:botond]]

Apologies for the long delay here. I've been on vacation and am just catching up on reviews and such now.

Comment 64

[Botond Ballo [:botond]]

Comment on attachment 8999438 [details] [diff] [review]
bug_1420996_4-Replace_CompositorHitTestInfo.patch

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

Thanks!

::: gfx/src/CompositorHitTestInfo.h
@@ +74,5 @@
> +namespace gfx {
> +
> +// Checks if the CompositorHitTestFlags max enum value is less than N.
> +template <int N>
> +static constexpr bool DoesCompositorHitTestInfoFitsIntoBits()

grammar nit: Fits -> Fit

I made this change locally (so you don't have to upload a new patch), and rebased the patches on top of the latest mozilla-central. Here is an updated Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=611b0b29e76510b7fce4bd5dcee7bc0dbde9a84b

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01ed229650de
EnumSet enhancement. r=botond,Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/413bd039c126
Replace CompositorHitTestInfo with an EnumSet. r=botond

Comment 66

[Narcis Beleuzu [:NarcisB]]

Backed out for marionette-headless perma failures

Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=413bd039c1263f3b9cef04e19b538e8d4097b721
Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/fe26ccb3ec3b981c2321951df210402a6f58bff8
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=196591162&repo=mozilla-inbound&lineNumber=44657

Comment 67

[Botond Ballo [:botond]]

The test failure in question is an assertion failure in a worker thread that looks like it's doing audio processing (the assertion is in a TimeStamp function called by SoundTouch::operator=).

I'm having a hard time seeing how that can be caused by this patch, but I'll do some before-and-after Try pushes to be sure.

Comment 68

[Botond Ballo [:botond]]

With patches rebased on latest inbound:

Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4268c2ba3273998a85c6a26f805bf8190206ebc4
After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4388832778188301456c3ffc93ce7bfa244bf3

Comment 69

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #68)
> With patches rebased on latest inbound:
> 
> Before:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4268c2ba3273998a85c6a26f805bf8190206ebc4
> After:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1a4388832778188301456c3ffc93ce7bfa244bf3

These builds are indeed showing the patches causing the test to permafail. Very strange...

Comment 70

[Botond Ballo [:botond]]

Here is a Try push with just the first patch ("EnumSet enhancements") applied:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8a88670cec3072ec8901f821748e71ddf5007a3

The test is permafiling in that push, suggesting those EnumSet changes are somehow responsible.

Comment 71

[Botond Ballo [:botond]]

I have more specifically narrowed down the failure to the addition of EnumSet<T>::operator=(T):

Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8880d268e389949fd169efac1ab56adfff94829e
After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a92808012039640a3bcf3df608f389f475d06717

Comment 72

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #71)
> I have more specifically narrowed down the failure to the addition of
> EnumSet<T>::operator=(T):

This suggests that there is some existing code in the codebase, whose meaning is changed by the addition of this operator=.

To find that code, I added a |static_assert(false, "");| to the body of the added operator (except I had to spell |false| as |sizeof(T) == 0| to make the condition dependent so the static_assert only fires when the assignment operator is actually instantiated, rather than every time EnumSet.h is included).

It turned out to be the following line of code in ScriptPreloader [1]:

  mProcessTypes = {};

(where mProcessTypes is an EnumSet<ProcessType>).

Adding the operator= changes the meaning of this code, from assigning a default-constructed EnumSet value (which doesn't have any ProcessType flags set), to assigning a default-constructed ProcessType value (which yields an EnumSet with that one ProcessType flag set).

That's one of the more impressive instances of "action at a distance" that I've seen in recent times...

[1] https://searchfox.org/mozilla-central/rev/43969de34f5d4b113133d090f024e5eed7a82af0/js/xpconnect/loader/ScriptPreloader.cpp#1165

Comment 73

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #72)
> It turned out to be the following line of code in ScriptPreloader [1]:
> 
>   mProcessTypes = {};
> 
> (where mProcessTypes is an EnumSet<ProcessType>).
> 
> Adding the operator= changes the meaning of this code, from assigning a
> default-constructed EnumSet value (which doesn't have any ProcessType flags
> set), to assigning a default-constructed ProcessType value (which yields an
> EnumSet with that one ProcessType flag set).

Here is a reduced testcase:

  #include <iostream>
  
  enum E { A, B };
  
  template <typename T>
  struct EnumSet  {
    EnumSet() : mValue(0) {}
    //void operator=(T t) { mValue = (1 << t); }
    int mValue;
  };
  
  int main() {
    EnumSet<E> e;
    e = {};
    std::cout << e.mValue << "\n";
  }

As is, this code prints "0".

If you uncomment the operator=(T), it prints "1". This is the case even if an explicit operator=(const EnumSet&) is added.

Comment 74

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #73)
> If you uncomment the operator=(T), it prints "1". This is the case even if
> an explicit operator=(const EnumSet&) is added.

This is a consequence of overload resolution rules. Quoting from [1]

  "Note that, if a non-template assignment operator from some 
   non-class type is available, it is preferred over the 
   copy/move assignment in E1 = {} because {} to non-class is 
   an identity conversion, which outranks the user-defined 
   conversion from {} to a class type."

(Thanks to Nika for finding this!)

[1] https://en.cppreference.com/w/cpp/language/operator_assignment

Comment 75

[Botond Ballo [:botond]]

Now, for what to do about it:

I think it would be surprising if this code:

  mProcessTypes = {};

had any other behaviour than setting the EnumSet to be empty.

So, my suggestion is to:

  - Remove the EnumSet::operator=(T)

  - Change places that use it, like this:

      result = CompositorHitTestFlags::eVisibleToHitTest;

    to instead be:

      result = {CompositorHitTestFlags::eVisibleToHitTest};

    This makes it more clear that the object on the left
    hand side is a set of values of type CompositorHitTestFlags,
    without adding much verbosity.

    This should compile without adding any other members to
    EnumSet, since the existing initializer_list constructor
    should be matched (and then the default assignment operator
    will be called).

Daniel, would you like to revise the patch as described?

Comment 76

[Botond Ballo [:botond]]

(Ehsan and I also talked about possibly banning the syntactic form "obj = {}" via static analysis. We can pursue that as a follow-up.)

Comment 77

[zielas.daniel]

(In reply to Botond Ballo [:botond] from comment #75)
> Daniel, would you like to revise the patch as described?

Sure, I just would like to investigate the issue a little bit first before I will do the changes if this is not a problem.

Comment 78

[zielas.daniel]

Indeed adding assignment operator is resulting with a surprising behavior.

(In reply to Botond Ballo [:botond] from comment #75)
> Now, for what to do about it:
> 
> I think it would be surprising if this code:
> 
>   mProcessTypes = {};
> 
> had any other behaviour than setting the EnumSet to be empty.
> 
> So, my suggestion is to:
> 
>   - Remove the EnumSet::operator=(T)
> 
>   - Change places that use it, like this:
> 
>       result = CompositorHitTestFlags::eVisibleToHitTest;
> 
>     to instead be:
> 
>       result = {CompositorHitTestFlags::eVisibleToHitTest};
> 
>     This makes it more clear that the object on the left
>     hand side is a set of values of type CompositorHitTestFlags,
>     without adding much verbosity.
> 
>     This should compile without adding any other members to
>     EnumSet, since the existing initializer_list constructor
>     should be matched (and then the default assignment operator
>     will be called).

It looks like the solution is also surprising simple. Removing EnumSet::operator=(T) solves the issue without changing source code from 'result = CompositorHitTestFlags::eVisibleToHitTest;' into 'result = {CompositorHitTestFlags::eVisibleToHitTest};' because the expression 'result = CompositorHitTestFlags::eVisibleToHitTest;' is invoking a ctor 'EnumSet(T aEnum)'.

Comment 79

[zielas.daniel]

Attachment: bug_1420996_5-EnumSet.patch

Comment 80

[zielas.daniel]

Attachment: bug_1420996_5-Replace_CompositorHitTestInfo.patch

Please review the changes.

Try Push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26b02cf198a42ebab62e69e176c60b12c6bf9bf9&selectedJob=201502024

Comment 81

[Botond Ballo [:botond]]

(In reply to zielas.daniel from comment #80)
> Try Push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=26b02cf198a42ebab62e69e176c60b12c6bf9bf9&selectedJob=2
> 01502024

There are a few test failures which are _probably_ unrelated, but I did a Try push with the base commit of the patches just in case:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=345954a2d44d296a28da7442c525f44d60b72281

Comment 82

[Botond Ballo [:botond]]

Ok, I'm seeing the same failures on the Try push for the base commit, so this patch should be good to land.

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e07824e36062
EnumSet enhancement. r=botond,Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/712cd59c2437
Replace CompositorHitTestInfo with an EnumSet. r=botond

Comment 84

[Botond Ballo [:botond]]

Comment on attachment 9011960 [details] [diff] [review]
bug_1420996_5-EnumSet.patch

(Going to carry forward Waldo's r+ from the previous version.)

Comment 85

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e07824e36062
https://hg.mozilla.org/mozilla-central/rev/712cd59c2437

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a995ac0742ab
Follow-up to fix debug logging code. r=me and DONTBUILD because NPOTB

Comment 87

[Cristina Coroiu [:ccoroiu]]

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

Comment 88

[Ionuț Goldan [:igoldan]]

Perf wins for push from comment 85:

== Change summary for alert #16209 (as of Wed, 26 Sep 2018 17:40:27 GMT) ==

Improvements:

  6%  tscrollx windows10-64-qr opt e10s stylo     2.10 -> 1.97

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16209

Comment 89

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #88)
>   6%  tscrollx windows10-64-qr opt e10s stylo     2.10 -> 1.97

This looks like it's actually from bug 1493867 which landed just before. That's a more likely culprit anyway since it explicitly touches the talos code. I retriggered the job a few more times to verify. ni to :igoldan as a FYI

Comment 90

[Kartikaya Gupta (email:[e-mail])]

The retriggers seem to confirm: https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-inbound,1686555,1,1&series=autoland,1686482,1,1&zoom=1537993262342.5225,1537994728638.61,1.6925280295851186,2.2216900719173283

Comment 91

[Ionuț Goldan [:igoldan]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #89)
> (In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment
> #88)
> >   6%  tscrollx windows10-64-qr opt e10s stylo     2.10 -> 1.97
> 
> This looks like it's actually from bug 1493867 which landed just before.
> That's a more likely culprit anyway since it explicitly touches the talos
> code. I retriggered the job a few more times to verify. ni to :igoldan as a
> FYI

Oh, sorry! You are right. Somehow I missed that, but thank you for correcting.