1316251 - [Pointer Event] Separate pointer event implementation from PresShell

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P2)

Description

[Ming-Chou Shih [:stone]]

Some of pointer event behaviors are implemented in PresShell and hard to understand. We should move them to a class and maybe trigger it in EventStateManager::PreHandleEvent(). See [1] for more details.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1303704#c31

Comment 1

[Ming-Chou Shih [:stone]]

Attachment: Part1: Separate pointer event implementation from PresShell

Only move pointer event related implementation to a new file and rename some functions in this patch.

Comment 2

[Ming-Chou Shih [:stone]]

Attachment: Part2: Revise pointer event implementation

Comment 3

[Ming-Chou Shih [:stone]]

Trying to trigger PE implementation in ESM but got some problems.

1. We get the target frame in PresShell
2. PE (setPointerCapture) could change the target content of event dispatching.
3. Spec says we should process pending pointer capture before processing the next pointer event.

We could generate PE events and process pending pointer capture in ESM::PreHandleEvent. But it's too late to change the event target frame since PresShell already chooses it. Changing the event target frame and content in ESM might be a bad idea.

Comment 4

[Ming-Chou Shih [:stone]]

Attachment: Part1: Separate pointer event implementation from PresShell

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Part2: Revise pointer event implementation

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Part3: Trigger pointer event implementation in EventStateManager

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Part1: Separate pointer event implementation from PresShell

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Part2: Revise pointer event implementation

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Part3: Trigger pointer event implementation in EventStateManager

Comment 10

[Ming-Chou Shih [:stone]]

Comment on attachment 8910682 [details] [diff] [review]
Part1: Separate pointer event implementation from PresShell

This patch moves pointer event related implementations to a new class PointerEventUtils.

Comment 11

[Ming-Chou Shih [:stone]]

Comment on attachment 8910683 [details] [diff] [review]
Part2: Revise pointer event implementation

Revised some pointer event implementations in this patch.

Comment 12

[Ming-Chou Shih [:stone]]

Comment on attachment 8910684 [details] [diff] [review]
Part3: Trigger pointer event implementation in EventStateManager

Invoked pointer event related functions in ESM.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry, I don't work today. So, I could check it next Monday. If you need to land them ASAP, ask smaug to review them.

Comment 14

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #13)
> Sorry, I don't work today. So, I could check it next Monday. If you need to
> land them ASAP, ask smaug to review them.
This is not urgent. Take your time.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8910682 [details] [diff] [review]
Part1: Separate pointer event implementation from PresShell

>diff --git a/dom/events/PointerEventUtils.cpp b/dom/events/PointerEventUtils.cpp
>+#include "PointerEventUtils.h"
>+#include "PointerEvent.h"
>+#include "nsIFrame.h"
>+
>+using namespace mozilla;

Please make all code in this file in mozilla namespace rather than in the global namespace.

>+/* static */ bool
>+PointerEventUtils::IsPointerEventEnabled()
>+{
>+  return sPointerEventEnabled;
>+}
>+
>+/* static */ bool
>+PointerEventUtils::IsPointerEventImplicitCaptureForTouchEnabled()
>+{
>+  return sPointerEventImplicitCapture;
>+}

I wonder, even if pointer event is disabled, sPointerEventImplicitCapture can be true?

>+/* static */ void
>+PointerEventUtils::UpdateActivePointerState(WidgetEvent* aEvent)

Well, why are you changing aEvent from WidgetGUIEvent* to WidgetEvent*? The only caller is PresShell::HandleEvent(), which guarantees the event is at least WidgetGUIEvent. So, you should keep using WidgetGUIEvent* here.

