Closed Bug 1420996 Opened 2 years ago Closed Last year

Replace CompositorHitTestInfo with an EnumSet

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

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

References

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(2 files, 10 obsolete files)

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
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.
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
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.
Flags: needinfo?(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?
Flags: needinfo?(botond)
Ah yes, now I see.  The bitFor method.  Thanks.
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?
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
Hopefully I'm on the right track.
Thanks for the patch! I assigned the bug to you to reflect that you're working on it.
Assignee: nobody → spencerb
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).
Depends on: 1425926
(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.
Note, bug 1425926 has been fixed, so work here can proceed.
Attached patch CompositorHitTestInfo.h (obsolete) — Splinter Review
Thanks.
Here is CompositorHitTestInfo.h.
I am currently working on changing all the usages of CompositorHitTestInfo.
Hmm.  I guess I am still getting used to the workflow.  That was definitely not the right diff.
I think this is correct.
Attachment #8937465 - Attachment is obsolete: true
Attachment #8945236 - Attachment is obsolete: true
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.
Attachment #8945245 - Flags: feedback+
(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.
(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"
Hey spencerb, how is the rest of the patch here coming? Anything I can help with?
Attached patch In-Progress Patch (obsolete) — Splinter Review
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?
Attachment #8945245 - Attachment is obsolete: true
Scratch that, there are a few more usages besides that I still haven't adjusted yet, although that doesn't affect my question.
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.
(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.
Depends on: 1427229
(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.
(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).
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!
Hi spencerb, are you still working on this?
Flags: needinfo?(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.
Flags: needinfo?(spencerb)
Cool, thanks for the update!
Hi spencerb, just wanted to check in again. How are things going with this patch?
Flags: needinfo?(spencerb)
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 :)
Assignee: spencerb → nobody
Flags: needinfo?(spencerb)
Hello, I would like to take this one.
(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!
Assignee: nobody → zielas.daniel
Attached patch bug_1420996.patch (obsolete) — Splinter Review
Hello,

Please review my patch. Below is a Try Server check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f043436c3d6eb13a47653b684e33eeab9645978d
Attachment #8987732 - Flags: review?(botond)
Attachment #8952582 - Attachment is obsolete: true
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.
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).
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
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?
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.
(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.
(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.
(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
Attached patch bug_1420996_2.patch (obsolete) — Splinter Review
Please review the new patch.

Here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a952eca9b4008e9472018d1dbd0eba81b3a4a8a
Attachment #8987732 - Attachment is obsolete: true
Attachment #8987732 - Flags: review?(botond)
Attachment #8988556 - Flags: review?(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.
Attachment #8988556 - Flags: review?(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?
Attachment #8988556 - Flags: feedback?(bugmail)
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).
(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.
Thank you for the review!


(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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'?
Flags: needinfo?(botond)
(In reply to zielas.daniel from comment #49)
> Thank you for the review!
> 
> 
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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:kats@mozilla.com) 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.
Flags: needinfo?(botond)
(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.
(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.
Attached patch bug_1420996_3-EnumSet.patch (obsolete) — Splinter Review
Attachment #8988556 - Attachment is obsolete: true
Attachment #8994603 - Flags: review?(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>();
===
Attachment #8994604 - Flags: review?(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
Attachment #8994603 - Flags: review?(jwalden+bmo)
Attachment #8994603 - Flags: review?(botond)
Attachment #8994603 - Flags: feedback+
Attachment #8994604 - Flags: feedback+
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.
Attachment #8994603 - Flags: review?(jwalden+bmo) → review+
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");
Flags: needinfo?(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 :))
Flags: needinfo?(botond)
Attached patch bug_1420996_4-EnumSet.patch (obsolete) — Splinter Review
Attachment #8994603 - Attachment is obsolete: true
Attachment #8994604 - Attachment is obsolete: true
Attachment #8999437 - Flags: review?(jwalden+bmo)
Attachment #8999437 - Flags: review?(botond)
Attachment #8999437 - Flags: review?(jwalden+bmo) → review+
Apologies for the long delay here. I've been on vacation and am just catching up on reviews and such now.
Attachment #8999437 - Flags: review?(botond) → review+
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
Attachment #8999438 - Flags: review?(botond) → review+
Pushed by bballo@mozilla.com:
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
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.
Flags: needinfo?(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...
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.
(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
(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.
(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
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?
Flags: needinfo?(zielas.daniel)
(Ehsan and I also talked about possibly banning the syntactic form "obj = {}" via static analysis. We can pursue that as a follow-up.)
(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.
Flags: needinfo?(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)'.
Attachment #8999437 - Attachment is obsolete: true
Attachment #8999438 - Attachment is obsolete: true
Attachment #9011960 - Flags: review?(jwalden+bmo)
Attachment #9011960 - Flags: review?(botond)
Attachment #9011960 - Flags: review?(botond) → review+
Attachment #9011961 - Flags: review?(botond) → review+
(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
Ok, I'm seeing the same failures on the Try push for the base commit, so this patch should be good to land.
Pushed by bballo@mozilla.com:
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 on attachment 9011960 [details] [diff] [review]
bug_1420996_5-EnumSet.patch

(Going to carry forward Waldo's r+ from the previous version.)
Attachment #9011960 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/e07824e36062
https://hg.mozilla.org/mozilla-central/rev/712cd59c2437
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a995ac0742ab
Follow-up to fix debug logging code. r=me and DONTBUILD because NPOTB
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
(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
Flags: needinfo?(igoldan)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(igoldan)
Blocks: 1496830
See Also: → 1496870
You need to log in before you can comment on or make changes to this bug.