>+{
>+  if (!PointerEventUtils::IsPointerEventEnabled()) {
>+    return;
>+  }

This is checked by PresShell.  I think that PresShell shouldn't check this and it's checked here.

>+/* static */ void
>+PointerEventUtils::SetPointerCaptureById(uint32_t aPointerId,
>+                                         nsIContent* aContent)
>+{
>+  MOZ_ASSERT(aContent != nullptr);

nit: MOZ_ASSERT(aContent);

>+/* static */ void
>+PointerEventUtils::CheckPointerCaptureState(WidgetGUIEvent* aEvent)

Hmm, both callers of this method can guarantee aEvent is a WidgetPointerEvent* with nothing special. So, you shouldn't change the type from WidgetPointerEvent* to WidgetGUIEvent*.

>+{
>+  // Handle pending pointer capture before any pointer events except
>+  // gotpointercapture / lostpointercapture.
>+  if (!IsPointerEventEnabled() || aEvent->mClass != ePointerEventClass) {

Then, you don't need to check if mClass is ePointerEventClass here.

>+  WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
>+  if (!pointerEvent) {
>+    return;
>+  }

And you can avoid this redundant virtual call.

>+/* static */ nsIContent*
>+PointerEventUtils::GetPointerCapturingContent(uint32_t aPointerId)
>+{
>+  PointerCaptureInfo* pointerCaptureInfo = GetPointerCaptureInfo(aPointerId);
>+  if (pointerCaptureInfo) {
>+    return pointerCaptureInfo->mOverrideContent;
>+  }
>+  return nullptr;
>+}
>+
>+/* static */ void
>+PointerEventUtils::ReleaseIfCaptureByDescendant(nsIContent* aContent)
>+{
>+  // We should check that aChild does not contain pointer capturing elements.
>+  // If it does we should release the pointer capture for the elements.
>+  for (auto iter = sPointerCaptureList->Iter(); !iter.Done(); iter.Next()) {
>+    PointerCaptureInfo* data = iter.UserData();
>+    if (data && data->mPendingContent &&
>+        nsContentUtils::ContentIsDescendantOf(data->mPendingContent, aContent)) {

nit: too long line. Please wrap after |data->mPendingContent,|.

>+/* static */ void
>+PointerEventUtils::PreHandlePointerEventsPreventDefault(
>+                     WidgetPointerEvent* aPointerEvent,
>+                     WidgetEvent* aMouseOrTouchEvent)

You should keep using WidgetGUIEvent* for aMouseOrTouchEvent. There is no reason to use super class in this case.

>+/* static */ void
>+PointerEventUtils::PostHandlePointerEventsPreventDefault(
>+                     WidgetPointerEvent* aPointerEvent,
>+                     WidgetEvent* aMouseOrTouchEvent)

Same.

>+/* static */ nsresult
>+PointerEventUtils::DispatchPointerFromMouseOrTouch(PresShell* aShell,
>+                                                   nsIFrame* aFrame,
>+                                                   WidgetGUIEvent* aEvent,
>+                                                   bool aDontRetargetEvents,
>+                                                   nsEventStatus* aStatus,
>+                                                   nsIContent** aTargetContent)
>+{
>...
>+    WidgetPointerEvent event(*mouseEvent);
>+    event.pointerId = mouseEvent->pointerId;
>+    event.inputSource = mouseEvent->inputSource;
>+    event.mMessage = pointerMessage;
>+    event.button = button;
>+    event.buttons = mouseEvent->buttons;
>+    event.pressure = event.buttons ?
>+                     mouseEvent->pressure ? mouseEvent->pressure : 0.5f :
>+                     0.0f;
>+    event.convertToPointer = mouseEvent->convertToPointer = false;
>+    PreHandlePointerEventsPreventDefault(&event, aEvent);
>+    aShell->HandleEvent(aFrame, &event, aDontRetargetEvents, aStatus,
>+                        aTargetContent);

In this method, there is no guarantee that aShell won't be destroyed during dispatching the event. So, please grab it with RefPtr<PresShell>.

>+    for (uint32_t i = 0; i < touchEvent->mTouches.Length(); ++i) {
>+      mozilla::dom::Touch* touch = touchEvent->mTouches[i];
>+      if (!TouchManager::ShouldConvertTouchToPointer(touch, touchEvent)) {
>+        continue;
>+      }
>+
>+      WidgetPointerEvent event(touchEvent->IsTrusted(), pointerMessage,
>+                               touchEvent->mWidget);
>+      event.mIsPrimary = i == 0;
>+      event.pointerId = touch->Identifier();
>+      event.mRefPoint = touch->mRefPoint;
>+      event.mModifiers = touchEvent->mModifiers;
>+      event.mWidth = touch->RadiusX(CallerType::System);
>+      event.mHeight = touch->RadiusY(CallerType::System);
>+      event.tiltX = touch->tiltX;
>+      event.tiltY = touch->tiltY;
>+      event.mTime = touchEvent->mTime;
>+      event.mTimeStamp = touchEvent->mTimeStamp;
>+      event.mFlags = touchEvent->mFlags;
>+      event.button = button;
>+      event.buttons = buttons;
>+      event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_TOUCH;
>+      event.convertToPointer = touch->convertToPointer = false;
>+      PreHandlePointerEventsPreventDefault(&event, aEvent);
>+      aShell->HandleEvent(aFrame, &event, aDontRetargetEvents, aStatus,
>+                          aTargetContent);

Same. (I'd like you to grab it outside of the for loop for reducing the grabbing cost.)

>+/* static */ void
>+PointerEventUtils::DispatchGotOrLostPointerCaptureEvent(
>+                     bool aIsGotCapture,
>+                     const WidgetPointerEvent* aPointerEvent,
>+                     nsIContent* aCaptureTarget)
>+{
>+  nsIDocument* targetDoc = aCaptureTarget->OwnerDoc();
>+  nsCOMPtr<nsIPresShell> shell = targetDoc->GetShell();
>+  NS_ENSURE_TRUE_VOID(shell);

Please rewrite this as:

if (NS_WARN_IF(!shell)) {
  return;
}

>+  if (!aIsGotCapture && !aCaptureTarget->IsInUncomposedDoc()) {
>+    // If the capturing element was removed from the DOM tree, fire
>+    // ePointerLostCapture at the document.
>+    PointerEventInit init;
>+    init.mPointerId = aPointerEvent->pointerId;
>+    init.mBubbles = true;
>+    init.mComposed = true;
>+    ConvertPointerTypeToString(aPointerEvent->inputSource, init.mPointerType);
>+    init.mIsPrimary = aPointerEvent->mIsPrimary;
>+    RefPtr<mozilla::dom::PointerEvent> event;

You don't need |mozilla::|. And perhaps, this file should use using dom; in mozilla namespace block.

>+    event = PointerEvent::Constructor(aCaptureTarget,
>+                                      NS_LITERAL_STRING("lostpointercapture"),
>+                                      init);
>+    bool dummy;
>+    targetDoc->DispatchEvent(event->InternalDOMEvent(), &dummy);
>+    return;
>+  }
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  WidgetPointerEvent localEvent(aPointerEvent->IsTrusted(),
>+                                aIsGotCapture ? ePointerGotCapture :
>+                                                ePointerLostCapture,
>+                                aPointerEvent->mWidget);
>+  localEvent.AssignPointerEventData(*aPointerEvent, true);
>+  nsresult rv = shell->HandleEventWithTarget(
>+                         &localEvent,
>+                         aCaptureTarget->GetPrimaryFrame(),
>+                         aCaptureTarget, &status);
>+  NS_ENSURE_SUCCESS_VOID(rv);

Please use:

if (NS_WARN_IF(NS_FAILED(rv))) {
  return;
}

>diff --git a/dom/events/PointerEventUtils.h b/dom/events/PointerEventUtils.h
>+#ifndef mozilla_PointerEventUtils_h
>+#define mozilla_PointerEventUtils_h
>+
>+#include "mozilla/EventForwards.h"
>+#include "mozilla/PresShell.h"

Looks like that you don't need to include PresShell.h in this header file. Why don't you just use forward declaration for PresShell?

Then, perhaps, you also need to declare nsIContent, nsIFrame, etc.

>+
>+namespace mozilla {
>+
>+class PointerEventHelper final
>+{
>+public:
>+  PointerEventHelper();
>+private:
>+};

I don't understand this class. Please drop this from this patch even if this will be implemented in a following patch.

>+class PointerEventUtils final
>+{
>+public:
>+  static void Initialize();
>+  static void InitializeStatics();
>+  static void ReleaseStatics();
>+  static bool IsPointerEventEnabled();
>+  static bool IsPointerEventImplicitCaptureForTouchEnabled();
>+  static void UpdateActivePointerState(WidgetEvent* aEvent);
>+  static void SetPointerCaptureById(uint32_t aPointerId, nsIContent* aContent);
>+  static PointerCaptureInfo* GetPointerCaptureInfo(uint32_t aPointerId);
>+  static void ReleasePointerCaptureById(uint32_t aPointerId);

I'd be happy if you'd add explanation to each method except its name is self-documented. E.g., Initialize() is unclear when it's called and what's different from InitializeStatics().

>+private:
>+  // GetPointerType returns pointer type like mouse, pen or touch for pointer
>+  // event with pointerId
>+  static uint16_t GetPointerType(uint32_t aPointerId);

I'd like you to explain the result is one of nsIDOMMouseEvent::MOZ_SOURCE_*.

>+  // GetPointerPrimaryState returns state of attribute isPrimary for pointer
>+  // event with pointerId
>+  static bool GetPointerPrimaryState(uint32_t aPointerId);
>+  static void DispatchGotOrLostPointerCaptureEvent(

Please insert an empty line before this line since above comment isn't related to this method.

>+                bool aIsGotCapture,
>+                const mozilla::WidgetPointerEvent* aPointerEvent,

nit: |mozilla::| sholdn't be necessary because here is in |::mozilla|.

>diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp
>@@ -7149,22 +6780,22 @@ PresShell::HandleEvent(nsIFrame* aFrame,
>   // Update the latest focus sequence number with this new sequence number
>   if (mAPZFocusSequenceNumber < aEvent->mFocusSequenceNumber) {
>     mAPZFocusSequenceNumber = aEvent->mFocusSequenceNumber;
> 
>     // Schedule an empty transaction to transmit this focus update
>     aFrame->SchedulePaint(nsIFrame::PAINT_COMPOSITE_ONLY);
>   }
> 
>-  if (sPointerEventEnabled) {
>+  if (PointerEventUtils::IsPointerEventEnabled()) {
>     AutoWeakFrame weakFrame(aFrame);
>     nsCOMPtr<nsIContent> targetContent;
>-    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents,
>-                                    aEventStatus,
>-                                    getter_AddRefs(targetContent));
>+    PointerEventUtils::DispatchPointerFromMouseOrTouch(
>+                         this, aFrame, aEvent, aDontRetargetEvents,
>+                         aEventStatus, getter_AddRefs(targetContent));

Checking PointerEventUtils::IsPointerEventEnabled() looks okay. Although, PresShell shouldn't check it as below.

>@@ -7204,18 +6835,18 @@ PresShell::HandleEvent(nsIFrame* aFrame,
>         // If the event is consumed, cancel APZC panning by setting
>         // mMultipleActionsPrevented.
>         aEvent->mFlags.mMultipleActionsPrevented = true;
>         return NS_OK;
>       }
>     }
>   }
> 
>-  if (sPointerEventEnabled) {
>-    UpdateActivePointerState(aEvent);
>+  if (PointerEventUtils::IsPointerEventEnabled()) {
>+    PointerEventUtils::UpdateActivePointerState(aEvent);

So, as I said above, I think that whether pointer event is enabled isn't checked by outer classes unless it causes performance problem.

>@@ -7685,17 +7318,17 @@ PresShell::HandleEvent(nsIFrame* aFrame,
>           }
>         }
>       }
>     }
> 
>     // Before HandlePositionedEvent we should save mPointerEventTarget in some
>     // cases
>     AutoWeakFrame weakFrame;
>-    if (sPointerEventEnabled && aTargetContent &&
>+    if (PointerEventUtils::IsPointerEventEnabled() && aTargetContent &&
>         ePointerEventClass == aEvent->mClass) {

Same here. Unless pointer event is enabled, coming events should never be ePointerEventClass. So, PresShell shouldn't need to check PointerEventUtils::IsPointerEventEnabled().

>@@ -7710,17 +7343,17 @@ PresShell::HandleEvent(nsIFrame* aFrame,
>       // now ask the subshell to dispatch it normally.
>       rv = shell->HandlePositionedEvent(frame, aEvent, aEventStatus);
>     } else {
>       rv = HandlePositionedEvent(frame, aEvent, aEventStatus);
>     }
> 
>     // After HandlePositionedEvent we should reestablish
>     // content (which still live in tree) in some cases
>-    if (sPointerEventEnabled && aTargetContent &&
>+    if (PointerEventUtils::IsPointerEventEnabled() && aTargetContent &&
>         ePointerEventClass == aEvent->mClass) {

Ditto.

> void nsIPresShell::InitializeStatics()
> {
>-  MOZ_ASSERT(!sPointerCaptureList, "InitializeStatics called multiple times!");
>-  sPointerCaptureList =
>-    new nsClassHashtable<nsUint32HashKey, PointerCaptureInfo>;
>-  sActivePointersIds = new nsClassHashtable<nsUint32HashKey, PointerInfo>;
>+  PointerEventUtils::InitializeStatics();
> }

I don't think the call of PointerEventUtils::InitializeStatics() should be wrapped with nsIPresShell::InitializeStatics(). nsLayoutStatics should call PointerEventUtils::InitializeStatics() directly instead.

> void nsIPresShell::ReleaseStatics()
> {
>-  MOZ_ASSERT(sPointerCaptureList, "ReleaseStatics called without Initialize!");
>-  delete sPointerCaptureList;
>-  sPointerCaptureList = nullptr;
>-  delete sActivePointersIds;
>-  sActivePointersIds = nullptr;
>+  PointerEventUtils::ReleaseStatics();
> }

Same.


I'd like to check newer patch, so, temporarily r-'ing but looks really good.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ming-Chou Shih [:stone] from comment #10)
> Comment on attachment 8910682 [details] [diff] [review]
> Part1: Separate pointer event implementation from PresShell
> 
> This patch moves pointer event related implementations to a new class
> PointerEventUtils.

Ah, and please include this kind of explanation in the commit message.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8910683 [details] [diff] [review]
Part2: Revise pointer event implementation

># HG changeset patch
># User Stone Shih <[e-mail]>
># Date 1504690108 -28800
>#      Wed Sep 06 17:28:28 2017 +0800
># Node ID 086fb86e6745121298d702b7dda096c9dc976f2f
># Parent  abe25ece3513566a9be293906475bb7da73ae20e
>Bug 1316251 Part2: Revise pointer event implementation

Please include explanation what this patch does.

>diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
>@@ -3201,31 +3191,28 @@ EventStateManager::PostHandleEvent(nsPre
>               fm->SetFocusedWindow(mDocument->GetWindow());
>             }
>           }
>         }
>       }
>       SetActiveManager(this, activeContent);
>     }
>     break;
>-  case ePointerCancel: {
>-    if(WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent()) {
>-      GenerateMouseEnterExit(mouseEvent);
>-    }
>-    // After firing the pointercancel event, a user agent must also fire a
>-    // pointerout event followed by a pointerleave event.
>-    MOZ_FALLTHROUGH;
>-  }
>+  case ePointerCancel:
>   case ePointerUp: {

I don't understand the reason why this change is reasonable with other changes in this patch. You should explain with comment or commit message.

>     WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
>-    // After UP/Cancel Touch pointers become invalid so we can remove relevant helper from Table
>-    // Mouse/Pen pointers are valid all the time (not only between down/up)
>     if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
>+      // After firing the pointercancel event, a user agent must also fire a
>+      // pointerout event followed by a pointerleave event.
>+      GenerateMouseEnterExit(pointerEvent);

So, looks like that GenerateMouseEnterExit() is called only when it's caused by touch device rather than mouse event?

>@@ -4258,16 +4245,19 @@ GetWindowClientRectCenter(nsIWidget* aWi
> void
> EventStateManager::GeneratePointerEnterExit(EventMessage aMessage,
>                                             WidgetMouseEvent* aEvent)
> {
>+  if (!PointerEventUtils::IsPointerEventEnabled()) {
>+    return;
>+  }

Yeah, I like this kind of capsuling.

>diff --git a/dom/events/PointerEventUtils.cpp b/dom/events/PointerEventUtils.cpp
> /* static */ void
> PointerEventUtils::Initialize()
> {
>-  static bool addedPointerEventEnabled = false;
>-  if (!addedPointerEventEnabled) {
>-    Preferences::AddBoolVarCache(&sPointerEventEnabled,
>-                                 "dom.w3c_pointer_events.enabled", true);
>-    addedPointerEventEnabled = true;
>+  static bool initialized = false;
>+  if (initialized) {
>+    return;
>   }
>-  static bool addedPointerEventImplicitCapture = false;
>-  if (!addedPointerEventImplicitCapture) {
>-    Preferences::AddBoolVarCache(&sPointerEventImplicitCapture,
>-                                 "dom.w3c_pointer_events.implicit_capture",
>-                                 true);
>-    addedPointerEventImplicitCapture = true;
>-  }
>+  initialized = true;
>+  Preferences::AddBoolVarCache(&sPointerEventEnabled,
>+                               "dom.w3c_pointer_events.enabled", true);
>+  Preferences::AddBoolVarCache(&sPointerEventImplicitCapture,
>+                               "dom.w3c_pointer_events.implicit_capture", true);

Sure, makes sense.

>@@ -219,16 +215,39 @@ PointerEventUtils::CheckPointerCaptureSt
>+/* static */ nsIFrame*
>+PointerEventUtils::MaybeCaptureEvent(nsIFrame* aFrame, WidgetGUIEvent* aEvent)

I don't understand the meaning of this method name even after I read this implementation. How about GetTargetFrame() and rename aFrame to aFrameUnderCursor or something?

>+{
>+  if (!IsPointerEventEnabled() || (aEvent->mClass != ePointerEventClass &&
>+                                   aEvent->mClass != eMouseEventClass) ||
>+      (aEvent->mMessage == ePointerDown && aEvent->mMessage == eMouseDown)) {
>+    return aFrame;
>+  }

Looks like that |!IsPointerEventEnabled()| and |aEvent->mMessage == ePointerDown && aEvent->mMessage == eMouseDown| are makes sense in runtime check. How about to use MOZ_ASSERT for checking mClass?

>+  // Capture the event.
>+  WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent();
>+  if (!mouseEvent) {

Anyway, it's being checked here.

>-/* static */ nsresult
>+/* static */ void
> PointerEventUtils::DispatchPointerFromMouseOrTouch(PresShell* aShell,
>                                                    nsIFrame* aFrame,
>                                                    WidgetGUIEvent* aEvent,
>                                                    bool aDontRetargetEvents,
>                                                    nsEventStatus* aStatus,
>                                                    nsIContent** aTargetContent)
> {
>+  MOZ_ASSERT(PointerEventUtils::IsPointerEventEnabled() && aFrame && aEvent);

I like:

MOZ_ASSERT(PointerEventUtils::IsPointerEventEnabled());
MOZ_ASSERT(aFrame);
MOZ_ASSERT(aEvent);

Then, an assertion tells us which state is exactly wrong.

>diff --git a/dom/events/PointerEventUtils.h b/dom/events/PointerEventUtils.h
>--- a/dom/events/PointerEventUtils.h
>+++ b/dom/events/PointerEventUtils.h
>@@ -59,16 +59,17 @@ public:
>   // device, false otherwise.
>   // aActiveState is additional information, which shows state of pointer like
>   // button state for mouse.
>   static bool GetPointerInfo(uint32_t aPointerId, bool& aActiveState);
> 
>   // CheckPointerCaptureState checks cases, when got/lostpointercapture events
>   // should be fired.
>   static void CheckPointerCaptureState(WidgetGUIEvent* aEvent);
>+  static nsIFrame* MaybeCaptureEvent(nsIFrame* aFrame, WidgetGUIEvent* aEvent);

Please explain what this method does and what will be returned. IIUC,

/**
 * GetTargetFrame returns a target frame of aEvent.  If aEvent->mMessage is
 * eMouseDown or ePointerDown and there is a capturing content by pointer,
 * this returns its primary frame.  Otherwise, aFrameUnderCursor.
 *
 * @param aFrameUnderCursor    A frame under cursor.
 * @param aEvent               A mouse event or pointer event.
 * @return                     Target frame for aEvent.
 */


r-'ing for now because I don't understand the ESM change.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8910684 [details] [diff] [review]
Part3: Trigger pointer event implementation in EventStateManager

>Bug 1316251 Part3: Trigger pointer event implementation in EventStateManager

It makes sense to manage pointer event "state" in ESM. So, please add this reason to the commit message.

And looks like that you use MozReview mode in your local tree. But you just attached diff files to request the reviews. Why? Especially in PresShell::HandleEvent, I need to check before and after the changes wider.  MozReview makes that easier.

>diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
>@@ -3191,19 +3193,26 @@ EventStateManager::PostHandleEvent(nsPre
>               fm->SetFocusedWindow(mDocument->GetWindow());
>             }
>           }
>         }
>       }
>       SetActiveManager(this, activeContent);
>     }
>     break;
>+  case ePointerDown:
>+    PointerEventUtils::ProcessImplicitCapture(aTargetFrame, aEvent);

Is this really correct? Here is PostHandleEvent. However, the old code does this before PreHandleEvent.

Now, I think that PointerEventHandler or PointerEventManager might be better name for this helper class. But up to you.

>@@ -5103,16 +5114,17 @@ EventStateManager::ContentRemoved(nsIDoc
> 
>   // See bug 292146 for why we want to null this out
>   ResetLastOverForContent(0, mMouseEnterLeaveHelper, aContent);
>   for (auto iter = mPointersEnterLeaveHelper.Iter();
>        !iter.Done();
>        iter.Next()) {
>     ResetLastOverForContent(iter.Key(), iter.Data(), aContent);
>   }
>+  PointerEventUtils::ReleaseIfCaptureByDescendant(aContent);

Might be better to release capture first, but I have no idea. So, it must be okay.

>diff --git a/dom/events/PointerEventUtils.cpp b/dom/events/PointerEventUtils.cpp
>+/* static */ void
>+PointerEventUtils::ProcessImplicitCapture(nsIFrame* aFrame, WidgetEvent* aEvent)

How about "PostHandlePointerDown"? Because others may add different job in this method. (But perhaps, this should be called by PreHandleEvent and named PreHandlePointerDown, isn't it?)

>+{
>+  if (!aFrame || !IsPointerEventEnabled() || !sPointerEventImplicitCapture ||

One is using wrapper method and the other is referring the static variable directly. I like the former.

>+      aEvent->mMessage != ePointerDown) {
>+    return;
>+  }
>+  WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
>+  if (!pointerEvent ||

Perhaps, NS_WARN_IF(!pointerEvent)?

>+  nsCOMPtr<nsIContent> target;
>+  aFrame->GetContentForEvent(aEvent, getter_AddRefs(target));
>+  while (target && !target->IsElement()) {
>+    target = target->GetParent();
>+  }
>+  if (target) {
>+    PointerEventUtils::SetPointerCaptureById(pointerEvent->pointerId, target);
>+  }

I wonder, looks like that you don't assume that target won't be nullptr. How about assert or warn if it's nullptr? E.g.,

if (NS_WARN_IF(!target)) {
  return;
}
PointerEventUtils::SetPointerCaptureById(pointerEvent->pointerId, target);

>+/* static */ void
>+PointerEventUtils::ProcessImplicitReleaseCapture(WidgetEvent* aEvent)

So, how about "PostHandlePointerUpOrCancel"?

I'd like to check new patch.

Comment 19

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17)
> Comment on attachment 8910683 [details] [diff] [review]
> Part2: Revise pointer event implementation
>  
> >diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
> >@@ -3201,31 +3191,28 @@ EventStateManager::PostHandleEvent(nsPre
> >               fm->SetFocusedWindow(mDocument->GetWindow());
> >             }
> >           }
> >         }
> >       }
> >       SetActiveManager(this, activeContent);
> >     }
> >     break;
> >-  case ePointerCancel: {
> >-    if(WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent()) {
> >-      GenerateMouseEnterExit(mouseEvent);
> >-    }
> >-    // After firing the pointercancel event, a user agent must also fire a
> >-    // pointerout event followed by a pointerleave event.
> >-    MOZ_FALLTHROUGH;
> >-  }
> >+  case ePointerCancel:
> >   case ePointerUp: {
> 
> I don't understand the reason why this change is reasonable with other
> changes in this patch. You should explain with comment or commit message.

The original implementation calls GenerateMouseEnterExit to fire pointerout and pointerleave when handling pointercancel. Now we only generate pointercancel from touchcancel.

> >     WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
> >-    // After UP/Cancel Touch pointers become invalid so we can remove relevant helper from Table
> >-    // Mouse/Pen pointers are valid all the time (not only between down/up)
> >     if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
> >+      // After firing the pointercancel event, a user agent must also fire a
> >+      // pointerout event followed by a pointerleave event.
> >+      GenerateMouseEnterExit(pointerEvent);
> 
> So, looks like that GenerateMouseEnterExit() is called only when it's caused
> by touch device rather than mouse event?

Here we call GenerateMouseEnterExit for touch devices here. I think the logic of pointercancel and pointerup for touch device could be the same so I try to merge them.

> >+{
> >+  if (!IsPointerEventEnabled() || (aEvent->mClass != ePointerEventClass &&
> >+                                   aEvent->mClass != eMouseEventClass) ||
> >+      (aEvent->mMessage == ePointerDown && aEvent->mMessage == eMouseDown)) {
> >+    return aFrame;
> >+  }
> 
> Looks like that |!IsPointerEventEnabled()| and |aEvent->mMessage ==
> ePointerDown && aEvent->mMessage == eMouseDown| are makes sense in runtime
> check. How about to use MOZ_ASSERT for checking mClass?
We capture the pointer when handling pointerdown. Mousedown should be fired to the same target as pointerdown. Here we should only capture pointer and mouse events except pointerdown and mousedown.

Comment 20

[Ming-Chou Shih [:stone]]

Attachment: Bug 1316251 Part1: Separate pointer event implementation from PresShell.

Review commit: https://reviewboard.mozilla.org/r/183118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183118/

Comment 21

[Ming-Chou Shih [:stone]]

Attachment: Bug 1316251 Part2: Revise pointer event implementation.

Review commit: https://reviewboard.mozilla.org/r/183120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183120/

Comment 22

[Ming-Chou Shih [:stone]]

Attachment: Bug 1316251 Part3: Trigger pointer event implementation in EventStateManager.

Review commit: https://reviewboard.mozilla.org/r/183122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183122/

Comment 23

[Ming-Chou Shih [:stone]]

Comment on attachment 8911742 [details]
Bug 1316251 Part1: Separate pointer event implementation from PresShell.

This is the first revision.

Comment 24

[Ming-Chou Shih [:stone]]

Comment on attachment 8911743 [details]
Bug 1316251 Part2: Revise pointer event implementation.

This is the first revision.

Comment 25

[Ming-Chou Shih [:stone]]

Comment on attachment 8911744 [details]
Bug 1316251 Part3: Trigger pointer event implementation in EventStateManager.

This is the first revision.

Comment 26

[Ming-Chou Shih [:stone]]

Comment on attachment 8911742 [details]
Bug 1316251 Part1: Separate pointer event implementation from PresShell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183118/diff/1-2/

Comment 27

[Ming-Chou Shih [:stone]]

Comment on attachment 8911743 [details]
Bug 1316251 Part2: Revise pointer event implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183120/diff/1-2/

Comment 28

[Ming-Chou Shih [:stone]]

Comment on attachment 8911744 [details]
Bug 1316251 Part3: Trigger pointer event implementation in EventStateManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183122/diff/1-2/

Comment 29

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18)
> Comment on attachment 8910684 [details] [diff] [review]
> Part3: Trigger pointer event implementation in EventStateManager
> 
> >Bug 1316251 Part3: Trigger pointer event implementation in EventStateManager
> >diff --git a/dom/events/PointerEventUtils.cpp b/dom/events/PointerEventUtils.cpp
> >+/* static */ void
> >+PointerEventUtils::ProcessImplicitCapture(nsIFrame* aFrame, WidgetEvent* aEvent)
> 
> How about "PostHandlePointerDown"? Because others may add different job in
> this method. (But perhaps, this should be called by PreHandleEvent and named
> PreHandlePointerDown, isn't it?)

I'm thinking to keep the terms 'implicit capture' and 'implicit release pointer capture' which match the PointerEvent spec. And we won't have to rename them once the spec changes. Is it make sense to you?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ming-Chou Shih [:stone] from comment #19)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17)
> > Comment on attachment 8910683 [details] [diff] [review]
> > Part2: Revise pointer event implementation
> >  
> > >diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
> > >@@ -3201,31 +3191,28 @@ EventStateManager::PostHandleEvent(nsPre
> > >               fm->SetFocusedWindow(mDocument->GetWindow());
> > >             }
> > >           }
> > >         }
> > >       }
> > >       SetActiveManager(this, activeContent);
> > >     }
> > >     break;
> > >-  case ePointerCancel: {
> > >-    if(WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent()) {
> > >-      GenerateMouseEnterExit(mouseEvent);
> > >-    }
> > >-    // After firing the pointercancel event, a user agent must also fire a
> > >-    // pointerout event followed by a pointerleave event.
> > >-    MOZ_FALLTHROUGH;
> > >-  }
> > >+  case ePointerCancel:
> > >   case ePointerUp: {
> > 
> > I don't understand the reason why this change is reasonable with other
> > changes in this patch. You should explain with comment or commit message.
> 
> The original implementation calls GenerateMouseEnterExit to fire pointerout
> and pointerleave when handling pointercancel. Now we only generate
> pointercancel from touchcancel.

Changing any behavior doesn't make sense for this bug. This bug's purpose is refactoring of PointerEvent implementation. So, such behavior change shouldn't be included in this bug.  Instead, you should file a new bug and change the behavior for conforming to the spec.

Separating code clean up bug and changing behavior bug is important for regression testers.

> > >     WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
> > >-    // After UP/Cancel Touch pointers become invalid so we can remove relevant helper from Table
> > >-    // Mouse/Pen pointers are valid all the time (not only between down/up)
> > >     if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
> > >+      // After firing the pointercancel event, a user agent must also fire a
> > >+      // pointerout event followed by a pointerleave event.
> > >+      GenerateMouseEnterExit(pointerEvent);
> > 
> > So, looks like that GenerateMouseEnterExit() is called only when it's caused
> > by touch device rather than mouse event?
> 
> Here we call GenerateMouseEnterExit for touch devices here. I think the
> logic of pointercancel and pointerup for touch device could be the same so I
> try to merge them.

And this is same. You're trying to change the behavior.

> > >+{
> > >+  if (!IsPointerEventEnabled() || (aEvent->mClass != ePointerEventClass &&
> > >+                                   aEvent->mClass != eMouseEventClass) ||
> > >+      (aEvent->mMessage == ePointerDown && aEvent->mMessage == eMouseDown)) {
> > >+    return aFrame;
> > >+  }
> > 
> > Looks like that |!IsPointerEventEnabled()| and |aEvent->mMessage ==
> > ePointerDown && aEvent->mMessage == eMouseDown| are makes sense in runtime
> > check. How about to use MOZ_ASSERT for checking mClass?
> We capture the pointer when handling pointerdown. Mousedown should be fired
> to the same target as pointerdown. Here we should only capture pointer and
> mouse events except pointerdown and mousedown.

Yeah, I'm not saying about what you are trying. But I'm now confused. You're checking

|(aEvent->mMessage == ePointerDown && aEvent->mMessage == eMouseDown)|

This is always false. I was thinking that you were trying to ignore events which are neither ePointerDown nor eMouseDown. Therefore, I though that you don't need to check mClass.

(In reply to Ming-Chou Shih [:stone] from comment #29)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18)
> > Comment on attachment 8910684 [details] [diff] [review]
> > Part3: Trigger pointer event implementation in EventStateManager
> > 
> > >Bug 1316251 Part3: Trigger pointer event implementation in EventStateManager
> > >diff --git a/dom/events/PointerEventUtils.cpp b/dom/events/PointerEventUtils.cpp
> > >+/* static */ void
> > >+PointerEventUtils::ProcessImplicitCapture(nsIFrame* aFrame, WidgetEvent* aEvent)
> > 
> > How about "PostHandlePointerDown"? Because others may add different job in
> > this method. (But perhaps, this should be called by PreHandleEvent and named
> > PreHandlePointerDown, isn't it?)
> 
> I'm thinking to keep the terms 'implicit capture' and 'implicit release
> pointer capture' which match the PointerEvent spec. And we won't have to
> rename them once the spec changes. Is it make sense to you?

If your method names are appear in the spec, it's nice. Let me check it.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8911742 [details]
Bug 1316251 Part1: Separate pointer event implementation from PresShell.

https://reviewboard.mozilla.org/r/183118/#review188354

::: dom/events/PointerEventHandler.cpp:92
(Diff revision 2)
> +}
> +
> +/* static */ bool
> +PointerEventHandler::IsPointerEventImplicitCaptureForTouchEnabled()
> +{
> +  return sPointerEventEnabled ? sPointerEventImplicitCapture : false;

|return sPointerEventEnabled && sPointerEventImplicitCapture;|

is simpler.

::: dom/events/PointerEventHandler.cpp:475
(Diff revision 2)
> +  localEvent.AssignPointerEventData(*aPointerEvent, true);
> +  nsresult rv = shell->HandleEventWithTarget(
> +                         &localEvent,
> +                         aCaptureTarget->GetPrimaryFrame(),
> +                         aCaptureTarget, &status);
> +  Unused << NS_WARN_IF(NS_FAILED(rv));

Shouldn't use NS_WARN_IF() with Unused. Use NS_WARNING_ASSERTION instead.

Comment 32

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #30)
> (In reply to Ming-Chou Shih [:stone] from comment #19)
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17)
> > > Comment on attachment 8910683 [details] [diff] [review]
> > > Part2: Revise pointer event implementation
> > >  
> > > >diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
> > > >@@ -3201,31 +3191,28 @@ EventStateManager::PostHandleEvent(nsPre
> > > >               fm->SetFocusedWindow(mDocument->GetWindow());
> > > >             }
> > > >           }
> > > >         }
> > > >       }
> > > >       SetActiveManager(this, activeContent);
> > > >     }
> > > >     break;
> > > >-  case ePointerCancel: {
> > > >-    if(WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent()) {
> > > >-      GenerateMouseEnterExit(mouseEvent);
> > > >-    }
> > > >-    // After firing the pointercancel event, a user agent must also fire a
> > > >-    // pointerout event followed by a pointerleave event.
> > > >-    MOZ_FALLTHROUGH;
> > > >-  }
> > > >+  case ePointerCancel:
> > > >   case ePointerUp: {
> > > 
> > > I don't understand the reason why this change is reasonable with other
> > > changes in this patch. You should explain with comment or commit message.
> > 
> > The original implementation calls GenerateMouseEnterExit to fire pointerout
> > and pointerleave when handling pointercancel. Now we only generate
> > pointercancel from touchcancel.
> 
> Changing any behavior doesn't make sense for this bug. This bug's purpose is
> refactoring of PointerEvent implementation. So, such behavior change
> shouldn't be included in this bug.  Instead, you should file a new bug and
> change the behavior for conforming to the spec.
> 
> Separating code clean up bug and changing behavior bug is important for
> regression testers.
Yes, you are right. I'll fire another bug to do this.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8911743 [details]
Bug 1316251 Part2: Revise pointer event implementation.

https://reviewboard.mozilla.org/r/183120/#review188358

::: dom/events/EventStateManager.cpp:3199
(Diff revision 2)
> -  case ePointerCancel: {
> +  case ePointerCancel:
> -    if(WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent()) {
> -      GenerateMouseEnterExit(mouseEvent);
> -    }
> -    // After firing the pointercancel event, a user agent must also fire a
> -    // pointerout event followed by a pointerleave event.
> -    MOZ_FALLTHROUGH;
> -  }
>    case ePointerUp: {
>      WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
> -    // After UP/Cancel Touch pointers become invalid so we can remove relevant helper from Table
> -    // Mouse/Pen pointers are valid all the time (not only between down/up)
>      if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
> -      mPointersEnterLeaveHelper.Remove(pointerEvent->pointerId);
> +      // After firing the pointercancel event, a user agent must also fire a
> +      // pointerout event followed by a pointerleave event.
>        GenerateMouseEnterExit(pointerEvent);
> +
> +      // After UP/Cancel Touch pointers become invalid so we can remove relevant
> +      // helper from Table. Mouse/Pen pointers are valid all the time (not only
> +      // between down/up)
> +      mPointersEnterLeaveHelper.Remove(pointerEvent->pointerId);

Hmm, if the event is ePointerCancel and the input source is nsIDOMMouseEvent::MOZ_SOURCE_TOUCH, GenerateMouseEnterExit is called twice in current code... Looks like that you're trying to fix an actual problem of our PointerEvent implementation.

And you're swapping the call order of GenerateMouseEnterExit() and mPointerEnterLeaveHelper.Remove(). I'm not investigate what means this swap, though. Anyway, I'd like you to land changes of here from another bug.

::: dom/events/PointerEventHandler.cpp:237
(Diff revision 2)
> +PointerEventHandler::GetPointerCapturingFrame(nsIFrame* aFrameUnderCursor,
> +                                              WidgetGUIEvent* aEvent)
> +{
> +  if (!IsPointerEventEnabled() || (aEvent->mClass != ePointerEventClass &&
> +                                   aEvent->mClass != eMouseEventClass) ||
> +      (aEvent->mMessage == ePointerDown && aEvent->mMessage == eMouseDown)) {

So, this line is always false.

If you're intention is |(aEvent->mMessage != ePointerDown && aEvent->mMessage != eMouseDown), you don't need to check mClass because if aEvent is ePointerDown or eMouseDown, the event class is always ePointerEventClass or eMouseEventClass.

Otherwise, my previous review's comment example of this method is wrong.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8911744 [details]
Bug 1316251 Part3: Trigger pointer event implementation in EventStateManager.

https://reviewboard.mozilla.org/r/183122/#review188370

::: dom/events/EventStateManager.cpp:716
(Diff revision 2)
>      }
>      MOZ_FALLTHROUGH;
>    case eMouseMove:
>    case ePointerDown:
>    case ePointerMove: {
> +    PointerEventHandler::ImplicitlyCapturePointer(aTargetFrame, aEvent);

Okay, if you want to keep using this name, this should be called only when ePointerDown rather than checking event message in the method since when everybody read this code, other event messages look start to capture the content too.

So, just use MOZ_ASSERT(aEvent->mMessage == ePointerDown); in ImplicitlyCapturePointer().

::: dom/events/PointerEventHandler.h:62
(Diff revision 2)
>    // Return the preference value of implicit capture.
>    static bool IsPointerEventImplicitCaptureForTouchEnabled();
>  
>    // Called in ESM::PreHandleEvent to update current active pointers in a hash
>    // table.
> -  static void UpdateActivePointerState(WidgetGUIEvent* aEvent);
> +  static void UpdateActivePointerState(WidgetMouseEvent* aEvent);

Nice!

::: dom/events/PointerEventHandler.cpp:226
(Diff revision 2)
> +      !IsPointerEventImplicitCaptureForTouchEnabled() ||
> +      aEvent->mMessage != ePointerDown) {
> +    return;
> +  }
> +  WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
> +  Unused << NS_WARN_IF(!pointerEvent);

Use NS_WARNING_ASSERTION instead.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Unfortunately, I won't work tomorrow, but perhaps, I can check only the part 2 change. Sorry, my working schedule is really unstable until next week.

Comment 36

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #35)
> Unfortunately, I won't work tomorrow, but perhaps, I can check only the part
> 2 change. Sorry, my working schedule is really unstable until next week.
Never mind and thanks for your review.

Comment 37

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #33)
> Comment on attachment 8911743 [details]
> Bug 1316251 Part2: Revise pointer event implementation.
> 
> https://reviewboard.mozilla.org/r/183120/#review188358
> 
> ::: dom/events/EventStateManager.cpp:3199
> (Diff revision 2)
> ::: dom/events/PointerEventHandler.cpp:237
> (Diff revision 2)
> > +PointerEventHandler::GetPointerCapturingFrame(nsIFrame* aFrameUnderCursor,
> > +                                              WidgetGUIEvent* aEvent)
> > +{
> > +  if (!IsPointerEventEnabled() || (aEvent->mClass != ePointerEventClass &&
> > +                                   aEvent->mClass != eMouseEventClass) ||
> > +      (aEvent->mMessage == ePointerDown && aEvent->mMessage == eMouseDown)) {
> 
> So, this line is always false.

Oops. Should be 
  if (!IsPointerEventEnabled() || (aEvent->mClass != ePointerEventClass &&
                                   aEvent->mClass != eMouseEventClass) ||
      aEvent->mMessage == ePointerDown || aEvent->mMessage == eMouseDown) {

> If you're intention is |(aEvent->mMessage != ePointerDown &&
> aEvent->mMessage != eMouseDown), you don't need to check mClass because if
> aEvent is ePointerDown or eMouseDown, the event class is always
> ePointerEventClass or eMouseEventClass.

My intention is to filter out those events that are not pointer events, mouse events, pointerdown, mousedown

Comment 38

[Ming-Chou Shih [:stone]]

Comment on attachment 8911742 [details]
Bug 1316251 Part1: Separate pointer event implementation from PresShell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183118/diff/2-3/

Comment 39

[Ming-Chou Shih [:stone]]

Comment on attachment 8911743 [details]
Bug 1316251 Part2: Revise pointer event implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183120/diff/2-3/

Comment 40

[Ming-Chou Shih [:stone]]

Comment on attachment 8911744 [details]
Bug 1316251 Part3: Trigger pointer event implementation in EventStateManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183122/diff/2-3/

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8911743 [details]
Bug 1316251 Part2: Revise pointer event implementation.

https://reviewboard.mozilla.org/r/183120/#review188720

::: dom/events/PointerEventHandler.cpp:235
(Diff revisions 2 - 3)
>    if (!IsPointerEventEnabled() || (aEvent->mClass != ePointerEventClass &&
>                                     aEvent->mClass != eMouseEventClass) ||
> -      (aEvent->mMessage == ePointerDown && aEvent->mMessage == eMouseDown)) {
> +      aEvent->mMessage == ePointerDown || aEvent->mMessage == eMouseDown) {

Okay, it makes sense to check mClass now.

However, I suggested explanation of this method in the header file with misunderstanding the intention. So, please fix the comment before landing if it's still wrong.

Comment 42

[Ming-Chou Shih [:stone]]

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

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa9c6d59bd5
Part1: Separate pointer event implementation from PresShell. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/c60d8a8d46df
Part2: Revise pointer event implementation. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0935d48846
Part3: Trigger pointer event implementation in EventStateManager. r=masayuki

Comment 44

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/aaa9c6d59bd5
https://hg.mozilla.org/mozilla-central/rev/c60d8a8d46df
https://hg.mozilla.org/mozilla-central/rev/5b0935d48846